Closed Bug 1401894 Opened 3 years ago Closed 3 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)

enhancement
Not set
normal

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)
We'll assign it in triage. These tests can target 58 and won't need to uplift to 57?
Assignee: nobody → najiang
No longer blocks: 1401777
Depends on: 1401777
Flags: needinfo?(edilee)
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)
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?
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
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)
Ah, I see the test is checking the tab favicon, and that's a nice addition.
Flags: needinfo?(mak77)
> 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
Actually, I may copy your patch part creating the separate folder and the icons... :)
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
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.
ah also note I'm modifying browser_discovery in bug 1403508, so the splitting may lose some changes. You'll need to rebase.
> 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.
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.
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 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)
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? ;)
(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 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+
Pushed by usarracini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae2fbd093aac
Add various tests for rich icon collection. r=mak
Depends on: 1405448
https://hg.mozilla.org/mozilla-central/rev/ae2fbd093aac
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.