Closed Bug 1448057 Opened 4 years ago Closed 4 years ago

Modernize some favicon tests, removing a custom addVisits helper and adding a test for icons on bookmark redirects

Categories

(Toolkit :: Places, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: standard8, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

There is an old `addVisits` helper function in toolkit/components/places/tests/browser/head.js that we should replace by PlacesTestUtils.addVisits.

Here's where the old file is used:

https://searchfox.org/mozilla-central/search?q=%5CsaddVisits&case=false&regexp=true&path=toolkit%2Fcomponents%2Fplaces%2Ftests%2Fbrowser

This is the new API:

https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/toolkit/components/places/tests/PlacesTestUtils.jsm#17-35

Note the new API is async [1] where as the old one is sync based.

The smaller fix would be to use `PlacesTestUtils.addVisits().then(callback)`, but a nicer fix might be to rewrite the tests to use async/await like we do for the other tests. For example `function run_test()` would change to `add_task(async function test_description()`, allowing the use of await.

[1] https://developer.mozilla.org/docs/Web/JavaScript/Reference/Statements/async_function
Hi Mark,

I'd like to take on this task.
I'm new to Open Source contributions, is that ok?
If that's ok, it would be awesome if you could give some directions on how to begin with this task.

Thanks!
Hi Rafael, thanks for the offer.

If you haven't already checked out the source code & built it, then that's a good place to start. See these for more details:

https://developer.mozilla.org/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
https://developer.mozilla.org/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

Note: you can do an artifact build here rather than a full build, as it'll be quicker, and it is all you need as you're just changing tests.

Then try doing the changes detailed in comment 0. Run the tests before and after with `./mach mochitest path/to/test`

You can then attach patches to the bug for review (add "r?Standard8" onto the commit message): https://developer.mozilla.org/docs/Mozilla/Developer_guide/Introduction#Step_4_Get_your_code_reviewed

If there's any questions along the way, please ask.

We generally only assign the bug when the first patch is attached.
Hi Rafael, one of my co-developers, Marco, has just informed me that he's currently rewriting these tests for another bug.

They're not as useful as they could be, and they cold be made much simpler.

So I'm going to pass this bug along to him.

We're sorry about that, I don't have other bugs to recommend at the moment, but you should be able to find more via https://www.joshmatthews.net/bugsahoy/.
Assignee: nobody → mak77
Mentor: standard8
Whiteboard: [fxsearch][lang=js] → [fxsearch]
Thanks, I'm sorry but I was looking at an issue and I hit the horrible code in these tests. Since it's not a trivial change I preferred to just take it.
Status: NEW → ASSIGNED
I'm sorry this patch ended up much larger than I was expecting because moving tests to xpcshell exposed a bug where we fetch icons from the network even before knowing if we can store them. And then the moved code was not covered by a test, so I had to add one...
Summary: Replace addVisits helper with PlacesTestUtils.addVisits → Modernize some favicon tests, removing a custom addVisits helper and adding a test for icons on bookmark redirects
Comment on attachment 8965613 [details]
Bug 1448057 - Asincify some Places tests and add a test for favicons on bookmark redirects.

https://reviewboard.mozilla.org/r/234466/#review240226

Generally looks good, but not quite r+ as I'd like to check again after the issue I raised in test_setAndFetchFaviconForPage_failures.js is resolved.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_favicon.js:218
(Diff revision 2)
>    await assignCookies(gBrowser, TEST_SITE, cookies[1]);
>  
>    // Add the observer earlier in case we don't capture events in time.
>    let promiseObserveFavicon = observeFavicon(true, cookies[0], pageURI);
>  
> +  // The page must be bookmarks for favicon requests to go through in PB mode.

nit: s/bookmarks/bookmarked/

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_favicon.js:277
(Diff revision 2)
>    is(response.privateBrowsingId, 0, "We should observe the network response for the non-private tab.");
>  
>    // Create a private browsing window.
>    let privateWindow = await BrowserTestUtils.openNewBrowserWindow({ private: true });
>  
> +  // The page must be bookmarks for favicon requests to go through in PB mode.

nit: s/bookmarks/bookmarked/

::: toolkit/components/places/FaviconHelpers.cpp:554
(Diff revision 2)
>                                (mIcon.fetchMode == FETCH_IF_MISSING && isInvalidIcon);
>  
> +  // Check if we can associate the icon to this page.
> +  rv = FetchPageInfo(DB, mPage);
> +  if (NS_FAILED(rv)) {
> +    if (rv == NS_ERROR_NOT_AVAILABLE){

nit: missing space before {

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage_failures.js:19
(Diff revision 2)
> -    stmt.finalize();
> -  }
>  
> -  function testNullPageURI(aWindow, aCallback) {
> +  info("Test null page uri");
> -    try {
> +  try {
> -      aWindow.PlacesUtils.favicons.setAndFetchFaviconForPage(null, favIcon16URI,
> +    PlacesUtils.favicons.setAndFetchFaviconForPage(null, faviconURI,

Please could you change this to Assert.throws? (and provide the expected exception).

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage_failures.js:30
(Diff revision 2)
> -    }
> -  }
> -
> -  function testNullFavIconURI(aWindow, aCallback) {
> -    try {
> +  try {
> -      aWindow.PlacesUtils.favicons.setAndFetchFaviconForPage(
> +    PlacesUtils.favicons.setAndFetchFaviconForPage(

Assert.throws.

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage_failures.js:87
(Diff revision 2)
> -        });
> -        stmt.finalize();
> -    });
> -
> -    // This is the only test that should cause the waitForFaviconChanged
> +  // This is the only test that should cause the waitForFaviconChanged
> -    // callback to be invoked.  In turn, the callback will invoke
> +  // callback to be invoked.

From the test perspective, how do we know this to be true?

AFAICT there's nothing checking that we haven't received the onPageChanged notification.

Should we keep the database check for the icons in the database?

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage_redirects.js:38
(Diff revision 2)
> +    SMALLPNG_DATA_URI, true, PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE, null,
> +    Services.scriptSecurityManager.getSystemPrincipal());
> +
> +  await promise;
> +
> +  // The favicon should  be set also on the bookmarked url that redirected.

nit: double space in "should  be"

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage_redirects.js:70
(Diff revision 2)
> +
> +  PlacesUtils.favicons.setAndFetchFaviconForPage(Services.io.newURI(destUrl),
> +    SMALLPNG_DATA_URI, true, PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE, null,
> +    Services.scriptSecurityManager.getSystemPrincipal());
> +
> +  await Assert.rejects(promise);

Please can you add an expected parameter here? I realise it might be a bit harder, but I'd like to make sure we're asserting the right thing.
Attachment #8965613 - Flags: review?(standard8)
Comment on attachment 8965613 [details]
Bug 1448057 - Asincify some Places tests and add a test for favicons on bookmark redirects.

https://reviewboard.mozilla.org/r/234466/#review240996

Great, thanks for the update. r=Standard8
Attachment #8965613 - Flags: review?(standard8) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/ad061f4fec73
Asincify some Places tests and add a test for favicons on bookmark redirects. r=standard8
https://hg.mozilla.org/mozilla-central/rev/ad061f4fec73
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.