Closed Bug 1150009 Opened 6 years ago Closed 6 years ago

Add cleanup functions to the gaia UI tests that install webapps

Categories

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

Details

Attachments

(2 files)

There are some Gaia UI tests, that install webapps. They need to have a cleanup method that uninstalls the app afterwards (when it's still there):
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/tests/functional/marketplace/test_marketplace_packaged_app.py
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/tests/functional/homescreen/test_homescreen_launch_app_packaged.py
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/tests/functional/homescreen/test_homescreen_delete_app_packaged.py
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/tests/functional/homescreen/test_homescreen_delete_app.py
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/tests/functional/homescreen/test_homescreen_launch_app.py


When that is done, the "self.device.file_manager.remove('/data/local/webapps')" line at: https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L819
..can be removed, because:
14:27 jlorenzo: mwargers: because in the build I have (supposedly an engineering one), all the apps are in this folder
14:28 jlorenzo: mwargers: so basically, at the first run, Marionette deletes everything and you're not able to run anything
14:30 mwargers: jlorenzo: I remember it vaguely, I had some issue where webapps stayed in between tests or something
14:30 mwargers: oh, yes, bug 1080487
14:30 firebot: https://bugzil.la/1080487 — FIXED, nobody@mozilla.org — Revert bug 990214, to re-introduce deletion of webapps during cleanup
14:30 mwargers: oh, yes, when running the install_packaged_app tests, for example
14:31 mwargers: jlorenzo: hmm, why are the apps in that folder?
14:32 jlorenzo: mwargers: I asked a couple of guys in the Paris office, it supposed to be the default folder for eng build. But I don't know why we don't have this issue on the eng build from pvt builds
14:56 jlorenzo: mwargers: mwargers: our eng build from pvt are not pure eng build. This is the config file we use https://dxr.mozilla.org/build:mozharness/source/configs/b2g/releng-otoro-eng.py
15:00 jlorenzo: mwargers: it says it's an engineering, but they set "B2G_SYSTEM_APPS": "1" which changes the location of the webapps to the same as user builds

So a pure engineering build currently barfs while running the gaia UI tests.
B2G_SYSTEM_APPS moves apps from /data to /system, as I understand it. I think it also prevents the app code from being combined into the *defer*js files that concatenate all the app JS together.

I'm sure what it amounts to is A) making the apps easier to debug, and probably B) more changeable on-device at runtime.

So as mentioned, we need to have better handling of whether things are deleted from the device out of /data. 

I think the origins for the apps (*.gaiamobile.org) might be sufficient to recognize a default app from a deployed one. Differentiating on origin would be the first thing I'd look at.

Alternately, very first setup could scan for installed apps, either via filesystem or app manager, and drop a dynamic whitelist on the device that we use during cleanup to decide what to delete.
19:16 geo: is origin a non-starter?
19:16 geo: or are the apps we install during tests also gaiamobile.org apps?
19:16 geo: there’s facility in GaiaApps to pull back all the app origins installed afaik
19:17 mwargers: geo, yeah, that would be a solution too, check for origin or so
19:17 geo: so if there’s a difference, we pull those back, filter on that origin, delete the others
19:17 geo: if that facility isn’t there I can add it
19:17 geo: it’s on the JS side, not sure what we did or didn’t expose on Python
19:18 geo: but I think there should be a get_installed_apps() or similar that gives back a list of dicts, including origins
19:19 geo: apps_to_delete = [app for app in installed_apps where not app[‘origin’].endswith(‘gaiamobile.org’)] or similar
19:20 geo: er, s/where/if
See Also: → 1150537
Depends on: 1154784
Comment on attachment 8593461 [details] [review]
[gaia] mwargers:teardown > mozilla-b2g:master

So something like this? This will make sure that only the apps installed by the tests will get removed.
Attachment #8593461 - Flags: review?(dave.hunt)
Comment on attachment 8593461 [details] [review]
[gaia] mwargers:teardown > mozilla-b2g:master

See pull request for comments.
Attachment #8593461 - Flags: review?(dave.hunt) → review-
Comment on attachment 8593461 [details] [review]
[gaia] mwargers:teardown > mozilla-b2g:master

From Dave Hunt on github:
"
I would prefer to explicitly uninstall any apps in the individual tests tear down, but I understand that if there's a failure in the set up or earlier in the tear down then the apps will not be uninstalled.
"

I've done that now with this pull request.
I tested it on b2g desktop and on device locally.

I guess we still want the clean up of the none bundled apps (if they were not cleaned up by the test themselves) in /data/local/webapps, right?
Attachment #8593461 - Flags: review- → review?(dave.hunt)
If we want to clean up the none bundled apps on tear down (outside of the tests), then it's good to know what currently the apps are in the /data/local/webapps/ directory:
- "{14750155-5f3d-41ab-a527-f16adf4c3293}", "name: "Mochitest", "origin": "http://mochi.test:8888"
- "marketplace.firefox.com", "name": "Marketplace", "origin": "app://marketplace.firefox.com"
- "marketplace-dev.allizom.org", "name": "Dev" ,"origin": "app://marketplace-dev.allizom.org"
- "marketplace.allizom.org", "name": "Stage", "origin": "app://marketplace.allizom.org"

It's all mentioned in the /data/local/webapps/webapps.json file.
The bundled apps all have "basePath": "/system/b2g/webapps" (so you don't see them in the /data/local/webapps/ directory anyway) and with origin *.gaiamobile.org, it seems.
Comment on attachment 8600443 [details] [review]
[gaia] mwargers:teardown2 > mozilla-b2g:master

Something like this perhaps?
Attachment #8600443 - Flags: review?(dave.hunt)
Comment on attachment 8593461 [details] [review]
[gaia] mwargers:teardown > mozilla-b2g:master

Looks good, but let's reduce the duplication by adding an uninstall method to GaiaApps.
Attachment #8593461 - Flags: review?(dave.hunt) → review-
Comment on attachment 8600443 [details] [review]
[gaia] mwargers:teardown2 > mozilla-b2g:master

See comments in pull request.
Attachment #8600443 - Flags: review?(dave.hunt) → review-
Comment on attachment 8593461 [details] [review]
[gaia] mwargers:teardown > mozilla-b2g:master

Updated pull request with your suggestion.
Attachment #8593461 - Flags: review- → review?(dave.hunt)
Comment on attachment 8593461 [details] [review]
[gaia] mwargers:teardown > mozilla-b2g:master

Nice, thanks!
Attachment #8593461 - Flags: review?(dave.hunt) → review+
Attachment #8593461 - Flags: review?(jlorenzo)
Comment on attachment 8593461 [details] [review]
[gaia] mwargers:teardown > mozilla-b2g:master

r+ with one remaining question in the PR.
Attachment #8593461 - Flags: review?(jlorenzo) → review+
Comment on attachment 8600443 [details] [review]
[gaia] mwargers:teardown2 > mozilla-b2g:master

Something like this? I'm using the system app for the installTime check.
Not sure how I can prevent the nested for..in loops.

I want to make sure I also delete the directories that are not mentioned in the webapps.json file (in case webapps.json file is not updated correctly with the latest installed webapps, I don't know if it is real time updated).
Attachment #8600443 - Flags: review- → review?(dave.hunt)
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #14)
> Comment on attachment 8593461 [details] [review]
> [gaia] mwargers:teardown > mozilla-b2g:master
> 
> r+ with one remaining question in the PR.

I followed this suggestion and also did git fetch and git rebase origin/master for this branch, because Marionette was upated.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #16)
> (In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #14)
> > Comment on attachment 8593461 [details] [review]
> > [gaia] mwargers:teardown > mozilla-b2g:master
> > 
> > r+ with one remaining question in the PR.

There was also a small JSLint issue that I had to fix (some missing semi-colons), after fixing that, Treeherder Try was green, so merged it now: https://github.com/mozilla-b2g/gaia/commit/b6ef0ba70185d6a687485672862742fc9e3b482c
Comment on attachment 8600443 [details] [review]
[gaia] mwargers:teardown2 > mozilla-b2g:master

I think this is pretty close. I added a suggestion in the pull request.
Attachment #8600443 - Flags: review?(dave.hunt) → review-
Comment on attachment 8600443 [details] [review]
[gaia] mwargers:teardown2 > mozilla-b2g:master

This is now actually more your patch, than mine.

I tested this by running test_marketplace_packaged_app.py and then test_homescreen_launch_app (twice) and the marketplace app and homescreen installed app both would be deleted correctly when the next test would start up.
Attachment #8600443 - Flags: review- → review?(dave.hunt)
Attachment #8600443 - Flags: review?(dave.hunt) → review+
Comment on attachment 8600443 [details] [review]
[gaia] mwargers:teardown2 > mozilla-b2g:master

With this pull request checked in, people that would build their own builds and upload them to the Flame device would get to see apps on the homescreen, instead of having all the apps wiped, as Johan was telling me.
Attachment #8600443 - Flags: review?(jlorenzo)
Attachment #8600443 - Flags: review?(jlorenzo) → review+
(In reply to Martijn Wargers [:mwargers] (QA) from comment #20)
> Comment on attachment 8600443 [details] [review]
> [gaia] mwargers:teardown2 > mozilla-b2g:master

Merged: https://github.com/mozilla-b2g/gaia/commit/252c00ce153bf6d635ca56cde63fc5555090b3dd
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.