Permissions tests cleanups

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: aswan, Assigned: aswan)

Tracking

51 Branch
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

10 months ago
The unit tests for webextension permission prompts started small but they've grown fairly large with a lot of repeated code across files and an unfortunate requestLongerTimeout() in browser_extension_permissions.js since it wasn't easy to split that file up without copying a ton of code.
This bug is to consider moving all the permissions tests to their own directory (or at least their own .ini file) so they can have their own custom head.js with all the shared code and we can make the individual test files much more fine grained.
(Assignee)

Updated

10 months ago
Assignee: nobody → aswan
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8840681 - Flags: review?(florian)
Attachment #8840682 - Flags: review?(florian)
Attachment #8840683 - Flags: review?(florian)
(Assignee)

Updated

10 months ago
Attachment #8840681 - Flags: review?(florian) → review?(dtownsend)
Attachment #8840682 - Flags: review?(florian) → review?(dtownsend)
Attachment #8840683 - Flags: review?(florian) → review?(dtownsend)

Comment 4

10 months ago
mozreview-review
Comment on attachment 8840681 [details]
Bug 1340586 Part 1 Move webextensions tests to their own directory

https://reviewboard.mozilla.org/r/115114/#review116900
Attachment #8840681 - Flags: review?(dtownsend) → review+

Comment 5

10 months ago
mozreview-review
Comment on attachment 8840682 [details]
Bug 1340586 Part 2 Move common code to head.js

https://reviewboard.mozilla.org/r/115116/#review116906

::: browser/base/content/test/webextensions/browser_extension_sideloading.js:246
(Diff revision 1)
>    is(addon2.userDisabled, false, "Addon 2 should now be enabled");
>  
>    yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
>  });
> +
> +add_task(() => cleanup());

So rather than doing this how about having a global cleanup variable in head.js that you can assign a function to to be run at the start of the cleanup function?
Attachment #8840682 - Flags: review?(dtownsend)

Comment 6

10 months ago
mozreview-review
Comment on attachment 8840683 [details]
Bug 1340586 Part 3 Split up browser_extension_permissions.js

https://reviewboard.mozilla.org/r/115118/#review116908

I didn't look too closely at this as I'm assuming it is all just copy and paste.
Attachment #8840683 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 7

10 months ago
mozreview-review-reply
Comment on attachment 8840682 [details]
Bug 1340586 Part 2 Move common code to head.js

https://reviewboard.mozilla.org/r/115116/#review116906

> So rather than doing this how about having a global cleanup variable in head.js that you can assign a function to to be run at the start of the cleanup function?

Good idea, this is much nicer.
(Assignee)

Comment 8

10 months ago
mozreview-review-reply
Comment on attachment 8840683 [details]
Bug 1340586 Part 3 Split up browser_extension_permissions.js

https://reviewboard.mozilla.org/r/115118/#review116908

Yep
excited about the r+'s here- looking forward to this landing as it should help out a lot with bug 1333713.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

10 months ago
mozreview-review
Comment on attachment 8840682 [details]
Bug 1340586 Part 2 Move common code to head.js

https://reviewboard.mozilla.org/r/115116/#review117182
Attachment #8840682 - Flags: review?(dtownsend) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

10 months ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/096a249febe7
Part 1 Move webextensions tests to their own directory r=mossop
https://hg.mozilla.org/integration/autoland/rev/c8bcd3a86de6
Part 2 Move common code to head.js r=mossop
https://hg.mozilla.org/integration/autoland/rev/06efe0295e1e
Part 3 Split up browser_extension_permissions.js r=mossop

Comment 17

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/096a249febe7
https://hg.mozilla.org/mozilla-central/rev/c8bcd3a86de6
https://hg.mozilla.org/mozilla-central/rev/06efe0295e1e
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1343645

Updated

9 months ago
See Also: → bug 1343612
You need to log in before you can comment on or make changes to this bug.