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

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.

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

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

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.

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

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/special-powers@mozilla.org/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/special-powers@mozilla.org/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

(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.

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

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

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

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.

Carrying r+

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 on attachment 784513 [details] [diff] [review] fixPackagedAppsTest https://hg.mozilla.org/integration/mozilla-inbound/rev/e9429bd59114

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

(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.

(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.

(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.

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

The early return uncovered another small problem

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

(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.

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

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

(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!

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