Closed Bug 1212843 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: manel)

Details

Attachments

(2 files, 3 obsolete files)

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.
Flags: needinfo?(manel.rhaiem92)
(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
Flags: needinfo?(manel.rhaiem92)
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
(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 :)
Assignee: martijn.martijn → manel.rhaiem92
You can also remove uninstallWithName from gaia_apps.js once that's not used anymore, btw.
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 ?
Flags: needinfo?(martijn.martijn)
Attachment #8671456 - Flags: review?(martijn.martijn)
Attachment #8671456 - Flags: review?(jlorenzo)
Comment on attachment 8671456 [details] [review]
[gaia] mermi:bug-1212843 > mozilla-b2g:master

See comment in the pull request.
Flags: needinfo?(martijn.martijn)
Attachment #8671456 - Flags: review?(martijn.martijn)
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.
Attachment #8671456 - Flags: review?(jlorenzo)
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 !?
Flags: needinfo?(martijn.martijn)
Flags: needinfo?(jlorenzo)
Attached patch all.diff (obsolete) — Splinter Review
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.
Flags: needinfo?(martijn.martijn)
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.
Flags: needinfo?(jlorenzo)
(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
(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.
Attachment #8671456 - Attachment is obsolete: true
Attachment #8675506 - Attachment is obsolete: true
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.
Flags: needinfo?(martijn.martijn)
Attached patch gaia_apps.diffSplinter Review
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.
Attachment #8672292 - Attachment is obsolete: true
Flags: needinfo?(martijn.martijn)
Attachment #8678473 - Flags: review?(martijn.martijn)
Attachment #8678473 - Flags: review?(jlorenzo)
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.
Attachment #8678473 - Flags: review?(martijn.martijn)
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
Attachment #8678473 - Flags: review?(jlorenzo) → feedback+
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.
Attachment #8678473 - Flags: review?(martijn.martijn)
Attachment #8678473 - Flags: review?(jlorenzo)
The test is running fine without your pull request, right? Then in that case, there is something wrong with the pull request.
(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 ?
Flags: needinfo?(jlorenzo)
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
Attachment #8678473 - Flags: review?(martijn.martijn)
Attachment #8678473 - Flags: review?(jlorenzo)
Attachment #8678473 - Flags: review-
(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.
Flags: needinfo?(jlorenzo)
Attachment #8678473 - Flags: review?(martijn.martijn)
Attachment #8678473 - Flags: review?(jlorenzo)
Attachment #8678473 - Flags: review-
test_homescreen_delete_app.py is currently failing because of bug 1220736. It's a little difficult to test out now.
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.
Attachment #8678473 - Flags: review?(martijn.martijn) → review+
(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 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
Attachment #8678473 - Flags: review?(jlorenzo) → review+
Landed in master at: https://github.com/mozilla-b2g/gaia/commit/db4dabf9b3932fcfccd078fde6bac2d56624d2d9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.