688279 - Clean up test_favicons.js

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[Richard Newman [:rnewman]]

rnewman: mak: any objection to me completely rewriting test_favicons.js?
rnewman: if I'm going to be testing async writing, the whole testnum++ stuff is going to get in the way
mak: rnewman: no, provided you don't drop some test
rnewman: would I do a thing like that? 
rnewman: will provide a mechanical patch
rnewman: for ease of review
mak: rnewman: sounds good

Comment 1

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

* Eliminate do_test_pending/do_test_finished, switching entirely to run_next_test.
* Each test is now a named function with descriptive log output; no more "test 5".
* Refactor out a bunch of helpers, allowing us to define short and sweet tests like

  add_test(function test_storing_a_normal_32x32_icon() {
    _("Test storing a normal 32x32 icon.");
  
    // 32x32 png, 344 bytes.
    let iconName   = "favicon-normal32.png";
    let inMimeType = "image/png";
    do_check_file_contents(iconName, inMimeType, 344);
    run_next_test();
  });

* Use modern JS features (e.g., let).
* Kill trailing whitespace.
* Add some comments and generally tidy up.

Same number of checks run. \o/

Comment 2

[Richard Newman [:rnewman]]

Oh, forgot to mention: it might be easier to push this patch and just read the file. The diff is kinda useless; it's a rewrite.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 561586 [details] [diff] [review]
Proposed patch. v1

Review of attachment 561586 [details] [diff] [review]:
-----------------------------------------------------------------

ah, it looks much better, may be still improved though!

::: toolkit/components/places/tests/unit/test_favicons.js
@@ +3,1 @@
>   */

nit: add a PD license and make this a line comment.

@@ +3,5 @@
>   */
>  
> +function _(s) {
> +  dump(s + "\n");
> +}

We have do_log_info(), don't we? This additional dump helper will lose

@@ +19,3 @@
>                     createInstance(Ci.nsILocalFile);
>    outputFile.initWithPath(path);
>    outputFile.append("testdump.png");

I think may use FileUtils.getFile("TmpD", ["testdump.png"]); (you can add FileUtils to head_common.js if you wish)

@@ +24,3 @@
>                       createInstance(Ci.nsIFileOutputStream);
>    // WR_ONLY|CREAT|TRUNC
>    outputStream.init(outputFile, 0x02 | 0x08 | 0x20, 0644, null);

as well as FileUtils.openFileOutputStream(outputFile, FileUtils.MODE_WRONLY | FileUtils.MODE_CREATE | FileUtils.MODE_APPEND, FileUtils.PERMS_FILE)

@@ +41,5 @@
>   * and then retrieves it with getFaviconData(). Returns
>   * and array of bytes and mimetype.
>   */
>  function setAndGetFaviconData(aFilename, aData, aMimeType) {
> +  let iconURI = uri("http://places.test/" + aFilename);

I know it's annoying, but since you are rewriting, a search and replace should allow you to replace all uri() with NetUtil.newURI()

@@ +60,5 @@
>  }
>  
> +/*
> + * Load a favicon file, verifying expected length.
> + * Return the loaded data to allow composing more complex checks.

nit: javadoc as @return The loaded data...
Actually, all function comments are not proper decorated with /**... I don't care you add all the params and such, but they may at least have the correct form :)

@@ +74,5 @@
> +
> +/*
> + * Load a non-oversized favicon file, verifying expected contents and length.
> + */
> +function do_check_file_contents(iconName, inMimeType, expectedLength) {

check_icon_data (you may use camelCase if you prefer)

@@ +88,5 @@
> + * Load an oversized favicon file, verifying expected contents and lengths
> + * against a translated PNG file.
> + * If skipContent is true, the expected and output data are not compared.
> + */
> +function do_check_oversize(iconName, inMimeType, expectedLength, skipContent) {

check_oversized_icon_data

@@ +103,5 @@
> +    checkArrays(expectedData, outData);
> +  }
> +}
> +
> +function checkArrays(a, b) {

you named all the other functions do_check_something, this one differs. Btw, for just 5 uses I'd suggest to drop this util and just use do_check_true(compareArrays(a, b)) in place, so there is no need to see what the helper checks, and is short enough

@@ +110,5 @@
> +
> +/*
> + * Retrieve and read a file, verifying the length of the returned data.
> + */
> +function readFileOfLength(name, expected) {

maybe check_icon_data_length

@@ +115,5 @@
> +  let file = do_get_file(name);
> +  let data = readFileData(file);
> +  do_check_eq(data.length, expected);
> +  return data;
> +}

Looks like setAndGetFaviconDataFromName may internally use this method, they do the same

@@ +118,5 @@
> +  return data;
> +}
> +
> +
> +// Get favicon service.

