Closed Bug 873567 Opened 11 years ago Closed 11 years ago

Enable the installPackage mochitests

Categories

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

ARM
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: jsmith, Assigned: marco)

References

Details

(Whiteboard: [A4A][apps-automation:P3])

Attachments

(2 files, 1 obsolete file)

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.
Flags: needinfo?(jhugman)
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.
Flags: needinfo?(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.
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.
Whiteboard: [A4A]
Priority: -- → P2
Whiteboard: [A4A] → [A4A][apps-automation:P3]
Assignee: nobody → myk
Status: NEW → ASSIGNED
Attached patch fixPackagedAppsTest (obsolete) — Splinter Review
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=6d68e1c65bad
Attachment #784101 - Flags: review?(fabrice)
Sorry to steal the bug, but the Desktop fix fixed also Android.
Summary: Enable the installPackage mochitests from bug 821589 on FxAndroid → Enable the installPackage mochitests
Depends on: 777402
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.
Attachment #784101 - Flags: review?(fabrice) → review+
Carrying r+
Attachment #784101 - Attachment is obsolete: true
Keywords: checkin-needed
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.
Attachment #784513 - Flags: review+
Assignee: myk → mcastelluccio
Keywords: checkin-needed
(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
Attachment #785211 - Flags: review?(fabrice)
Attachment #785211 - Flags: review?(fabrice) → review+
(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.
Flags: needinfo?(ryanvm)
You'll have to forgive me for missing that when the other one has checkin+ set on it...
Flags: needinfo?(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
Attachment #784513 - Flags: checkin+
(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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Blocks: 903291
No longer blocks: 903291
Depends on: 903291
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.