873567 - Enable the installPackage mochitests

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P2)

Description

[Jason Smith [:jsmith]]

Enable the mochitests that were implemented in bug 821589 for the installPackage API on FxAndroid.

Comment 1

[Jason Smith [:jsmith]]

James - Can you look into this? We really need to get these tests running on FxAndroid so that we avoid regressions like we saw on bug 873134.

Not sure if we want to track this as part of the A4A initiative or not (given it's not production code changes), but we really need to do this to avoid regressions.

Comment 2

[Wesley Johnston (:wesj)]

Should be as simple as yanking this ifdef out and pushing this to try:

http://mxr.mozilla.org/mozilla-central/source/dom/apps/tests/Makefile.in#23

Comment 3

[Jason Smith [:jsmith]]

Saw the WebRT notes on this. Sounds like James is going to look into this.

Comment 4

[James Hugman [:jhugman] [@jhugman]]

Pushed the patch enabling the tests to try. The results are here: https://tbpl.mozilla.org/?tree=Try&rev=8a03b786760d

I'm now looking for some feedback and help interpreting the results.

Comment 5

[[:fabrice] Fabrice Desré]

It seems that we fail at https://mxr.mozilla.org/mozilla-central/source/dom/apps/tests/test_packaged_app_install.html?force=1#325, likely because neither ondownloaderror nor ondownloadsuccess are reached in https://mxr.mozilla.org/mozilla-central/source/dom/apps/tests/test_packaged_app_install.html?force=1#126

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

From the log:

--------- beginning of /dev/log/main
05-28 05:31:45.788 E/GeckoConsole( 1804): [JavaScript Error: "NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIWebProgress.DOMWindow]" {file: "chrome://browser/content/browser.js" line: 3424}]
05-28 05:31:45.788 E/GeckoConsole( 1804): [JavaScript Error: "NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIWebProgress.DOMWindow]" {file: "chrome://browser/content/browser.js" line: 3424}]
05-28 05:32:00.815 E/GeckoConsole( 1804): [JavaScript Warning: "Empty string passed to getElementById()." {file: "http://mochi.test:8888/tests/dom/imptests/webapps/DOMCore/tests/submissions/Ms2ger/test_Document-getElementById.html" line: 12}]
05-28 05:32:11.725 E/GeckoConsole( 1804): [JavaScript Error: "NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIWebProgress.DOMWindow]" {file: "chrome://browser/content/browser.js" line: 3424}]
05-28 05:32:11.725 E/GeckoConsole( 1804): [JavaScript Error: "NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIWebProgress.DOMWindow]" {file: "chrome://browser/content/browser.js" line: 3424}]
05-28 05:32:12.905 E/GeckoConsole( 1804): [JavaScript Warning: "Use of Mutation Events is deprecated. Use MutationObserver instead." {file: "http://mochi.test:8888/tests/dom/imptests/webapps/DOMCore/tests/submissions/Ms2ger/test_Node-replaceChild.html" line: 314}]
05-28 05:32:14.378 E/GeckoConsole( 1804): [JavaScript Warning: "Duplicate resource declaration for 'gre-resources' ignored." {file: "jar:jar:file:///data/app/org.mozilla.fennec-1.apk!/omni.ja!/chrome/chrome.manifest" line: 47}]
05-28 05:32:14.385 E/GeckoConsole( 1804): Could not read chrome manifest 'file:///data/data/org.mozilla.fennec/chrome.manifest'.
05-28 05:32:14.385 E/GeckoConsole( 1804): [JavaScript Warning: "Duplicate resource declaration for 'specialpowers' ignored." {file: "file:///mnt/sdcard/tests/profile/extensions/[e-mail]/chrome.manifest" line: 2}]
05-28 05:32:14.385 E/GeckoConsole( 1804): [JavaScript Warning: "Duplicate resource declaration for 'gre-resources' ignored." {file: "jar:jar:file:///data/app/org.mozilla.fennec-1.apk!/omni.ja!/chrome/chrome.manifest" line: 47}]
05-28 05:32:14.385 E/GeckoConsole( 1804): Could not read chrome manifest 'file:///data/data/org.mozilla.fennec/chrome.manifest'.
05-28 05:32:14.395 E/GeckoConsole( 1804): [JavaScript Warning: "Duplicate resource declaration for 'specialpowers' ignored." {file: "file:///mnt/sdcard/tests/profile/extensions/[e-mail]/chrome.manifest" line: 2}]
05-28 05:32:17.365 E/GeckoConsole( 1804): [JavaScript Error: "content is null" {file: "chrome://global/content/BrowserElementChildPreload.js" line: 268}]
05-28 05:32:17.768 E/JavaBinder( 1804): Unknown binder error code. 0xfffffff7

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to James Hugman [:jhugman] [@jhugman] from comment #4)
> Pushed the patch enabling the tests to try. The results are here:
> https://tbpl.mozilla.org/?tree=Try&rev=8a03b786760d
> 
> I'm now looking for some feedback and help interpreting the results.

The failure in the log is:

19917 INFO TEST-PASS | /tests/dom/apps/tests/test_packaged_app_install.html | == TEST == Mini-manifest app name is different from webapp manifest name
19918 INFO TEST-PASS | /tests/dom/apps/tests/test_packaged_app_install.html | App installed
19919 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_packaged_app_install.html | Test timed out.

The test in question confirms that an app name mismatch between the manifest and the minifest causes the install to fail:

http://hg.mozilla.org/mozilla-central/file/6a739f41d1ba/dom/apps/tests/test_packaged_app_install.html#l317

It does that by calling checkAppDownloadError, which tries to install the app and confirms that the resulting error is the expected one. And it looks like the navigator.mozApps.installPackage.onsuccess callback is correctly called (resulting in the "app installed" message in the log).

But navigator.mozApps.mgmt.oninstall is also supposed to be called, so it can define evt.application.ondownloaderror; after which that callback is supposed to be called. And one of those two callbacks isn't being called, so the test times out waiting for it.

Comment 8

[James Hugman [:jhugman] [@jhugman]]

Thanks all.

This should be nom'd for fixing as part of a4a.

Comment 10

[Marco Castelluccio [:marco]]

Attachment: fixPackagedAppsTest

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=6d68e1c65bad

Comment 11

[Marco Castelluccio [:marco]]

Sorry to steal the bug, but the Desktop fix fixed also Android.

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 784101 [details] [diff] [review]
fixPackagedAppsTest

Review of attachment 784101 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/apps/src/Webapps.jsm
@@ +1928,5 @@
> +      this.startOfflineCacheDownload(cacheDownload.manifest,
> +                                     cacheDownload.app,
> +                                     cacheDownload.profileDir,
> +                                     cacheDownload.offlineCacheObserver);
> +      delete this.queuedDownload[aManifestURL];

you can early return here.

Comment 13

[Marco Castelluccio [:marco]]

Attachment: fixPackagedAppsTest

Carrying r+

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 784513 [details] [diff] [review]
fixPackagedAppsTest

Setting review flag so it's clear that the latest patch had review carried forward and is ready to check in.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 784513 [details] [diff] [review]
fixPackagedAppsTest

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9429bd59114

Comment 16

[Ryan VanderMeulen [:RyanVM]]

The fix-up broke the tests. I'm assuming that wasn't the outcome you were shooting for. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b82c0f866f64

https://tbpl.mozilla.org/php/getParsedLog.php?id=26047618&tree=Mozilla-Inbound

Comment 17

[Marco Castelluccio [:marco]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> The fix-up broke the tests. I'm assuming that wasn't the outcome you were
> shooting for. Backed out.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b82c0f866f64

Weird, they were green on try.

Comment 18

[Jason Smith [:jsmith]]

(In reply to Marco Castelluccio [:marco] from comment #17)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> > The fix-up broke the tests. I'm assuming that wasn't the outcome you were
> > shooting for. Backed out.
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/b82c0f866f64
> 
> Weird, they were green on try.

Note - the try run you did had a bunch of extra patches including this patch. What likely happened in this situation is that the try run with this patch alone intermittently fails as a stand-alone, as it assumed certain other patches were landed before this patch landed.

Comment 19

[Marco Castelluccio [:marco]]

(In reply to Jason Smith [:jsmith] from comment #18)
> Note - the try run you did had a bunch of extra patches including this
> patch. What likely happened in this situation is that the try run with this
> patch alone intermittently fails as a stand-alone, as it assumed certain
> other patches were landed before this patch landed.

Most of them are already landed and some don't change anything important. But I've also triggered builds with the same set of patches and without this one, and the tests were failing.

Comment 20

[Marco Castelluccio [:marco]]

I think the problem here is the same that occured for bug 777402. Anyway it's better to wait tomorrow.

Comment 21

[Marco Castelluccio [:marco]]

Attachment: Queue appcache download only if the appcache manifest attribute is specified

The early return uncovered another small problem

Comment 22

[Marco Castelluccio [:marco]]

Try run is here: https://tbpl.mozilla.org/?tree=Try&rev=2c25069e60a7

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/eee1097940ea

Comment 24

[Jason Smith [:jsmith]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #23)
> https://hg.mozilla.org/integration/b2g-inbound/rev/eee1097940ea

This doesn't appear to be landed correctly. There's two patches here, not one.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

You'll have to forgive me for missing that when the other one has checkin+ set on it...

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 784513 [details] [diff] [review]
fixPackagedAppsTest

And this would be a lesson in why setting checkin+ on a single-patch bug can be a bad thing.

https://hg.mozilla.org/integration/b2g-inbound/rev/ce95e81ebee8

Comment 27

[Myk Melez [:myk] [@mykmelez]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #26)
> And this would be a lesson in why setting checkin+ on a single-patch bug can
> be a bad thing.
> 
> https://hg.mozilla.org/integration/b2g-inbound/rev/ce95e81ebee8

And I see that "please do the following after pushing to inbound" <https://wiki.mozilla.org/Tree_Rules/Inbound#Please_do_the_following_after_pushing_to_inbound> no longer recommends it.  Duly noted!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eee1097940ea
https://hg.mozilla.org/mozilla-central/rev/ce95e81ebee8