kill these comments please, too many made my eyes cry in the past

@@ +127,5 @@
>    // +/- 1 value compared to other platforms, so we need to compare against a
>    // different set of reference images. nsIXULRuntime.OS doesn't seem to be
>    // available in xpcshell, so we'll use this as a kludgy way to figure out if
>    // we're running on Windows.
>    var isWindows = ("@mozilla.org/windows-registry-key;1" in Cc);

I don't see why this is part of getting the favicon service.. btw we shouldn't need to get the service, we can just use PlacesUtils.favicons all over the test.
just keep isWindows globally, kill the try/catch and use let.

@@ +137,4 @@
>  try {
>    var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>                  getService(Ci.nsINavHistoryService);
>    var bhist = histsvc.QueryInterface(Ci.nsIBrowserHistory);

we can use PlacesUtils.bhistory all over the test, so all of this code is useless

@@ +150,4 @@
>  }
>  
> +add_test(function test_storing_a_normal_16x16_icon() {
> +  _("Test storing a normal 16x16 icon.");

actually, run_next_test already prints out the name of the function iirc, so these comments are useless unless they say something more

@@ +160,4 @@
>  });
>  
> +add_test(function test_storing_a_normal_32x32_icon() {
> +  _("Test storing a normal 32x32 icon.");

ditto, won't repeat further

@@ +281,5 @@
> +let out1Data;
> +let out2MimeType;
> +let out2Data;
> +let out3MimeType;
> +let out3Data;

hm, I think I'd prefer a global icons array with each entry like {name, iconURI, pageURI, inData, inMimeType, outData, outMimeType }

My fear is that the shared let block is easy to unwilling break or be unnoticed.

@@ +305,5 @@
> +    iconsvc.setFaviconData(icon1URI, icon1Data, icon1Data.length,
> +                           icon1MimeType, Number.MAX_VALUE);
> +  } catch (ex) {
> +    do_throw("Failure setting first page icon: " + ex);
> +  }

I suspect we'd notice an unhandled exception regardless, do we really need do_throw?

@@ +308,5 @@
> +    do_throw("Failure setting first page icon: " + ex);
> +  }
> +  iconsvc.setFaviconUrlForPage(page1URI, icon1URI);
> +  do_check_guid_for_uri(page1URI);
> +  savedIcon1URI = iconsvc.getFaviconForPage(page1URI);

does savedIcon1URI differ from icon1URI here? if not you may just compare for equality and drop the additional var

@@ +311,5 @@
> +  do_check_guid_for_uri(page1URI);
> +  savedIcon1URI = iconsvc.getFaviconForPage(page1URI);
> +
> +  // Now proceed with the test.
> +  run_next_test();

comment seems useless, rather improve the comment before the test saying it's made up of 4 parts and enumerating those

@@ +336,5 @@
> +                           icon2MimeType, Number.MAX_VALUE);
> +  } catch (ex) {}
> +  iconsvc.setFaviconUrlForPage(page2URI, icon2URI);
> +  do_check_guid_for_uri(page2URI);
> +  savedIcon2URI = iconsvc.getFaviconForPage(page2URI);

same question as before

@@ +345,5 @@
> +                           icon1MimeType, Number.MAX_VALUE);
> +  } catch (ex) {}
> +  iconsvc.setFaviconUrlForPage(page3URI, icon1URI);
> +  do_check_guid_for_uri(page3URI);
> +  savedIcon3URI = iconsvc.getFaviconForPage(page3URI);

and here

