Closed Bug 826058 Opened 12 years ago Closed 11 years ago

Write tests for app install/update

Categories

(Core Graveyard :: DOM: Apps, defect)

Product:
Core Graveyard
Component:
DOM: Apps
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla23

People

(Reporter: bholley, Assigned: fabrice)

References

Details

Attachments

(2 files, 6 obsolete files)

Hosted app install/update tests. WIP
16.17 KB, patch
Details | Diff | Splinter Review
Hosted app install/update tests
20.62 KB, patch
Details | Diff | Splinter Review 
sicking wants this to catch bugs like bug 823189 and bug 824697.

We want to test this for appcach-ed hosted apps as well as packaged apps, probably separately. sicking wants the following steps:

1. Install an app. Check that the correct events fire and that all the state on the app object is correct
2. Ask the app to check for update. Check that the correct events fire and that all the state on the app object is correct
3. Update the app serverside (by modifying state in the server using sjs)
4. repeat 2
5. Tell the API to download the update. Check events/state during download and at end of download
6. Apply (i.e. install) the download
7. repeat 2

We'll also want to run the app after installation to make sure it works properly.
Oh, another thing we need tests for is error handling. I.e. test that the right thing happens if the update ping returns an invalid response, or if the app download is aborted half-way through. We can definitely do this in followups though if that's easier.
These are the tests as they exist thus far. I don't have any time left to work on
these unfortunately, so someone else will have to pick them up. There's a lot of
useful machinery in here, so they're worth picking up IMO.

Currently, there are a number of issues:
* I tried getting these running on Desktop b2g (via bug 826111), but encountered
  breakage in BrowserElementPromptService that prevented the app from
  communicating with the parent.
* Uninstalling is broken. See bug 829726.
* When uninstall workings, the comments around a large section of the test can be
  removed. When they are, you'll hit a failure that is bug 827908.
* The tests stall when checking for updates. I've filed bug 829763.
Assignee: bobbyholley+bmo → dale
Thanks Dale for helping out here. Much appreciated.
These are now passing in b2g desktop, the "dom.mozBrowserFramesEnabled" pref needed enabled there were a few small changes, there are failures in firefox builds

> 12 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_app_update.html | App is not installed
> 17 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_app_update.html | App is not installed
> 23 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_app_update.html | App is not installed
> 42 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_app_update.html | App is not installed
> 48 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_app_update.html | lastUpdateCheck updated appropriately

Checking that now.

https://bugzilla.mozilla.org/show_bug.cgi?id=826146 also needs uplifted with a rebase, will send along a patch
Attached patch Hosted app install/update tests. (obsolete) — — Splinter Review 
There is 1 TODO which is a genuine bug I am filing, the rest pass

This will only work tested against b2g desktop
Attachment #703787 - Attachment is obsolete: true
Attachment #705203 - Flags: review?(fabrice)
Attachment #705203 - Attachment is patch: true
Also should note the bug I linked previously has been uplifted, this is the only patch needed
It was mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=827908 that we could have these working on desktop firefox builds using the same workaround as 

http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/head.js#4

