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/.

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.

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...

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.

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

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