@@ +386,5 @@
> +
> +add_test(function test_favicon_links() {
> +  _("Test favicon links.");
> +
> +  let pageURI = uri("http://foo.bar/");

rather than leaving empty newlines, if you need more readability just use function bracing on newline in all the test.
add_test(function test_favicon_links()
{
  let pageURI = uri("http://foo.bar/");

@@ +394,5 @@
> +  run_next_test();
> +});
> +
> +
> +add_test(function test_failed_favicon_cache() {

nit: double newline somewhere, single somewhere else

@@ +426,5 @@
> +  let avail;
> +  while (avail = bistream.available()) {
> +    expectedData = expectedData.concat(bistream.readByteArray(avail));
> +  }
> +  bistream.close();

I think you may easily modify readFileData to detect if it gets passed in a file or a channel and support both

Comment 4

[Richard Newman [:rnewman]]

(In reply to Marco Bonardo [:mak] from comment #3)

> We have do_log_info(), don't we?

Ah yes. Every module does these things differently :D

> I know it's annoying, but since you are rewriting, a search and replace
> should allow you to replace all uri() with NetUtil.newURI()

Heh, I went in the opposite direction for the new creation tests I transplanted! Figured uri() was the idiom for this module. Will change back.

> nit: javadoc as @return The loaded data...
> Actually, all function comments are not proper decorated with /**... I don't
> care you add all the params and such, but they may at least have the correct
> form :)

Sure. I tend to use /** for the comments that are actually vaguely correct docstrings, so perhaps I'll just do that :)
 
> check_icon_data (you may use camelCase if you prefer)
> check_oversized_icon_data

I was following the pattern for checks throughout xpcshell-tests, which is do_check_eq, do_check_true, etc.

I'm happy to break that convention if you want, but only with explicit say-so!

> > +function checkArrays(a, b) {
> 
> you named all the other functions do_check_something, this one differs.

I didn't name that function.

This was mostly reorganization and refactoring; I deliberately didn't rename existing helpers.

I'm happy to stomp on more code if you're fine with it, and make the whole thing follow more of a Services style guide :D

> for just 5 uses I'd suggest to drop this util and just use
> do_check_true(compareArrays(a, b)) in place, so there is no need to see what
> the helper checks, and is short enough

wfm.

> kill these comments please, too many made my eyes cry in the past
> ...
> I don't see why this is part of getting the favicon service.. btw we

This is old code, too.

> actually, run_next_test already prints out the name of the function iirc, so
> these comments are useless unless they say something more

It does (albeit at the end of a 100-character line with the full path):

TEST-INFO | /Users/rnewman/moz/hg/services-central/obj-ff-dbg/_tests/xpcshell/toolkit/components/places/tests/unit/test_favicons.js | Starting test_getFaviconData_on_the_default_favicon
Test getFaviconData on the default favicon.

In this case I mechanically generated the function names from the old descriptions, so they happen to be identical.

> hm, I think I'd prefer a global icons array with each entry like {name,
> iconURI, pageURI, inData, inMimeType, outData, outMimeType }
> 
> My fear is that the shared let block is easy to unwilling break or be
> unnoticed.

Works for me.

> I suspect we'd notice an unhandled exception regardless, do we really need
> do_throw?

Good style. do_throw always causes the test to fail, and logs. In this case it's the logging of an informative message that's nice.

> does savedIcon1URI differ from icon1URI here? if not you may just compare
> for equality and drop the additional var

I didn't change this test code, beyond adjusting scopes to separate into different functions. I didn't analyze whether the old tests made sense, so I didn't change them :)

> rather than leaving empty newlines, if you need more readability just use
> function bracing on newline in all the test.

I disagree. Whitespace is for clarity, and is a much better choice for that than inconsistent coding style.

> nit: double newline somewhere, single somewhere else

Humph. Thought I'd caught all of those.

> I think you may easily modify readFileData to detect if it gets passed in a
> file or a channel and support both

Well, this is turning into much more than a cleanup, isn't it? :D

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Richard Newman [:rnewman] from comment #4)
> (In reply to Marco Bonardo [:mak] from comment #3)
> 
> > We have do_log_info(), don't we?
> 
> Ah yes. Every module does these things differently :D

there was a plan to move it to xpchell head.js.. not sure where it died.

> > I know it's annoying, but since you are rewriting, a search and replace
> > should allow you to replace all uri() with NetUtil.newURI()
> 
> Heh, I went in the opposite direction for the new creation tests I
> transplanted! Figured uri() was the idiom for this module. Will change back.

ideally I would like to get rid of uri() helper. useless hiding.

> > check_icon_data (you may use camelCase if you prefer)
> > check_oversized_icon_data
> 
> I was following the pattern for checks throughout xpcshell-tests, which is
> do_check_eq, do_check_true, etc.

My fear was to override a method from the harness unadvertitely, but actually wouldn't matter that much... fine if you want to keep do_check_ style.

> This is old code, too.

old code sucks, kill it

> > rather than leaving empty newlines, if you need more readability just use
> > function bracing on newline in all the test.
> 
> I disagree. Whitespace is for clarity, and is a much better choice for that
> than inconsistent coding style.

well, btw the coding style guide says brace on newline...

> Well, this is turning into much more than a cleanup, isn't it? :D

You called for it :p

Comment 6

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

Thanks for the thorough comments, mak.

Changes in this patch:
* In head_common, implemented readInputStreamData, reimplemented readFileData
  in terms of it. Use readInputStreamData directly in test; cleaner IMO than
  making readFileData polymorphic.
  All 175 xpcshell-tests pass with this change to readFileData.
* Add PD header.
* No use of _ at all. Only one use of do_log_info.
* Use FileUtils. (No import needed.)
* Lifted isWindows to top-level let.
* Eliminated checkArrays and dumpToFile.
* Eliminate top-level foosvc vars.
* Use NetUtil.newURI() everywhere instead of uri().
* Refactoring of setAndGetFaviconDataFromName.
* Rename do_check* to check*, with name improvements.
* Removed and improved comments, including JavaDoc.
* Reworked the big `let`; instead we use `icons` and `pages` at the top level,
  with explanation. All the other cross-test state sharing (e.g.,
  savedIcon1URI) has been replaced by appropriate checks and use of `icons` and
  `pages`. Much neater.

I believe this'll be a rubber-stamp, so r? please!

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 561973 [details] [diff] [review]
Proposed patch. v2

Review of attachment 561973 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/tests/head_common.js
@@ +150,5 @@
> +    }
> +    return expectedData;
> +  } finally {
> +    bistream.close();
> +  }

move let expectedData = [] and return expectedData out of the try, so all control flows have a return value

::: toolkit/components/places/tests/unit/test_favicons.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain. http://creativecommons.org/publicdomain/zero/1.0/ */

ah, someone updated the boilerplate, good to know

@@ +5,5 @@
> +// Ugh, this is an ugly hack. The pixel values we get in Windows are sometimes
> +// +/- 1 value compared to other platforms, so we need to compare against a
> +// different set of reference images. nsIXULRuntime.OS doesn't seem to be
> +// available in xpcshell, so we'll use this as a kludgy way to figure out if
> +// we're running on Windows.

Actually the second part of the comment (nsIXulRuntime...) is nonsense, this method is suggested as the best method on the xpcshell documentation

@@ +19,3 @@
>   */
>  function setAndGetFaviconData(aFilename, aData, aMimeType) {
> +  let iconsvc = PlacesUtils.favicons;

nit: I usually prefer to just use it in place without further aliases (Surely having it like Places.icons in first place would have been shorter, damn unmutable past)

@@ +225,2 @@
>  
> +  let histsvc = PlacesUtils.history;

nit: ditto

@@ +265,5 @@
> +  let [icon0, icon1] = icons;
> +  try {
> +    PlacesUtils.favicons.setFaviconData(icon1.uri, icon1.data, icon1.data.length,
> +                                        icon1.mime, Number.MAX_VALUE);
> +  } catch (ex) {}

why is this hiding errors?

@@ +274,5 @@
> +  // Set third page icon as the same as first page one.
> +  try {
> +    PlacesUtils.favicons.setFaviconData(icon0.uri, icon0.data, icon0.data.length,
> +                                        icon0.mime, Number.MAX_VALUE);
> +  } catch (ex) {}

ditto

Comment 8

[Richard Newman [:rnewman]]

Pushed to services-central with nits addressed:

  https://hg.mozilla.org/services/services-central/rev/1e00033f27c6

s-c because that's where I'll be landing favicons sync, and I can keep an eye on the process more easily than I can on the places branch.

Thanks for the detailed feedback, mak!

Comment 9

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/1e00033f27c6