Re: [Semanticscuttle-devel] Status for configurable privacy ....

[Christian W. <cw...@cw...>]

Hello Brett,


> I have some concerns about the fact that there were Windows
> carriage returns in my last check-in. I am working on a Windows
> environment--I will eventually set up a Linux environment, but not
> for a few weeks at least--but I've been editing files with vi on
> Cygwin and MKS Toolkit, so there should not have been any Windows
> carriage returns.
No problem. Someone noticed it, now it's fixed, all is fine :)

> Any tips/advice you have concerning this would be
> appreciated.
Sorry, I've never worked with cygwin or so. I know that many
editors/IDEs on windows are able to save files with unix newlines only,
i.e. Notepad++ and all the PHP IDEs.


> Here are a few things I noticed when testing import.php and
> importNetscape.php:
> 1. import.php points the user to http://del.icio.us/api/posts/all to
> save bookmarks to an XML file. This is evidently not a good option any
> longer for "new" del.icio.us users who are using a Yahoo! ID to access
> their bookmarks. A better option for these users is to go to the
> Delicious XML Exporter at http://deliciousxml.com/. Otherwise, they
> must use http://api.del.icio.us/v2/posts/all with open authorization,
> and that complicates the process somewhat.
Yep, we should change that.

> 2. I think I may have found a bug in importNetscape.php, which ignores
> the last bookmark in the specified HTML file. I think this is
> occurring in the regular expression given as the first parameter of
> the preg_match_all function call. Consequently, my test data contains
> a last entry that I have assumed will be ignored. The test will need
> to be updated if this behavior in importNetscape.php changes.
Ok, I'll see if I can fix that.


About your tests:
- You assume that the default privacy is configured to the default one,
  but that's something you cannot assume. If you do such assumptions,
  at least do a assertion on your assumed variable so the test fails
  with an explanatory message.
- You assume a default privacy setting since there was no way to
  configure the privacy permanently for http requests - up to now! In
  the jquery branch, I had the same problem and solved it; the
  testbaseapi class has a way to set default configuration values.
- You had one big method in BookmarksTest that checked all the
  different api methods. In general, keeping test methods small is a
  really good idea since in case of a failure, you can narrow down the
  problematic code line much faster. The rule of thumb is to test one
  thing in a test (but I don't want only one assertion in one test
  method, that'd be overkill).
- I began to split up the big test method and
  moved the api/posts_add test to their corresponding Api test
  class. By doing that I could utilize the TestBaseApi methods that
  prepare authorized HTTP requests automatically for you if you wish
  that. Do the same for the rest of the tests in there.
- In tests, always use example.org as hostname, or example.com and
  example.net (of course, you can use foo.example.org,
  example.org/foobar and such).
- You assume that new bookmarks always get ID 1 in your tests. That
  might not always be the case, especially with database systems other
  than mysql. Better than fetching bookmark ID 1 is to fetch all
  bookmarks of the user with the ID you just created which keeps you on
  the safe side.
- Assuming http://localhost/ as URL fails for my setup since I have
  "http://bm.bogo/" as URL under which my dev instance of SC is
  available. TestBaseApi takes care of this.

I cherry-picked the two relevant changes to testbaseapi into your
branch. You still need to merge master, though.

-- 
Regards/Mit freundlichen Grüßen
Christian Weiske

-=≡ Geeking around in the name of science since 1982 ≡=-