1340586 - Permissions tests cleanups

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Andrew Swan [:aswan]]

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.

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1340586 Part 1 Move webextensions tests to their own directory

Review commit: https://reviewboard.mozilla.org/r/115114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115114/

Comment 2

[Andrew Swan [:aswan]]

Attachment: Bug 1340586 Part 2 Move common code to head.js

Review commit: https://reviewboard.mozilla.org/r/115116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115116/

Comment 3

[Andrew Swan [:aswan]]

Attachment: Bug 1340586 Part 3 Split up browser_extension_permissions.js

Review commit: https://reviewboard.mozilla.org/r/115118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115118/

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 8840681 [details]
Bug 1340586 Part 1 Move webextensions tests to their own directory

https://reviewboard.mozilla.org/r/115114/#review116900

Comment 5

[Dave Townsend [:mossop]]

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?

Comment 6

[Dave Townsend [:mossop]]

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.

Comment 7

[Andrew Swan [:aswan]]

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.

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8840683 [details]
Bug 1340586 Part 3 Split up browser_extension_permissions.js

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

Yep

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

excited about the r+'s here- looking forward to this landing as it should help out a lot with bug 1333713.

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8840682 [details]
Bug 1340586 Part 2 Move common code to head.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115116/diff/1-2/

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8840683 [details]
Bug 1340586 Part 3 Split up browser_extension_permissions.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115118/diff/1-2/

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8840682 [details]
Bug 1340586 Part 2 Move common code to head.js

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

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8840681 [details]
Bug 1340586 Part 1 Move webextensions tests to their own directory

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115114/diff/1-2/

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8840682 [details]
Bug 1340586 Part 2 Move common code to head.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115116/diff/2-3/

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8840683 [details]
Bug 1340586 Part 3 Split up browser_extension_permissions.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115118/diff/2-3/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/096a249febe7
https://hg.mozilla.org/mozilla-central/rev/c8bcd3a86de6
https://hg.mozilla.org/mozilla-central/rev/06efe0295e1e