I am confused by the original intent of the isLaunchable code, but it seems worth doing for these tests
(In reply to Dale Harvey (:daleharvey) from comment #9)
> It was mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=827908 that
> we could have these working on desktop firefox builds using the same
> workaround as 
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/
> head.js#4
> 
> I am confused by the original intent of the isLaunchable code, but it seems
> worth doing for these tests

isLaunchable I believe was originally there because webapps on desktop do a different set of checks to determine if an app is launchable - it checks that the app is "natively" installed on the OS, where as other platforms (Android, B2G), we follow the typical rule of checking the registry.

And yes, it would be worth doing the same for these tests. These tests aren't intending to test the native install logic on desktop, so feel free to do the same here with these tests.
Comment on attachment 705203 [details] [diff] [review]
Hosted app install/update tests.

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

great!
Attachment #705203 - Flags: review?(fabrice) → review+
FWIW, I didn't really consider these tests finished. I think a it's probably worth spending a little bit of time expanding them to check more cases, now that the infrastructure's all there.
Attached patch Hosted app install/update tests (obsolete) — — Splinter Review 
Added SpecialPowers API to make all apps launchable, now passes in Firefox
Attachment #705203 - Attachment is obsolete: true
Yeh there is definitely more testing needed, I just wasnt sure of the best workflow, get a set of tests working, commit and improve or do them all in one go.

I mentioned to fabrice earlier, happy to write more tests but I would prefer to be pointed to which cases / bugs to test for.
I'd actually try to get something landed such that the bare bones of framework is in the codebase, then do more tests as followups.

Btw - if you looking for something else to go after, I'd recommend tackling bug 821589. We currently don't have any gecko automation for packaged apps. So if you can get the bare bones of that framework up, that would be a huge win.
Comment on attachment 707511 [details] [diff] [review]
Hosted app install/update tests

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

::: testing/specialpowers/content/specialpowersAPI.js
@@ +673,5 @@
>      this.pushPrefEnv({set: [['dom.mozApps.auto_confirm_install', true]]}, cb);
>    },
>  
> +
> +  originalAppsLaunableValue: null,

typo: "originalAppsLaunchableValue"

@@ +684,5 @@
> +
> +  restoreAllAppsLaunchable: function() {
> +    var Webapps = {};
> +    Components.utils.import("resource://gre/modules/Webapps.jsm", Webapps);
> +    Webapps.DOMApplicationRegistry.allAppsLaunchable = this.originalAppsLaunchableVal;

Should this be: "this.originalAppsLaunchableValue"?
Can we address the small nits in comment 16 and get this landed?
Yup sorry, was changes I forgot to commit, will update the patch, test again and get it landed tomorrow
Attached patch Hosted app install/update tests (obsolete) — — Splinter Review 
Updated with SpecialPowers API to skip per platform webapp checks
Attachment #707511 - Attachment is obsolete: true
Depends on: 837572
Attached patch Hosted app install/update tests. (obsolete) — — Splinter Review 
Bobby

Planning to land this and follow up with more tests, this wont work until https://bugzilla.mozilla.org/show_bug.cgi?id=837572 lands. but its passing on try aside from that. 

I wanted to check in particular that the way I added the specialpowers api here is the right way, not that familiar with that code.
Attachment #709091 - Attachment is obsolete: true
Attachment #709666 - Flags: review?(bobbyholley+bmo)
Attachment #709666 - Attachment is patch: true
Comment on attachment 709666 [details] [diff] [review]
Hosted app install/update tests.

You should probably flag ted or someone for the SpecialPowers stuff, I'm not a peer there.

I can't really review the tests per se given that I wrote them. If you want me to look at any changes, can you attach an interdiff?
Attachment #709666 - Flags: review?(bobbyholley+bmo)
Its just the special powers stuff that needs looked at, fabrice already r+'d the tests themselves, the actual tests themselves didnt change, just some preferences and specialpowers things. Cheers
Comment on attachment 709666 [details] [diff] [review]
Hosted app install/update tests.

The tests themselves are r+'d, wanted to ensure the new special powers api was done correctly.
Attachment #709666 - Flags: review?(ted)
Ted, can you take a look at the special powers part? We really badly need to land this. Thanks!
Preemtively pushed to try and it looks like linux isnt downloading the appcache, investigating

https://tbpl.mozilla.org/?tree=Try&rev=fd19f73af1e5
Clint - Can we get help here? We've got no automation in the tree for the apps install/update API and really need a review here to get this landed, so that we have some basic tests in the tree.
Depends on: 839810
Comment on attachment 709666 [details] [diff] [review]
Hosted app install/update tests.

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

r- for the 127.0.0.1 bit.

::: dom/apps/tests/file_app.template.html
@@ +49,5 @@
> +  installed(!!app);
> +  if (app) {
> +    var appName = "Really Rapid Release (APPTYPETOKEN)";
> +    is(app.manifest.name, appName, "Manifest name should be correct");
> +    is(app.origin, "http://127.0.0.1:8888", "App origin should be correct");

If you're going to run these on non-desktop machines, we don't actually serve web content on 127.0.0.1.

::: dom/apps/tests/test_app_update.html
@@ +13,5 @@
> +  /** Test for Bug 826058 **/
> +
> +  SimpleTest.waitForExplicitFinish();
> +
> +  var gBaseURL = 'http://127.0.0.1:8888/tests/dom/apps/tests/';

You can't use 127.0.0.1 in Mochitests, that's not where we serve them from on Android/B2G.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +734,5 @@
> +  restoreAllAppsLaunchable: function() {
> +    var Webapps = {};
> +    Components.utils.import("resource://gre/modules/Webapps.jsm", Webapps);
> +    Webapps.DOMApplicationRegistry.allAppsLaunchable = this.originalAppsLaunchableVal;
> +  },

I don't really know how these APIs work, so I can't comment very deeply on them. Just a few things to think about:
a) Will these work if called from a content process?
b) If so, will the value be set synchronously, or asynchronously in the content process (prefs are set asynchronously).
Attachment #709666 - Flags: review?(ted) → review-
Will fix the host (curious that these tests pass on b2g with the previous try run though)

