Code like this: Wait(self.marionette).until(lambda m: self.apps.displayed_app.name == gallery_app.name) .. is not working when the locale is something different than en-US. It seems to me that all the apps like Homescreen, should have an origin as they now have a name. http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/apps/homescreen/app.py#14 That way, we could do the check for the origin instead of the name, like we do here: http://mxr.mozilla.org/gaia/search?string=displayed_app.name Also, I don't think it would hurt to centralize this check in gaia_test.

This is only adding name for all the app.py files and changing the Wait() calls in there to use origin (except for Homescreen, which is too complicated for now). After this lands, the rest of these Wait() calls can be changed: http://mxr.mozilla.org/gaia/search?string=.name%2B%3D%3D&find=.py http://mxr.mozilla.org/gaia/search?string=.name+!%3D&find=.py&findi=&filter=^[^\0]*%24&hitlimit=&tree=gaia

Comment on attachment 8642916 [details] [gaia] mwargers:1188926 > mozilla-b2g:master I was running smoketests locally with this and it seemed to work ok, apart from the failures that start because of bug 1190791.

Comment on attachment 8642916 [details] [gaia] mwargers:1188926 > mozilla-b2g:master minor comment added, but it looks good

Manel, after this first pull request gets in, we need to work on the work mentioned in comment 2. Would you be willing to work on this?

