1251412 - Fix the addon-sdk tests to pass with add-on signing

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Andy McKay]

Mossop: During the build process we use cfx to build these add-ons https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/addons
Mossop: Then they get loaded in the harness: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/jetpack-addon-harness.js#30
Mossop: Either checking in built versions that are signed or switching the harness to use loadTemporaryAddon would fix those tests

andym: cfx add-ons can't be signed through AMO, if we can use loadTemporaryAddon that's probably the easiest step

Comment 1

[Andrew Swan [:aswan]]

Attachment: MozReview Request: Bug 1251412 - use installTemporaryAddon for jetpack-addons tests

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

Comment 2

[Andrew Swan [:aswan]]

Comment on attachment 8723837 [details]
MozReview Request: Bug 1251412 - use installTemporaryAddon for jetpack-addons tests

This patch implements the basic mechanics but it causes several new failures and a noisy warning.
The failures are:
https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/addon-sdk/source/test/addons/main/main.js#17
With the temporary install, the loadReason is "enable" instead of "install" as the test expects.

https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/addon-sdk/source/test/addons/unpacked/main.js#10
With the temporary install, packed is true, instead of false as the test expects

https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/addon-sdk/source/test/addons/unpacked/main.js#14
With the temporary install, toFilename now throws this error:
Error: cannot map to filename: resource://test-url-at-jetpack/unpacked/lib/main.js

I suspect the first is actually undesirable for temporary installs and should be fixed, that we should change the test expectation for the second, and I have no idea about the third.

In addition to those, we also get these warnings every time an addon is installed:
1456444058018	addons.manager	WARN	AddonListener threw exception when calling onInstalled: TypeError: that.sandboxes[sandboxName] is undefined (chrome://marionette/content/driver.js:1090:1) JS Stack trace: GeckoDriver.prototype.executeWithCallback/res</[e-mail]:1090:1 < __marionetteFunc/[e-mail]:13:19 < [e-mail]:1723:11 < [e-mail]:2832:5 < this.XPIProvider.installTemporaryAddon<@XPIProvider.jsm:3879:5

I got lost after a few minutes of trying to figure out what the marionette driver is, I can go back to it and spend more time if nothing jumps out as you.

But overall, looking for some guidance on how to handle each of those...

Comment 3

[Dave Townsend [:mossop]]

(In reply to Andrew Swan [:aswan] from comment #2)
> Comment on attachment 8723837 [details]
> MozReview Request: Bug 1251412 - use installTemporaryAddon for
> jetpack-addons tests
> 
> This patch implements the basic mechanics but it causes several new failures
> and a noisy warning.
> The failures are:
> https://dxr.mozilla.org/mozilla-central/rev/
> d848a5628d801a460a7244cbcdea22d328d8b310/addon-sdk/source/test/addons/main/
> main.js#17
> With the temporary install, the loadReason is "enable" instead of "install"
> as the test expects.

Yeah this sounds like a bug we should fix. Can you file a follow-up? For now just comment out that check so we can get this landed.

> https://dxr.mozilla.org/mozilla-central/rev/
> d848a5628d801a460a7244cbcdea22d328d8b310/addon-sdk/source/test/addons/
> unpacked/main.js#10
> With the temporary install, packed is true, instead of false as the test
> expects
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> d848a5628d801a460a7244cbcdea22d328d8b310/addon-sdk/source/test/addons/
> unpacked/main.js#14
> With the temporary install, toFilename now throws this error:
> Error: cannot map to filename:
> resource://test-url-at-jetpack/unpacked/lib/main.js

This test verifies that the add-on is unpacked on install and we don't do that for temporary add-ons. In fact we should probably do a follow-up bug that refuses to load an add-on temporarily if it is marked to be unpacked. Can you file that too please? Let's just disable this test, It is really verifying cfx's behaviour here not the SDK and we don't support cfx any more so I don't think it is worth trying to get this test working.

Comment 4

[Dave Townsend [:mossop]]

(In reply to Andrew Swan [:aswan] from comment #2)
> In addition to those, we also get these warnings every time an addon is
> installed:
> 1456444058018	addons.manager	WARN	AddonListener threw exception when calling
> onInstalled: TypeError: that.sandboxes[sandboxName] is undefined
> (chrome://marionette/content/driver.js:1090:1) JS Stack trace:
> GeckoDriver.prototype.executeWithCallback/res</chromeAsyncReturnFunc@driver.
> js:1090:1 < __marionetteFunc/[e-mail]:13:19 <
> [e-mail]:1723:11 <
> [e-mail]:2832:5 <
> this.XPIProvider.installTemporaryAddon<@XPIProvider.jsm:3879:5

Andrew, do you know anything about this?

Comment 5

[Andrew Halberstadt [:ahal]]

Ah, I think we need to insert an "AddonManager.removeListener(listener);" call here:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/addons.py#68

Want me to file a bug and tackle it separately? Or want to just roll it into this patch?

Comment 6

[Andrew Swan [:aswan]]

Andrew H, since the Marionette fix is really distinct from the changes for this bug and this bug isn't really blocked on the the Marionette fix, I'd vote for a separate bug and patch.

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8723837 [details]
MozReview Request: Bug 1251412 - use installTemporaryAddon for jetpack-addons tests

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

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8723837 [details]
MozReview Request: Bug 1251412 - use installTemporaryAddon for jetpack-addons tests

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

Comment 9

[Andrew Swan [:aswan]]

Alright, with the latest patch, "mach mochitest -f jetpack-addon addon-sdk/source/test" passes both with and without signing enabled.
Follow-up bugs are
Bug 1251664 - With temporary add-on installation, options.loadReason is "enable" instead of "install" 
Bug 1251673 - Don't allow temporary installation of add-ons that require unpacking

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8723837 [details]
MozReview Request: Bug 1251412 - use installTemporaryAddon for jetpack-addons tests

https://reviewboard.mozilla.org/r/36763/#review33499

Nice, thanks

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/304c2876b1a2

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/304c2876b1a2

Comment 13

[Dave Townsend [:mossop]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9bd0b3869ad8