As far as I can tell preferences are set synchronously via |return(this._sendSyncMessage('SPPrefService', msg)[0])| but this api is unlikely to work when run oop, I will fix that to mirror the preferences api
(In reply to Dale Harvey (:daleharvey) from comment #28)
> As far as I can tell preferences are set synchronously via
> |return(this._sendSyncMessage('SPPrefService', msg)[0])| but this api is
> unlikely to work when run oop, I will fix that to mirror the preferences api

This works OOP if the code that runs immediately after does not depend on the value of the pref being correct.
What do you mean by 'immediately'? I havent seen any examples of tests that wait for preferences to be set and most of them will fail if there preferences arent

This shouldnt effect this test anyway, if we set allAppsLAunchable inside a syncMessage then it should be safe right?
The situation is that the message to set the preference is synchronous, so the code in the child won't continue running until the pref has been set in the parent process. However, the child doesn't see the updated value until the parent sends an update message, so code that immediately runs in the child before returning to the event loop will only see the pre-update value. Code that runs in the parent will see the updated value.
The correct thing to do in any case is use SpecialPowers.pushPrefEnv, which delays running the callback until the child sees the pref update: http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js?force=1#560
Attached patch Hosted app install/update tests. (obsolete) — — Splinter Review 
I am slightly confused by the reasoning to switch from 127.0.0.1, the app is specifically testing being installed from a host that differs from where it is served from and 127.0.0.1 is in 

http://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt

But switched it to another anyway.

The SpecialPrivileges API call will now work when called from oop content processes, oop cant yet be enabled for these tests. Will file a follow up bug for that.
Attachment #709666 - Attachment is obsolete: true
Attachment #714161 - Flags: review?
Attachment #714161 - Flags: review? → review?(ted)
Try push passing on all platforms, there are a bunch of failures there but all previously reported intermittents
Ted: Any chance of getting a review on this fairly quickly? It'd be very nice to not have to worry about regressions in the app update code since that is very critical that it's working. Likewise, this is holding up other testing work happening in other bugs.
I just pinged Ted over email to see if we can get a review as soon as possible.
Comment on attachment 714161 [details] [diff] [review]
Hosted app install/update tests.

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

Thanks for those fixes. r=me
Attachment #714161 - Flags: review?(ted) → review+
(In reply to Dale Harvey (:daleharvey) from comment #35)
> Try push passing on all platforms, there are a bunch of failures there but
> all previously reported intermittents

That try push actually failed on b2g, did you realize that? It failed saying that "app is not installed"  But if this worked for you locally then it may have been an orange.
rebased
Attachment #714161 - Attachment is obsolete: true
I hadnt, cheers for catching that Clint, will to another push to try to make sure, I cant reproduce locally
Update on this

The b2g failure clint noticed is a genuine bug, it seems like the special powers API to enabe the webapps-manage permission is not working correctly as the emulator hangs on assertPermission when trying to uninstall an application.

It doesnt seem to be a race condition (I just added a large timeout after adding the permissions), but its strange that this works on b2g / desktop builds and not emulator ones, it may be an oop problem, now that I have reproduced it I should get to the cause in the morning
Erg. Bug 685652 is required for the same reasons that pushPrefEnv was introduced - the synchronous message the sets the permission is not quite synchronous enough, since the actual permission update passes over IPDL from parent to child separately, so the child doesn't see the updated permission value until some time later in the future.
And yeah, this works on desktop because we disable OOP.
So is this bug basically blocked on bug 685652's completion then? If so, I can raise this to Clint about needing to get that bug resolved.
Sounds like it.
Depends on: 685652
I am not so sure, as said I put a 5 second timeout after calling special powers before starting the tests and it didnt help, so I think the permission is permanently not being set, looking into it now
So I got lost looking into this, I am still sure its something fairly basic, the permission looks to be set correctly but not read (possibly off the wrong process)

I spent a long time and got lost in the permissions code, right now I dont even have a machine I can reproduce this on so I will have to unassign myself
Assignee: dale → nobody
Blocks: 833657
Assignee: nobody → fabrice
Do we still need bug 685652 to fix this bug?
I am not convinced we do, at the least it needs verified by fabrice beforehand
I still can't reproduce the failure locally (I should get access to a build slave soon), but here's where we fail in try builds:

- when calling mozApps.mgmt.uninstall(), the child process recognize that the process has the webapps-manage permission, so sends an uninstall message to the parent.
- the parent, at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#791 checks again if the child has the webapps-manage permission.
- we end up at https://mxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#35 where we look for ASSERT_APP_PROCESS_PERMISSION. The tab->IsBrowserElement() returns true, so we don't go into the switch block and return false and get killed.

Jonas && Jusin, is there any reason for not adding aType == ASSERT_APP_PROCESS_PERMISSION at https://mxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#38 ?
Flags: needinfo?(jonas)
The way we do security checks are pretty broken :(. Definitely something that we need to apply a bigger hammer to and fix up. But let's not do that here.

I don't quite understand what you are proposing, but using anything related to ASSERT_APP_HAS_PERMISSION would be the wrong fix.

The problem is that by the time we get into this function, we have lost all sense of what the calling "origin" was. And without that we will have a very hard time using the nsIPermissionManager :(
Flags: needinfo?(jonas)
And now that the emulator has been updated on tbpl, we are green on try:
https://tbpl.mozilla.org/?tree=Try&rev=1a1a59966b17
The test failing was M2
So does this mean we actually don't need bug 685652 to fix this bug then?
(In reply to Jason Smith [:jsmith] from comment #55)
> So does this mean we actually don't need bug 685652 to fix this bug then?

That's likely, yes. I'm gonna do a full try run with the "ready to land" patch instead of my debug version to make sure that we are good to land.
So... The failures in this patch were due to the outdated emulator used by the test slaves (hence I could not repro locally). With that fixed, we still had a couple of other test failures because they were interfering with the new SpecialPowers.setAllAppsLaunchable() api from this patch.

I fixed the 2 faulty tests and got a green try build:
https://tbpl.mozilla.org/?tree=Try&rev=e095766410ceui

If no one objects, I will push this patch tomorrow.
https://hg.mozilla.org/mozilla-central/rev/e63cb4c3e063
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 858772
Depends on: 859337
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: