1212843 - Provide an install method in gaia_test.py and convert uninstall the use manifestURL

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#146
http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_apps.js#379

uninstall is used here:
http://mxr.mozilla.org/gaia/search?string=uninstall&find=.py

We should use manifestURL, since that's locale agnostic.
We should rewrite GaiaApps.uninstallWithName in gaia_apps.js to use GaiaApps.uninstallWithManifestURL.

The install method that we're going to add, should be similar as the uninstall method in gaia_test.py, except it should install the app instead of uninstalling it.

Manel, let me know if you want to work on this, otherwise I can take this.

Comment 1

[Manel Rhaiem [:manel]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #0)
> http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/
> gaia_test.py#146
> http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_apps.js#379
> 
> uninstall is used here:
> http://mxr.mozilla.org/gaia/search?string=uninstall&find=.py
> 
> We should use manifestURL, since that's locale agnostic.
> We should rewrite GaiaApps.uninstallWithName in gaia_apps.js to use
> GaiaApps.uninstallWithManifestURL.
> 
> The install method that we're going to add, should be similar as the
> uninstall method in gaia_test.py, except it should install the app instead
> of uninstalling it.
> 
> Manel, let me know if you want to work on this, otherwise I can take this.

yes, I already start on it, thanks Martijn

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] mermi:bug-1212843 > mozilla-b2g:master

Comment 3

[Martijn Wargers (dead)]

Comment on attachment 8671456 [details] [review]
[gaia] mermi:bug-1212843 > mozilla-b2g:master

Thanks, this looks very much of what I had in mind.
One thing I meant also is to have the uninstall method use manifestURL (or perhaps manifest_url, because this is Python, oops!).
So like this:
def uninstall(self, manifest_url):

You would then need to adjust the other uninstall usage:
http://mxr.mozilla.org/gaia/search?string=uninstall&find=.py

Comment 4

[Manel Rhaiem [:manel]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #3)
> Comment on attachment 8671456 [details] [review]
> [gaia] mermi:bug-1212843 > mozilla-b2g:master
> 
> Thanks, this looks very much of what I had in mind.
> One thing I meant also is to have the uninstall method use manifestURL (or
> perhaps manifest_url, because this is Python, oops!).
> So like this:
> def uninstall(self, manifest_url):
> 
> You would then need to adjust the other uninstall usage:
> http://mxr.mozilla.org/gaia/search?string=uninstall&find=.py

ah you mean the one in gaia_test.py, yes Martijn I am doing it now and I will adjust this files too http://mxr.mozilla.org/gaia/search?string=uninstall&find=.py , thanks :)

Comment 5

[Martijn Wargers (dead)]

You can also remove uninstallWithName from gaia_apps.js once that's not used anymore, btw.

Comment 6

[Manel Rhaiem [:manel]]

I tried out a test (test_homescreen_launch_app.py) and I got this failure: https://pastebin.mozilla.org/8848753
from this failure above, it couldn't launch the app with the manifestURL required, so I think we have to adjust the launchWithManifestURL http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_apps.js#179 to get it work with manifestURL not just entry_point and name, What do you think Martijn ?

Comment 7

[Martijn Wargers (dead)]

Comment on attachment 8671456 [details] [review]
[gaia] mermi:bug-1212843 > mozilla-b2g:master

See comment in the pull request.

Comment 8

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8671456 [details] [review]
[gaia] mermi:bug-1212843 > mozilla-b2g:master

The current implementation doesn't work. The main reason is that, currently, installAppWithManifest() tries to locate an app before installing it. I proposed a solution in the PR.

Comment 9

[Manel Rhaiem [:manel]]

After updating the PR with the changed from the comments of Jlorenzo and mwargers, I tried out a test but it's failure https://pastebin.mozilla.org/8848876

So here we are calling the install app https://github.com/mermi/gaia/blob/bug-1212843/tests/atoms/gaia_apps.js#L379 but this function is undefined, we have just uninstall defined there, then I am searching where I should add install function in another way !?

Comment 10

[Martijn Wargers (dead)]

Attachment: all.diff

I would do something like this. This is on top of your pull request. I tested it on test_homescreen_launch_app.py at least.
But Johan might have a different idea about this.

Comment 11

[Johan Lorenzo [:jlorenzo]]

Martijn's proposal looks fine to me. I'd reduce install() even more, thanks to a method like "handleRequest", but I wouldn't block on it.

Comment 12

[Manel Rhaiem [:manel]]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #11)
> Martijn's proposal looks fine to me. I'd reduce install() even more, thanks
> to a method like "handleRequest", but I wouldn't block on it.

Thanks Jlorenzo, I'll apply the patch of Matijn and try out some tests

Comment 13

[Martijn Wargers (dead)]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #11)
> Martijn's proposal looks fine to me. I'd reduce install() even more, thanks
> to a method like "handleRequest", but I wouldn't block on it.

I'm not opposed to it, mainly I was just trying to get Manel's patch in a working state and it was easier to get the handleRequest part out of it. But Manel can try to put it back if she wants or we can do in a follow-up bug.

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] mermi:bug1212843 > mozilla-b2g:master

Comment 15

[GitHub Autolander Bot]

Attachment: [gaia] mermi:patchBug1212843 > mozilla-b2g:master

Comment 16

[Manel Rhaiem [:manel]]

after running the test_homescreen_launch_app_packaged we got a failure on the app and apparently tearDown is failed:https://irccloud.mozilla.com/pastebin/6G8jAesb

So discussed on IRC with jlorenzo we think we need to add some cases here : http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_apps.js#179
We need the case
* when the app doesn't provide the entrypoint should return the manifest_url instead of the name 

* when no app installed for the instance, so no app will be returned from this line http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_apps.js#183

So I'll try to advance on this tonight, and Martijn What do you thing about it? if you have any suggestion?! 

Thanks.

Comment 17

[Martijn Wargers (dead)]

Attachment: gaia_apps.diff

This is on top of your pull request.
locateWithManifestURL needed a fix in the case where there is no app found.
For the rest, gaia_apps.js doesn't need any changes, afaik.
This makes test_homescreen_delete_app.py work for me at tearDown.

Comment 18

[Martijn Wargers (dead)]

Comment on attachment 8678473 [details] [review]
[gaia] mermi:patchBug1212843 > mozilla-b2g:master

You're removing things related to entryPoint in getDisplayedApp that shouldn't be removed, I think. If you think it should be removed, please explain to me why.
Also, check with jshint if gaia_apps.js doesn't show any errors.

Comment 19

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8678473 [details] [review]
[gaia] mermi:patchBug1212843 > mozilla-b2g:master

This patch is on the right track. Like Martijn said, some removed parts look odd. We also have to make sure that the packaged app installation needs all these changes. I left more details in the PR.

Just to note, JSHint passed on the latest revision: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=129c34ac1e4ebb805c1251eb580e882ce0a8fd4b

Comment 20

[Manel Rhaiem [:manel]]

Comment on attachment 8678473 [details] [review]
[gaia] mermi:patchBug1212843 > mozilla-b2g:master

I made all the necessary change, we have something weird in the test of packaged app when I run this test everything works well expect this wait https://github.com/mozilla-b2g/gaia/pull/32719/files#diff-45a3a9f9fabeddb526c8c478a4490d38R47
Failure: https://pastebin.mozilla.org/8851136
When I tried to see the value of the h1 element it shows "Error Response" even in the app and after removing the packaged variable https://github.com/mozilla-b2g/gaia/pull/32719/files#diff-507d02ce236ea5ec35a3aa5b3e5b051dR391 as jlorenzo suggested I still get this error, so I think something wrong with the Packaged app.

Comment 21

[Martijn Wargers (dead)]

The test is running fine without your pull request, right? Then in that case, there is something wrong with the pull request.

Comment 22

[Manel Rhaiem [:manel]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #21)
> The test is running fine without your pull request, right? Then in that
> case, there is something wrong with the pull request.

Yes I found the error yesterday, actually the install method was changed from installPackage to install so I think we need to add this one in gaia_test, what do you think ?

Comment 23

[Manel Rhaiem [:manel]]

Comment on attachment 8678473 [details] [review]
[gaia] mermi:patchBug1212843 > mozilla-b2g:master

I have some nits in gaia_app.js when I run with jshint I will ask for review when I fix them thanks

Comment 24

[Johan Lorenzo [:jlorenzo]]

(In reply to Manel Rhaiem [:manel] from comment #22)
> Yes I found the error yesterday, actually the install method was changed
> from installPackage to install so I think we need to add this one in
> gaia_test, what do you think ?
Yes, please expose install_package(self, manifest_url) in gaia_test.py.

Comment 25

[Martijn Wargers (dead)]

test_homescreen_delete_app.py is currently failing because of bug 1220736. It's a little difficult to test out now.

Comment 26

[Martijn Wargers (dead)]

Comment on attachment 8678473 [details] [review]
[gaia] mermi:patchBug1212843 > mozilla-b2g:master

Looks good, I assume you tested out test_homescreen_launch_app and test_homescreen_launch_app_packaged.

Comment 27

[Manel Rhaiem [:manel]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #26)
> Comment on attachment 8678473 [details] [review]
> [gaia] mermi:patchBug1212843 > mozilla-b2g:master
> 
> Looks good, I assume you tested out test_homescreen_launch_app and
> test_homescreen_launch_app_packaged.

yes I did Martijn, Also I tested delete and it works for me!

Comment 28

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8678473 [details] [review]
[gaia] mermi:patchBug1212843 > mozilla-b2g:master

Mermi and I discussed the last remaining issues on IRC. The last revision is good. I ran launch_app{_packaged).py and delete_app.py and every case were passing on top of [1]. I should mention that some parts of the delete_app test need bug 1220736. In order the make the tests pass, I had to manually cover the parts that bug 1220736 will fix. 

Thank you for your hard work on it, Mermi!

[1] Build ID               20151104120407
Gaia Revision          47da49f8206788d70d834c3a63d9245d50c89103
Gaia Date              2015-11-03 21:48:23
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/6077f51254c69a1e14e1b61acba4af451bf1783e
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151104.112232
Firmware Date          Wed Nov  4 11:22:40 UTC 2015
Bootloader             s1

Comment 29

[Johan Lorenzo [:jlorenzo]]

Landed in master at: https://github.com/mozilla-b2g/gaia/commit/db4dabf9b3932fcfccd078fde6bac2d56624d2d9