Closed
Bug 1401894
Opened 7 years ago
Closed 7 years ago
Create a test checking that we don't store mask favicons and don't use rich favicons as the tab icon
Categories
(Firefox :: Tabbed Browser, enhancement)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: mak, Assigned: nanj)
References
Details
Attachments
(1 file)
This bug is about having a test that loads a page with a bunch of icon definitions, and checks that mask icons are not stored in Places and rich icons are not used as the icon for a tab. Mardak, would someone in the AS team have time to write this?
Flags: needinfo?(edilee)
Comment 1•7 years ago
|
||
We'll assign it in triage. These tests can target 58 and won't need to uplift to 57?
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → najiang
Assignee | ||
Updated•7 years ago
|
Updated•7 years ago
|
Flags: needinfo?(edilee)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Hey mak, I've added various rich icon related tests in this patch. Wonder if it's easier to fix bug 1402373 on top of this one, I mean to add more other tests based on it.
Flags: needinfo?(mak77)
Assignee | ||
Comment 4•7 years ago
|
||
The try run looks fine https://treeherder.mozilla.org/#/jobs?repo=try&revision=7204bddc0dcc13f34d7c6eae1900bd16cb37e18b. I haven't touched those disabled tests in the "browser_discovery.js" though, perhaps we can move them to this test suite?
Reporter | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8913000 [details] Bug 1401894 - Add various tests for rich icon collection. https://reviewboard.mozilla.org/r/184364/#review189752 This test doesn't really differ much from browser_discovery.js, wouldn't that mean it could still be intermittent? The try run you got isn't enough to exclude an intermittent, you should probably: ./mach try: -b do -p win32,win64,linux64,macosx64 -u none -t none --artifact --rebuild 20 browser/base/content/test/richIcons/ How much additional testing this provides compared to adding a mask test to browser_discovery.js and investigating/solving its intermittent? Fwiw, I'd probably split browser_discovery.js in browser_discovery_icons.js and browser_discovery_search.js and then move the former into a favicons folder. I indeed appreciate the idea of a favicons/ subfolder where to move related tests... though this refactoring isn't likely to help much bug 1402373, it will still need a separate dedicated test. ::: browser/base/content/test/richIcons/.eslintrc.js:2 (Diff revision 1) > +"use strict"; > + I'd probably rename the folder to favicons/, so that we can then move some more tests from general/ to this subfolder and split them better. ::: browser/base/content/test/richIcons/browser_rich_icons.js:10 (Diff revision 1) > + > +/* eslint-disable mozilla/no-arbitrary-setTimeout */ > + > +add_task(async function test() { > + const rootDir = getRootDirectory(gTestPath); > + const URL = rootDir + "file_rich_icon.html"; Please use http://mochi.test... The reason is that it you use the file:// protocol things will be less close to the reality of our users. on file scheme we skip some icons work, for example we don't fetch the root icon. ::: browser/base/content/test/richIcons/browser_rich_icons.js:50 (Diff revision 1) > + // favicon loads, we have to wait some time before checking that icon was > + // stored properly. > + await BrowserTestUtils.waitForCondition(() => { > + tabIconUri = gBrowser.getIcon(); > + return tabIconUri != null; > + }, "wait for icon load to finish", 100, 5); In the discovery test I'm bumping this to 100, 20, it was intermittently failing otherwise ::: browser/base/content/test/richIcons/browser_rich_icons.js:63 (Diff revision 1) > + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, URL); > + > + try { > + await BrowserTestUtils.waitForCondition(() => { > + return gBrowser.getIcon() != null; > + }, "wait for icon load to finish", 100, 5); ditto
Reporter | ||
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8913000 [details] Bug 1401894 - Add various tests for rich icon collection. https://reviewboard.mozilla.org/r/184364/#review189766
Attachment #8913000 -
Flags: review?(mak77)
Reporter | ||
Comment 7•7 years ago
|
||
Ah, I see the test is checking the tab favicon, and that's a nice addition.
Flags: needinfo?(mak77)
Assignee | ||
Comment 8•7 years ago
|
||
> This test doesn't really differ much from browser_discovery.js, wouldn't > that mean it could still be intermittent? > The try run you got isn't enough to exclude an intermittent, you should > probably: > ./mach try: -b do -p win32,win64,linux64,macosx64 -u none -t none --artifact > --rebuild 20 browser/base/content/test/richIcons/ > Good point, just kicked off a new 20 run try test. > How much additional testing this provides compared to adding a mask test to > browser_discovery.js and investigating/solving its intermittent? > Fwiw, I'd probably split browser_discovery.js in browser_discovery_icons.js > and browser_discovery_search.js and then move the former into a favicons > folder. So this test checks, 1. mask icon will be ignored. 2. rich icon won't be loaded for the tab. Yes, I think it makes sense to split the "browser_discovery.jsm" and move rich icon tests to this directory, will do. > > I indeed appreciate the idea of a favicons/ subfolder where to move related > tests... though this refactoring isn't likely to help much bug 1402373, it > will still need a separate dedicated test. I thought it might be helpful to at least facilitate the test for bug 1402373, looks like you already have a better plan, I'll leave that to you :P
Reporter | ||
Comment 9•7 years ago
|
||
Actually, I may copy your patch part creating the separate folder and the icons... :)
Assignee | ||
Comment 10•7 years ago
|
||
Sure, go ahead :) So the repeat-20 try run just came back, looks like there is no intermittents so far. https://treeherder.mozilla.org/#/jobs?repo=try&revision=972a407cc3721b04a60f2f5d53844328279c6aaf I've also splitted the "browser_discovery.js" into "icon" and "search", and re-enabled those rich icon tests, bravely pushed to try again. Let's see what happens ;) https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbb05ee84b95e534933fbfec039a229268ab797c
Reporter | ||
Comment 11•7 years ago
|
||
cool, that sounds nice. maybe it's just the interaction with the other tests to cause the intermittent. I added a favicons/ folder to browser/base/content/test. if you use the same name it should be trivial to rebase.
Reporter | ||
Comment 12•7 years ago
|
||
ah also note I'm modifying browser_discovery in bug 1403508, so the splitting may lose some changes. You'll need to rebase.
Assignee | ||
Comment 13•7 years ago
|
||
> I added a favicons/ folder to browser/base/content/test. if you use the same
> name it should be trivial to rebase.
So did I. Look, race is everywhere ;)
No worries, rebase comes to rescue.
Assignee | ||
Comment 14•7 years ago
|
||
So the splitted test just passed the repeat-20 try run, sounds quite promising. Running the whole test on try again. mak, do you want me to wait your patch for bug 1402373 to avoid code conflict? I believe that one is more urgent.
Reporter | ||
Comment 15•7 years ago
|
||
I just set a r? on you there. Also bug 1403508 is pending review. I think at this point we should wait for both, shouldn't take much.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8913000 [details] Bug 1401894 - Add various tests for rich icon collection. https://reviewboard.mozilla.org/r/184364/#review190534 ::: browser/base/content/test/favicons/browser.ini:5 (Diff revision 2) > [DEFAULT] > support-files = > head.js > discovery.html > + file_icon_discovery.html you should be able to just reuse discovery.html that already exists here, instead of adding another file. ::: browser/base/content/test/favicons/browser_icon_discovery.js:4 (Diff revision 2) > +/* > + * Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ > + */ please use the official boilerplate from https://www.mozilla.org/en-US/MPL/headers/ ::: browser/base/content/test/favicons/browser_icon_discovery.js:6 (Diff revision 2) > +/* > + * Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ > + */ > + > +/* eslint-disable mozilla/no-arbitrary-setTimeout */ I'd probably hg cp browser_discovery.js to this, and remove the non relevant parts, so we can preserve the blame information on the kept lines ::: browser/base/content/test/favicons/browser_rich_icons.js:4 (Diff revision 2) > +/* > + * Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ > + */ ditto about the boilerplate ::: browser/base/content/test/favicons/browser_rich_icons.js:42 (Diff revision 2) > + }); > + > + const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, URL); > + > + await Promise.race([promiseMessage, > + new Promise((resolve, reject) => setTimeout(reject, 2000))]); we always expect these icons to exist and message (there is no unknown icon test), so I wonder if we should have a timeout here at all? ::: browser/base/content/test/favicons/browser_rich_icons.js:56 (Diff revision 2) > + }, "wait for icon load to finish", 100, 20); > + is(tabIconUri, EXPECTED_ICON, "should use the non-rich icon for the tab"); > + await BrowserTestUtils.removeTab(tab); > +}); > + > +add_task(async function test() { both of the test functions are named "test", please pick better names
Attachment #8913000 -
Flags: review?(mak77)
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8913000 [details] Bug 1401894 - Add various tests for rich icon collection. https://reviewboard.mozilla.org/r/184364/#review190576 ::: browser/base/content/test/favicons/browser_rich_icons.js:42 (Diff revision 2) > + }); > + > + const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, URL); > + > + await Promise.race([promiseMessage, > + new Promise((resolve, reject) => setTimeout(reject, 2000))]); Oh, I stole your Promise.race trick from the other test and applied it here so that this test (if that promise failed to resolve somehow) will hit the timeout error sooner, otherwise, it'll hang forever if running it locally. Did I mis-interpreted the purpose of that Promise.race trick? ;)
Reporter | ||
Comment 19•7 years ago
|
||
(In reply to Nan Jiang [:nanj] from comment #18) > Oh, I stole your Promise.race trick from the other test and applied it here > so that this test (if that promise failed to resolve somehow) will hit the > timeout error sooner, otherwise, it'll hang forever if running it locally. > > Did I mis-interpreted the purpose of that Promise.race trick? ;) That race trick is for those tests where an icon is not expected, and then we'd never get a message.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8913000 [details] Bug 1401894 - Add various tests for rich icon collection. https://reviewboard.mozilla.org/r/184364/#review191012
Attachment #8913000 -
Flags: review?(mak77) → review+
Comment 22•7 years ago
|
||
Pushed by usarracini@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ae2fbd093aac Add various tests for rich icon collection. r=mak
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae2fbd093aac
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in
before you can comment on or make changes to this bug.
Description
•