(In reply to Martijn Wargers [:mwargers] (QA) from comment #5) > Manel, after this first pull request gets in, we need to work on the work > mentioned in comment 2. > Would you be willing to work on this? Ok, Martijin I will work on it today

Comment on attachment 8643788 [details] make all apps have origin See comments in pull request.

I added the other file and this PR need review, Thanks

Comment on attachment 8644387 [details] Make all apps have manifestURL I think this looks good, thanks. In any case, this will only be used once we start doing what I mentioned in comment 2.

Comment on attachment 8644387 [details] Make all apps have manifestURL I changed the Wait Call for the file contains the origin, Could you review my PR , thanks

Comment on attachment 8644387 [details] Make all apps have manifestURL See comment in the pull request, you should use .origin, not origin_app_name.

I see you have updated your pull request. If you think it's ready for review, then flag it for review again, please. I noticed one issue with it in facebook.py, where you use .region, where you should just use .origin instead. For the rest, it looks good to me. The pull request pages says: This branch has conflicts that must be resolved So I guess you need to rebase and update the pull request to latest master. To be honest, I don't really know well myself how to do this from the command line. Usually, I just create a new pull request from latest master, then apply the patch from the previous pull request and resolve patch issues from there. After you resolved these issues, we should run an adhoc job on Jenkins for smoketests at least.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #14) > I see you have updated your pull request. If you think it's ready for > review, then flag it for review again, please. > I noticed one issue with it in facebook.py, where you use .region, where you > should just use .origin instead. > For the rest, it looks good to me. > > The pull request pages says: This branch has conflicts that must be resolved > So I guess you need to rebase and update the pull request to latest master. > To be honest, I don't really know well myself how to do this from the > command line. Usually, I just create a new pull request from latest master, > then apply the patch from the previous pull request and resolve patch issues > from there. > > After you resolved these issues, we should run an adhoc job on Jenkins for > smoketests at least. Thanks Martijin, I saw the conflicts issue, and I'll take a look on it maybe because I made the squash of the commit because it's appeared after the Squash.

martijin, it's okay for the conflict issue thanks to jlorenzo for the help on it

Comment on attachment 8644387 [details] Make all apps have manifestURL The pull request looks great now, thanks! Johan should also review. I'll do an adhoc Jenkins job on this pull request for smoke tests, somewhere this weekend hopefully.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #17) > Comment on attachment 8644387 [details] > Make all apps have origin > > The pull request looks great now, thanks! > Johan should also review. > > I'll do an adhoc Jenkins job on this pull request for smoke tests, somewhere > this weekend hopefully. Great, thanks Martijin

I saw this failure with the pull request: TEST-START | test_add_photo_to_contact.py TestContacts.test_add_photo_from_gallery_to_contact TEST-UNEXPECTED-ERROR | test_add_photo_to_contact.py TestContacts.test_add_photo_from_gallery_to_contact | AttributeError: 'Gallery' object has no attribute 'origin_app_name' Traceback (most recent call last): File "/Users/mwargers/.virtualenvs/test3/lib/python2.7/site-packages/marionette_client-0.16-py2.7.egg/marionette/marionette_test.py", line 296, in run testMethod() File "/Users/mwargers/B2G/gaia_clean/tests/python/gaia-ui-tests/gaiatest/tests/functional/contacts/test_add_photo_to_contact.py", line 45, in test_add_photo_from_gallery_to_contact gallery = activities_list.tap_gallery() File "/Users/mwargers/B2G/gaia_clean/tests/python/gaia-ui-tests/gaiatest/apps/system/regions/activities.py", line 53, in tap_gallery Wait(self.marionette).until(lambda m: self.apps.displayed_app.origin == gallery.origin) File "/Users/mwargers/.virtualenvs/test3/lib/python2.7/site-packages/marionette_driver-0.9-py2.7.egg/marionette_driver/wait.py", line 122, in until rv = condition(self.marionette) File "/Users/mwargers/B2G/gaia_clean/tests/python/gaia-ui-tests/gaiatest/apps/system/regions/activities.py", line 53, in <lambda> Wait(self.marionette).until(lambda m: self.apps.displayed_app.origin == gallery.origin) File "/Users/mwargers/B2G/gaia_clean/tests/python/gaia-ui-tests/gaiatest/apps/base.py", line 141, in origin if self.origin_app_name: TEST-INFO took 81591ms The patch I attached, prevents this failure from happening.

Comment on attachment 8644387 [details] Make all apps have manifestURL From the code review standpoint, I don't see any problematic change. Like Martijn said in a comment above, these changes need a full run on the adhoc job, so we'll make sure nothing is broken.

Manel, if you can update your pull request with the suggestion from comment 19, then I can retrigger an adhoc run. If that works, then we can check the pull request in.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #21) > Manel, if you can update your pull request with the suggestion from comment > 19, then I can retrigger an adhoc run. If that works, then we can check the > pull request in. Okay Martijin will do it now

Comment on attachment 8644387 [details] Make all apps have manifestURL Martijin, I updated the PR need review thanks

I see failures in test_browser_share_link.py (and I think more) regarding this pull request, so that's not good. You can also see those failure on your treeherder try run: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=eea47a290ba79084337219c2c6dcd203016838c4&exclusion_profile=false

For the failure in comment 24, I have something that I'll attach as a patch for you, Manel. Johan, for the Contacts app, we have as origin: app://communications.gaiamobile.org/contacts/index.html For this, the origin_app_name trick doesn't work. It just makes me wonder if it's really worth doing it this way. Can't we just define origin on every app.py, but get the hostname "gaiamobile.org" part from base.py? We already had to add origin_app_name for quite a few apps, which makes me think this trick is more trouble than worth.

Like this, this is on top of the latest pull request. There might be more places that need to be fixed. But like I asked in the last needino-?, perhaps we should just use .origin, because this approach becomes more trouble then it's worth, in my opinion.

You're right. origin_app_name is too hackish. Let's keep the origin only for the standard case. Every exception will provide its own origin. For example: In base.py: > DEFAULT_APP_HOSTNAME = 'gaiamobile.org' > > @property > def origin(self): > return 'app://{}.'.format(self.name.lower(), self.DEFAULT_APP_HOSTNAME) In contacts app.py: > origin = 'communications.{}/contacts/index.html'.format(self.DEFAULT_APP_HOSTNAME) In any other app being an exception: > origin = 'foo.{}'.format(self.DEFAULT_APP_HOSTNAME)

In my opinion, it would be better if 'name' would just go away (as soon as it's not used anymore). 'name' shouldn't be used, because it is locale specific. We should use 'origin'.

According to bug 1192628, comment 5, we should use .manifestURL because multiple apps can have the same origin.

Comment on attachment 8644387 [details] Make all apps have manifestURL I need your review for the email's app and the change in Base.py if it's okay I can go ahead for the others change

Comment on attachment 8644387 [details] Make all apps have manifestURL > Make all apps have origin You mean, make them have manifestURL, right? As I told you in bug 1192628, comment 22, this is not right. We need to fix GaiaApps.getDisplayedApp, I think, because it doesn't seem to return a result with a .manifestURL entry: http://mxr.mozilla.org/gaia/source/tests/atoms/gaia_apps.js#367

Comment on attachment 8644387 [details] Make all apps have manifestURL yea, I mean make all app have manifest_url

Comment on attachment 8644387 [details] Make all apps have manifestURL I made the change on all the file in this Pr, but I feel something wrong or missed, need your review and feedback, thanks a lot :)

Comment on attachment 8644387 [details] Make all apps have manifestURL I really feel that we shouldn't have manifest_url property depend on the name property and/or origin_app_name/manifest_url_app or whatever. It makes this all too complicated. I would just define the manifest_url for all the app.py files. In the end, we possibly want to remove the .name propery, if that's possible. Iirc, I chatted with jlorenzo also about this, but I don't recall exactly what we agreed upon.

Comment on attachment 8644387 [details] Make all apps have manifestURL (In reply to Martijn Wargers [:mwargers] (QA) from comment #34) > Iirc, I chatted with jlorenzo also about this, but I don't recall exactly > what we agreed upon. We agreed on the solution you proposed. I documented it in comment 27. The new version of the PR shows some improvement, there are a couple of nits that would break the test execution. You can see more details in the PR.

Comment on attachment 8644387 [details] Make all apps have manifestURL I added comments in the PR.

After discussing in irc me and jlorenzo, we found here: @property def manifest_url(self): return '{}{}.{}/manifest.webapp'.format(self.DEFAULT_PROTOCOL, self.name.lower(), self.DEFAULT_APP_HOSTNAME) we use property name in the url but there are some manifestURL the name contain is different than the one defined by us so we decide to use the name Class like here in the PR: https://github.com/mozilla-b2g/gaia/pull/31259#discussion-diff-37986457R153 And for some other App like Bugzilla Lite the class name and property name is different than the one used in manifestURL, so we define the manifestURL directly in the app like this: https://github.com/mozilla-b2g/gaia/pull/31259#discussion-diff-37987100R7

(In reply to Martijn Wargers [:mwargers] (QA) from comment #36) > Comment on attachment 8644387 [details] > Make all apps have manifestURL > > I added comments in the PR. I saw all your comments but I have something in the static method we test for the equals manifestURL: https://github.com/mermi/gaia/blob/bug-1188926/tests/python/gaia-ui-tests/gaiatest/apps/base.py#L158 and then, I guess we need another one for this case: https://github.com/mermi/gaia/blob/bug-1188926/tests/python/gaia-ui-tests/gaiatest/apps/wallpaper/app.py#L21

(In reply to Manel Rhaiem [:manel] from comment #38) > I saw all your comments but I have something in the static method we test > for the equals manifestURL: > https://github.com/mermi/gaia/blob/bug-1188926/tests/python/gaia-ui-tests/ > gaiatest/apps/base.py#L158 Yes, that's what I was asking you to use in the pull request.

Thanks, the pull request looks much better now. I added some comments and questions, but I'll wait on Johan to finish his review first, this time. One thing that could be a problem is that in this way, there is no distinction between phone and contacts (they both have app://communications.gaiamobile.org/manifest.webapp) as manifest_url. That's where the entry_point thing comes in, but I'd rather ignore that now and first try to fix this.

Comment on attachment 8644387 [details] Make all apps have manifestURL The factorization of the waits in `wait_to_be_displayed()`, makes the PR easier to read, thanks! I don't r+ mainly because of [1]. Mostly every test will fail, if we try define the class constants in __init__(). Python interprets these constants as local ones, and not class ones. A second factorization[2] will improve the readability of the remaining Waits. And some clean up is needed at [3]. If we redefine 'app://', 'gaiamobile.org' and 'manifest.webapp' as plain string in each Page Class, there is no point of creating class constants. Thank you very much for helping us here, this modification of the page classes is definitely a hard task. And, this PR is on the edge of being done :) [1] https://github.com/mozilla-b2g/gaia/pull/31259/files#r37986929 [2] https://github.com/mozilla-b2g/gaia/pull/31259/files#r37988212 [3] https://github.com/mozilla-b2g/gaia/pull/31259/files#r37987364

Comment on attachment 8644387 [details] Make all apps have manifestURL Like I said in comment 40, the pull request looks much better, but there are some things that need to be addressed, which I already mentioned in the pull request. For instance, you need to remove the occurences of 'manifest_url_app'. I added some other comments for things that need to improve, I think. Also, you need to address the things Johan mentioned in comment 41. And I think you need to run some tests (smoketests for instance) locally with your patch, to see if it's causing failures. And if there are failures, which are caused by your pull request, then you should try to fix those. Just let me know if you need help.

Comment on attachment 8644387 [details] Make all apps have manifestURL Agreed with Martijn.

I tried smoketests, and it's work well I think the issue we got Monday it because I didn't call python setup.py develop after the change I made on gaia_apps.js, then I think we don't need to change anything on gaia_test.py. thanks Martijin and Johan for your help and your time, I am trying to fix what is mentioned from the last comments (41 and 42)

Comment on attachment 8644387 [details] Make all apps have manifestURL There are still a couple of issues in the pull request that you need to address, I think. See comments in the pull request. Once you address these issues, I'll run the smoketests locally on my phone with this pull request applied to check if everything goes smoothly.

Comment on attachment 8644387 [details] Make all apps have manifestURL We're at a couple of lines (of code) from the end! Some small nits are remaining. This nits will make the test fail, that's why I don't r+. You also have some conflits, you'll need to rebase your PR.

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #46) > Comment on attachment 8644387 [details] > Make all apps have manifestURL > > We're at a couple of lines (of code) from the end! > > Some small nits are remaining. This nits will make the test fail, that's why > I don't r+. You also have some conflits, you'll need to rebase your PR. conflit issue fixed. Martijin and Johan, I will review the files to check if they are any other nits, Thanks again :)

Comment on attachment 8644387 [details] Make all apps have manifestURL I fixed the issue mentioned in the last comments in the PR, I launched smoketest and its working well in my side, need your feedback and Review, thanks a lot for your time :)

Unfortunately, there were actually many problems still to be solved. This is a patch on top of the pull request from Manel.

Comment on attachment 8644387 [details] Make all apps have manifestURL >https://github.com/mozilla-b2g/gaia/pull/31747

Comment on attachment 8658411 [details] [gaia] mermi:bug1188926 > mozilla-b2g:master In the system subdirectory, there is a whole bunch of tests that are failing during launch with the latest pull request: test_system_message.py test_system_message.TestSystemMessage.test_app_launched_by_system_message test_system_message_pending.py test_system_message_pending.TestSystemMessage.test_pending_system_message test_inter_app_comm.py test_inter_app_comm.TestInterAppComm.test_inter_app_comm test_privileged_app_audio_capture_prompt.py test_privileged_app_contacts_prompt.py test_privileged_app_device_music_prompt.py test_privileged_app_device_picture_prompt.py test_privileged_app_device_sdcard_prompt.py test_privileged_app_device_video_prompt.py test_privileged_app_geolocation_prompt.py Probably some app objects in there need some manifest_url fixing. Please ask again for review when you've fixed those failures.

Comment on attachment 8658411 [details] [gaia] mermi:bug1188926 > mozilla-b2g:master Some minor nits found (see the PR). Martijn is right, there are some Page classes that need to be updated also. For: * test_privileged_app_*.py, see https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/ui_tests_privileged/app.py * test_inter_app_comm.py, https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/system/regions/iac_publisher.py * test_system_message*.py, https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/testapp/app.py * test_email_keyboard, see https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/ui_tests/app.py Thank you for your patience with this patch. You made some great progress to know how our tests work. We thought it was an easy one to do at the beginning, but it turned out to be way longer with more edge cases than we expected.

Following up the previous comment, the URL are: * app://uitest-privileged.gaiamobile.org/manifest.webapp * app://test-iac-publisher.gaiamobile.org/manifest.webapp * app://test-container.gaiamobile.org/manifest.webapp * app://uitest.gaiamobile.org/manifest.webapp

Comment on attachment 8658411 [details] [gaia] mermi:bug1188926 > mozilla-b2g:master fixed all the failure tests mentioned in comment 53 and comment 52 please need your review and I'll check if we still have another failure test in this PR

Comment on attachment 8658411 [details] [gaia] mermi:bug1188926 > mozilla-b2g:master I don't see any nit. The patch looks good to me. Regarding the failure in test_music_share_ringtone.py, I'm unclear about the reason of the failure. Then, I'm fine with this patch (modulo the error). Before landing this patch, Martijn could you run an adhoc job so we can have a link that shows the patch is nicely working? Thank you!

Comment on attachment 8658411 [details] [gaia] mermi:bug1188926 > mozilla-b2g:master I think it looks good now. I kicked of 2 Jenkins runs: http://jenkins1.qa.scl3.mozilla.com/view/Mozilla%20Lab/job/flame-kk.ui.adhoc/888/ http://jenkins1.qa.scl3.mozilla.com/view/Mozilla%20Lab/job/flame-kk.ui.adhoc/889/

There were 2 issues showing up in http://jenkins1.qa.scl3.mozilla.com/view/Mozilla%20Lab/job/flame-kk.ui.adhoc/889/ , Mermi fixed those (easy fixes) in the pull request. Merged: https://github.com/mozilla-b2g/gaia/commit/966fa584e67354cf835151320670f682dee1ba7f Thanks for you help and perseverance, Manel!

There are 2 things that we need to do as a follow-up: * There are probably still quite some of these Wait self.apps.displayed_app.name calls left out there. We should all replace them with the wait_to_(not_)be_displayed method. * For Phone and Contacts checking, we should check for the entry_point, we need to adjust gaia_apps.js for that, to include entry_point for the app object, if it's there.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #59) > There are 2 things that we need to do as a follow-up: > * There are probably still quite some of these Wait > self.apps.displayed_app.name calls left out there. We should all replace > them with the wait_to_(not_)be_displayed method. for this point, I will open a new bug and start working on it tomorrow. > * For Phone and Contacts checking, we should check for the entry_point, we > need to adjust gaia_apps.js for that, to include entry_point for the app > object, if it's there. about the entry_point we need to know what will happen after this bug 1135340

This casues a JSHint error: tests/atoms/gaia_apps.js: line 375, col 115, Line is too long. (ERROR) It also does not have the necessary bug number in the commit message. Please fix the commit message, as well as the lint error (and ensure all tests pass before landing). Backout: https://github.com/mozilla-b2g/gaia/commit/25fd808f6ce2d8582077efc17feed8dba7dd4424

Comment on attachment 8660217 [details] PR-ManifestURL You put the bug number in the pull request now and shortened the line that jshint was complaining about in gaia_apps.js. I'm sure it looks ok, but I have to wait on the result here before I can merge: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=7b818f0bc50cccfdee2dc8e8ddc371a514e08e6a

Comment on attachment 8660217 [details] PR-ManifestURL Jshint working now for tests/atoms/gaia_apps.js and summary was added. Merged: https://github.com/mozilla-b2g/gaia/commit/4d9b996be4b1935651057d0651461c1a36d98a18

The browser app - search, doesn't have consistent behavior for manifestUrl. In fact, if it visits webpage the origin and name will change as well. This certainly blocked mtbf tests for browser.

(In reply to Paul Yang [:pyang] from comment #68) > The browser app - search, doesn't have consistent behavior for manifestUrl. > In fact, if it visits webpage the origin and name will change as well. This > certainly blocked mtbf tests for browser. You're talking about the Gaia App, not the Python App object, right? ? I noticed something similar like that when querying for the visible app, see bug 1169010, comment 12. What I noticed was that .manifestURL of the browser app gets null as soon as you visit an url. .origin still stays app://search.gaiamobile.org, though. Not sure about name, I didn't test that. I don't know how the pull request in this bug would have changed that, because the search app.py already made use of manifest_url, so launching the browser app would already go through the manifest_url path, I would think.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #59) > There are 2 things that we need to do as a follow-up: > * There are probably still quite some of these Wait > self.apps.displayed_app.name calls left out there. We should all replace > them with the wait_to_(not_)be_displayed method. I filed bug 1204888. > * For Phone and Contacts checking, we should check for the entry_point, we > need to adjust gaia_apps.js for that, to include entry_point for the app > object, if it's there. I filed bug 1204894.