Convert legacy extensions to WebExtensions in AddonManager tests that are still relevant

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Depends on 1 bug)

unspecified
mozilla61
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

a year ago
We have a bunch of Add-on Manager tests, particularly front-end tests, that test things like install flows with non-restartless legacy extensions, and only non-restartless legacy extensions. The tests that are already covered by duplicate tests for restartless add-ons can be removed, but the others need to be fixed.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8959899 - Flags: review?(aswan)

Comment 9

a year ago
mozreview-review
Comment on attachment 8959897 [details]
Bug 1446686: Part 1 - Fix browser install tests that rely on non-bootstrapped legacy extensions.

https://reviewboard.mozilla.org/r/228672/#review234786

::: browser/base/content/test/general/browser_bug553455.js:86
(Diff revision 2)
>    if (PopupNotifications.panel.childNodes.length) {
>      let nodes = Array.from(PopupNotifications.panel.childNodes);
>      let notification = nodes.find(n => n.id == notificationId + "-notification");
>      ok(notification, `Should have seen the right notification`);
> -    ok(notification.button.hasAttribute("disabled"),
> +    is(notification.button.hasAttribute("disabled"), wantDisabled,
>         "The install button should be disabled");

this message is wrong if wantDisabled is false.

::: browser/base/content/test/general/browser_bug553455.js
(Diff revision 2)
> -  let notification = panel.childNodes[0];
> -  is(notification.button.label, "Allow", "Should have seen the right button");
> -  is(notification.getAttribute("origin"), "example.com",
> -     "Should have seen the right origin host");
> -  is(notification.getAttribute("label"),
> -     gApp + " prevented this site from asking you to install software on your computer.",
> -     "Should have seen the right message");

Is showing this notification covered by another existing test?

::: browser/base/content/test/general/browser_bug553455.js:270
(Diff revision 2)
> -
>  async function test_failedDownload() {
>    let pm = Services.perms;
>    pm.add(makeURI("http://example.com/"), "install", pm.ALLOW_ACTION);
>  
> -  let progressPromise = waitForProgressNotification();
> +  let progressPromise = waitForProgressNotification(undefined, undefined, false);

what caused this to change?

::: browser/base/content/test/general/browser_bug553455.js:326
(Diff revision 2)
> -     "XPI Test could not be installed because it is not compatible with " +
> -     gApp + " " + gVersion + ".",
> +     "The add-on downloaded from this site could not be installed because " +
> +     "it appears to be corrupt.",

what caused this to change?

::: browser/base/content/test/general/browser_bug553455.js
(Diff revision 2)
>  
>    Services.perms.remove(makeURI("http://example.com/"), "install");
>    await removeTab();
>  },
>  
> -async function test_url() {

This is meant to test installing an extension by entering a remote url to the xpi in the location bar.  If we don't have another test of that somewhere else, please file a bug to test this with the webextension install flow.

::: browser/base/content/test/general/browser_bug553455.js
(Diff revision 2)
>    Services.perms.remove(makeURI("http://example.com/"), "install");
>    await loadPromise;
>    await BrowserTestUtils.removeTab(gBrowser.selectedTab);
>  },
>  
> -async function test_urlBar() {

same comment as above

::: toolkit/mozapps/extensions/test/xpinstall/browser.ini:24
(Diff revision 2)
>    signed-tampered.xpi
>    signed-untrusted.xpi

these can go now

::: toolkit/mozapps/extensions/test/xpinstall/browser.ini
(Diff revision 2)
> -[browser_navigateaway.js]
> -[browser_navigateaway2.js]
> -[browser_navigateaway3.js]
> -skip-if = (os == "mac" || os == "win") # Bug 1198261
> -[browser_navigateaway4.js]

can you file bugs to add test coverage for the things these tests covered, but with the webextensions install flow

::: toolkit/mozapps/extensions/test/xpinstall/browser.ini
(Diff revision 2)
> -[browser_whitelist.js]
> -[browser_whitelist2.js]
> -[browser_whitelist3.js]
> -[browser_whitelist4.js]
> -[browser_whitelist5.js]
> -[browser_whitelist6.js]
> -[browser_whitelist7.js]

please file a new bug to add tests covering this but with the webextension install flow

Comment 10

a year ago
mozreview-review
Comment on attachment 8959898 [details]
Bug 1446686: Part 2 - Remove tests for non-bootstrapped dictionary add-ons.

https://reviewboard.mozilla.org/r/228674/#review234866

Was the browser_install stuff meant to be included in this patch?
Regardless, I don't understand the intention, you updated the xpis used by the test but then just removed the test.
Either the test needs to be updated or the correspodning xpis should be removed.
Assignee

Comment 11

a year ago
mozreview-review-reply
Comment on attachment 8959897 [details]
Bug 1446686: Part 1 - Fix browser install tests that rely on non-bootstrapped legacy extensions.

https://reviewboard.mozilla.org/r/228672/#review234786

> Is showing this notification covered by another existing test?

No. I should probably fix the fact that this test fails when installing a WebExtension.

> what caused this to change?

It's not a change. The other instances are changed, because the button in question is not disabled when installing WebExtensions. This just keeps the old check the same for bootstrapped extensions.

> what caused this to change?

This should actually probably be in the last patch... With the pref to disable non-bootstrapped add-ons flipped, we throw when parsing non-bootstrapped manifests, so it gets treated as invalid rather than incompatible.
Assignee

Comment 12

a year ago
mozreview-review-reply
Comment on attachment 8959898 [details]
Bug 1446686: Part 2 - Remove tests for non-bootstrapped dictionary add-ons.

https://reviewboard.mozilla.org/r/228674/#review234866

I realized that the entire test was useless, since its entire purpose is to test installs that require restart, after I finished updating the signed add-ons.

I'm not sure how those changes wound up in this commit. I probably botched a rebase when I was trying to remove the new signed add-ons.
Assignee

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8959897 [details]
Bug 1446686: Part 1 - Fix browser install tests that rely on non-bootstrapped legacy extensions.

https://reviewboard.mozilla.org/r/228672/#review234786

> please file a new bug to add tests covering this but with the webextension install flow

This is all basically tested by the aptly-named browser_bug553455. These are all just massive overkill.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 18

a year ago
mozreview-review-reply
Comment on attachment 8959897 [details]
Bug 1446686: Part 1 - Fix browser install tests that rely on non-bootstrapped legacy extensions.

https://reviewboard.mozilla.org/r/228672/#review234786

> can you file bugs to add test coverage for the things these tests covered, but with the webextensions install flow

I actually think that having these tests at all is probably a bad idea. This behavior is handled by the PopupNotifications code. It should be tested there, but we shouldn't re-test every aspect of it in add-on manager tests.

Comment 19

a year ago
mozreview-review-reply
Comment on attachment 8959897 [details]
Bug 1446686: Part 1 - Fix browser install tests that rely on non-bootstrapped legacy extensions.

https://reviewboard.mozilla.org/r/228672/#review234786

> I actually think that having these tests at all is probably a bad idea. This behavior is handled by the PopupNotifications code. It should be tested there, but we shouldn't re-test every aspect of it in add-on manager tests.

The UI aspect is handled in PopupNotifications but addons-specific code still has to react to that and properly clean up the pending installation.  Or in other words, we should have tests that this code and everything downstream from it is working correctly:
https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/modules/ExtensionsUI.jsm#398-402

Comment 20

a year ago
mozreview-review-reply
Comment on attachment 8959897 [details]
Bug 1446686: Part 1 - Fix browser install tests that rely on non-bootstrapped legacy extensions.

https://reviewboard.mozilla.org/r/228672/#review234786

> This is all basically tested by the aptly-named browser_bug553455. These are all just massive overkill.

Ah indeed.  You've already slogged through a lot of stuff in this bug but can I persuade you to move that file (browser_bug553455.js) to this directory and give it a better name at the same time?

Comment 21

a year ago
mozreview-review
Comment on attachment 8959897 [details]
Bug 1446686: Part 1 - Fix browser install tests that rely on non-bootstrapped legacy extensions.

https://reviewboard.mozilla.org/r/228672/#review235160

::: browser/base/content/test/general/browser_bug553455.js
(Diff revision 3)
> -async function test_urlBar() {
> -  let progressPromise = waitForProgressNotification();
> -  let dialogPromise = waitForInstallDialog();
> -
> -  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser, "about:blank");
> -  await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> -  gURLBar.value = TESTROOT + "amosigned.xpi";
> -  gURLBar.focus();
> -  EventUtils.synthesizeKey("KEY_Enter");
> -
> -  await progressPromise;
> -  let installDialog = await dialogPromise;
> -
> -  let notificationPromise = waitForNotification("addon-install-restart");
> -  acceptInstallDialog(installDialog);
> -  let panel = await notificationPromise;
> -
> -  let notification = panel.childNodes[0];
> -  is(notification.button.label, "Restart Now", "Should have seen the right button");
> -  is(notification.getAttribute("label"),
> -     "XPI Test will be installed after you restart " + gApp + ".",
> -     "Should have seen the right message");
> -
> -  let installs = await getInstalls();
> -  is(installs.length, 1, "Should be one pending install");
> -  installs[0].cancel();
> -
> -  await removeTab();
> -},

I don't think we have another test that covers this scenario, it should be updated to use the webextension flow, not removed.

::: toolkit/mozapps/extensions/test/browser/browser_install.rdf:1
(Diff revision 3)
>  <?xml version="1.0" encoding="UTF-8"?>

This file (as well as browser_install1_3.xpi) can just be deleted now
Attachment #8959897 - Flags: review?(aswan) → review+

Comment 22

a year ago
mozreview-review
Comment on attachment 8959898 [details]
Bug 1446686: Part 2 - Remove tests for non-bootstrapped dictionary add-ons.

https://reviewboard.mozilla.org/r/228674/#review235162
Attachment #8959898 - Flags: review?(aswan) → review+

Comment 23

a year ago
mozreview-review
Comment on attachment 8959899 [details]
Bug 1446686: Part 3 - Fix discovery pane and drag-and-drop tests which rely on legacy extensions.

https://reviewboard.mozilla.org/r/228676/#review235168

Why not convert the discovery tests to mozAddonManager instead of removing them

::: toolkit/mozapps/extensions/test/browser/browser_file_xpi_no_process_switch.js:47
(Diff revision 3)
> +window.addEventListener("popupshown", ({target}) => {
> +  dump(`MEH ${target.outerHTML}\n`);
> +}, true);
> +

remove
Assignee

Comment 24

a year ago
mozreview-review-reply
Comment on attachment 8959899 [details]
Bug 1446686: Part 3 - Fix discovery pane and drag-and-drop tests which rely on legacy extensions.

https://reviewboard.mozilla.org/r/228676/#review235168

Because those tests are appalling, and if we want to test the disco pane with mozAddonManager and don't already have tests for that, we should just write new ones. But we're not losing coverage by removing these tests, since they don't test anything that we care about (and fail when we try to use them with WebExtensions), so I don't think this bug should block on it.

Comment 25

a year ago
mozreview-review-reply
Comment on attachment 8959899 [details]
Bug 1446686: Part 3 - Fix discovery pane and drag-and-drop tests which rely on legacy extensions.

https://reviewboard.mozilla.org/r/228676/#review235168

That's fine by me, can you please file a bug for it so it doesn't get forgotten
Assignee

Comment 26

a year ago
(In reply to Andrew Swan [:aswan] from comment #25)
> That's fine by me, can you please file a bug for it so it doesn't get
> forgotten

Yes, I'll file follow-up bugs for all of the WebExtension install flow tests we need to add before this lands.
Assignee

Updated

a year ago
See Also: → 1447427
Assignee

Updated

a year ago
See Also: → 1447429
Assignee

Updated

a year ago
Keywords: leave-open
Assignee

Comment 27

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/73534eb3eb8c5820e2332c33e0a188761aa88c0b
Bug 1446686: Part 1 - Fix browser install tests that rely on non-bootstrapped legacy extensions. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/04b3e7cdabb262242a65e315530b4211b3c7b6cf
Bug 1446686: Part 2 - Remove tests for non-bootstrapped dictionary add-ons. r=aswan
Assignee

Updated

a year ago
Blocks: 1447903
Assignee

Comment 28

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/284dcbf88f1147e407d0f04d8aca08b23a7ba5dd
Bug 1446686: Follow-up: Skip browser_httphash6 on win/mac opt/verify for frequent intermittent failures. r=bustage CLOSED TREE
Assignee

Comment 29

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/76d7d6012a152d4a1a9088d54b46bcd3b5c23e3b
Bug 1446686: Follow-up: Fix sync tests that load head_addons.js and fixture add-ons from add-on manager tests. r=they're-lucky-i-don't-just-disable-the-tests,test-only

Comment 31

a year ago
mozreview-review
Comment on attachment 8959899 [details]
Bug 1446686: Part 3 - Fix discovery pane and drag-and-drop tests which rely on legacy extensions.

https://reviewboard.mozilla.org/r/228676/#review236656
Attachment #8959899 - Flags: review?(aswan) → review+

Comment 32

a year ago
mozreview-review
Comment on attachment 8959900 [details]
Bug 1446686: Part 4 - Disable non-restartless extensions by default, with a pref to enable for xpcshell tests.

https://reviewboard.mozilla.org/r/228678/#review236660
Attachment #8959900 - Flags: review?(aswan) → review+
Assignee

Comment 34

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d854ebbcbb7e39b29508c856b118ca52f6623d53
Bug 1446686: Part 3 - Fix discovery pane and drag-and-drop tests which rely on legacy extensions. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6162e512477c86e920dd24b27522526c23cf6b
Bug 1446686: Part 4 - Disable non-restartless extensions by default, with a pref to enable for xpcshell tests. r=aswan
Assignee

Comment 35

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e5efe49f034c364260d5f1e0964060693c50d7e
Bug 1446686: Follow-up: Fix pending permission prompts when prompt is dismissed. r=bustage
Assignee

Comment 38

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca332e5906164f20150063d72002a6ca9b2268d8
Bug 1446686: Follow-up: Temporarily skip test_install.js on Windows for too frequent intermittent failures. r=me DONTBUILD
Depends on: 1449071
Assignee

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Updated

a year ago
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.