Closed
Bug 1188926
Opened 9 years ago
Closed 9 years ago
Make all the apps have an manifestURL as well as a name and check for that
Categories
(Firefox OS Graveyard :: Gaia::UI Tests, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: martijn.martijn, Assigned: manel)
References
Details
Attachments
(1 file, 11 obsolete files)
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.
Reporter | ||
Updated•9 years ago
|
Summary: Make → Make all the apps have an origin as well as a name and check for that
Comment 1•9 years ago
|
||
Reporter | ||
Comment 2•9 years ago
|
||
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
Reporter | ||
Comment 3•9 years ago
|
||
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.
Attachment #8642916 -
Flags: review?(npark)
Attachment #8642916 -
Flags: review?(jlorenzo)
Comment 4•9 years ago
|
||
Comment on attachment 8642916 [details]
[gaia] mwargers:1188926 > mozilla-b2g:master
minor comment added, but it looks good
Attachment #8642916 -
Flags: review?(npark) → review+
Reporter | ||
Comment 5•9 years ago
|
||
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?
Flags: needinfo?(manel.rhaiem92)
Assignee | ||
Comment 6•9 years ago
|
||
(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
Flags: needinfo?(manel.rhaiem92)
Reporter | ||
Updated•9 years ago
|
Assignee: martijn.martijn → manel.rhaiem92
Reporter | ||
Updated•9 years ago
|
Attachment #8642916 -
Attachment is obsolete: true
Attachment #8642916 -
Flags: review?(jlorenzo)
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8643788 -
Flags: review?(martijn.martijn)
Attachment #8643788 -
Flags: review?(jlorenzo)
Reporter | ||
Comment 9•9 years ago
|
||
Comment on attachment 8643788 [details]
make all apps have origin
See comments in pull request.
Attachment #8643788 -
Flags: review?(martijn.martijn) → review-
Assignee | ||
Comment 10•9 years ago
|
||
I added the other file and this PR need review, Thanks
Attachment #8644387 -
Flags: review?(martijn.martijn)
Reporter | ||
Comment 11•9 years ago
|
||
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.
Attachment #8644387 -
Flags: review?(martijn.martijn)
Attachment #8644387 -
Flags: review?(jlorenzo)
Attachment #8644387 -
Flags: review+
Reporter | ||
Updated•9 years ago
|
Attachment #8643788 -
Attachment is obsolete: true
Attachment #8643788 -
Flags: review?(jlorenzo)
Reporter | ||
Updated•9 years ago
|
Attachment #8643785 -
Attachment is obsolete: true
Assignee | ||
Comment 12•9 years ago
|
||
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
Attachment #8644387 -
Flags: review?(martijn.martijn)
Attachment #8644387 -
Flags: review?(jlorenzo)
Attachment #8644387 -
Flags: review+
Reporter | ||
Comment 13•9 years ago
|
||
Comment on attachment 8644387 [details]
Make all apps have manifestURL
See comment in the pull request, you should use .origin, not origin_app_name.
Attachment #8644387 -
Flags: review?(martijn.martijn) → review-
Reporter | ||
Comment 14•9 years ago
|
||
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.
Assignee | ||
Comment 15•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review- → review?(jlorenzo)
Assignee | ||
Comment 16•9 years ago
|
||
martijin, it's okay for the conflict issue thanks to jlorenzo for the help on it
Flags: needinfo?(martijn.martijn)
Reporter | ||
Comment 17•9 years ago
|
||
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.
Flags: needinfo?(martijn.martijn)
Attachment #8644387 -
Flags: review+
Assignee | ||
Comment 18•9 years ago
|
||
(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
Reporter | ||
Comment 19•9 years ago
|
||
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.
Attachment #8644387 -
Flags: review?(jlorenzo) → review+
Reporter | ||
Comment 21•9 years ago
|
||
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.
Flags: needinfo?(manel.rhaiem92)
Assignee | ||
Comment 22•9 years ago
|
||
(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
Flags: needinfo?(manel.rhaiem92)
Assignee | ||
Comment 23•9 years ago
|
||
Comment on attachment 8644387 [details]
Make all apps have manifestURL
Martijin, I updated the PR need review thanks
Reporter | ||
Comment 24•9 years ago
|
||
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
Reporter | ||
Comment 25•9 years ago
|
||
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.
Flags: needinfo?(jlorenzo)
Reporter | ||
Comment 26•9 years ago
|
||
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.
Attachment #8645479 -
Attachment is obsolete: true
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)
Flags: needinfo?(jlorenzo)
Reporter | ||
Comment 28•9 years ago
|
||
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'.
Blocks: 1195364
Reporter | ||
Comment 29•9 years ago
|
||
According to bug 1192628, comment 5, we should use .manifestURL because multiple apps can have the same origin.
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review+ → review?(jlorenzo)
Assignee | ||
Comment 30•9 years ago
|
||
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
Attachment #8644387 -
Flags: review+ → review?(martijn.martijn)
Reporter | ||
Comment 31•9 years ago
|
||
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
Attachment #8644387 -
Flags: review?(martijn.martijn) → review-
Assignee | ||
Comment 32•9 years ago
|
||
Comment on attachment 8644387 [details]
Make all apps have manifestURL
yea, I mean make all app have manifest_url
Attachment #8644387 -
Flags: review- → review?(martijn.martijn)
Assignee | ||
Updated•9 years ago
|
Summary: Make all the apps have an origin as well as a name and check for that → Make all the apps have an manifestURL as well as a name and check for that
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review?(martijn.martijn)
Attachment #8644387 -
Flags: review?(jlorenzo)
Attachment #8644387 -
Flags: review-
Assignee | ||
Comment 33•9 years ago
|
||
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 :)
Attachment #8644387 -
Flags: review?(martijn.martijn)
Attachment #8644387 -
Flags: review?(jlorenzo)
Attachment #8644387 -
Flags: review-
Reporter | ||
Comment 34•9 years ago
|
||
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.
Attachment #8644387 -
Flags: review?(martijn.martijn)
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Attachment description: Make all apps have origin → Make all apps have manifestURL
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.
Attachment #8644387 -
Flags: review?(jlorenzo)
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review?(martijn.martijn)
Attachment #8644387 -
Flags: review?(jlorenzo)
Reporter | ||
Comment 36•9 years ago
|
||
Comment on attachment 8644387 [details]
Make all apps have manifestURL
I added comments in the PR.
Attachment #8644387 -
Flags: review?(martijn.martijn) → review-
Assignee | ||
Comment 37•9 years ago
|
||
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
Assignee | ||
Comment 38•9 years ago
|
||
(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
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review- → review?(martijn.martijn)
Reporter | ||
Comment 39•9 years ago
|
||
(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.
Reporter | ||
Comment 40•9 years ago
|
||
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
Attachment #8644387 -
Flags: review?(jlorenzo)
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review?(martijn.martijn) → review-
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review- → review?(jlorenzo)
Reporter | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review?(martijn.martijn)
Reporter | ||
Comment 42•9 years ago
|
||
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.
Attachment #8644387 -
Flags: review?(martijn.martijn)
Comment on attachment 8644387 [details]
Make all apps have manifestURL
Agreed with Martijn.
Attachment #8644387 -
Flags: review?(jlorenzo)
Assignee | ||
Comment 44•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review?(martijn.martijn)
Attachment #8644387 -
Flags: review?(jlorenzo)
Reporter | ||
Comment 45•9 years ago
|
||
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.
Attachment #8644387 -
Flags: review?(martijn.martijn) → review-
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.
Attachment #8644387 -
Flags: review?(jlorenzo)
Assignee | ||
Comment 47•9 years ago
|
||
(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 :)
Assignee | ||
Comment 48•9 years ago
|
||
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 :)
Attachment #8644387 -
Flags: review- → review?(jlorenzo)
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review?(martijn.martijn)
Reporter | ||
Comment 49•9 years ago
|
||
Unfortunately, there were actually many problems still to be solved. This is a patch on top of the pull request from Manel.
Attachment #8648407 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review?(martijn.martijn)
Comment 50•9 years ago
|
||
Assignee | ||
Comment 51•9 years ago
|
||
Comment on attachment 8644387 [details]
Make all apps have manifestURL
>https://github.com/mozilla-b2g/gaia/pull/31747
Attachment #8644387 -
Flags: review?(jlorenzo) → review-
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review?(martijn.martijn)
Attachment #8644387 -
Flags: review?(jlorenzo)
Attachment #8644387 -
Flags: review-
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Flags: review?(martijn.martijn)
Attachment #8644387 -
Flags: review?(jlorenzo)
Attachment #8644387 -
Flags: review-
Assignee | ||
Updated•9 years ago
|
Attachment #8658411 -
Flags: review?(martijn.martijn)
Attachment #8658411 -
Flags: review?(jlorenzo)
Assignee | ||
Updated•9 years ago
|
Attachment #8644387 -
Attachment is obsolete: true
Reporter | ||
Comment 52•9 years ago
|
||
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.
Attachment #8658411 -
Flags: review?(martijn.martijn)
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.
Attachment #8658411 -
Flags: review?(jlorenzo)
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
Assignee | ||
Comment 55•9 years ago
|
||
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
Attachment #8658411 -
Flags: review?(martijn.martijn)
Attachment #8658411 -
Flags: review?(jlorenzo)
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!
Attachment #8658411 -
Flags: review?(jlorenzo) → review+
Reporter | ||
Comment 57•9 years ago
|
||
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/
Attachment #8658411 -
Flags: review?(martijn.martijn) → review+
Reporter | ||
Comment 58•9 years ago
|
||
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!
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 59•9 years ago
|
||
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.
Assignee | ||
Comment 60•9 years ago
|
||
(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
Comment 61•9 years ago
|
||
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
Status: RESOLVED → REOPENED
Flags: needinfo?(martijn.martijn)
Flags: needinfo?(manel.rhaiem92)
Resolution: FIXED → ---
Comment 62•9 years ago
|
||
Comment 63•9 years ago
|
||
Assignee | ||
Comment 64•9 years ago
|
||
Flags: needinfo?(manel.rhaiem92)
Attachment #8660217 -
Flags: review?(martijn.martijn)
Assignee | ||
Updated•9 years ago
|
Attachment #8660205 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8660216 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8660217 -
Attachment description: hope-final PR → PR-ManifestURL
Reporter | ||
Comment 65•9 years ago
|
||
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
Flags: needinfo?(martijn.martijn)
Attachment #8660217 -
Flags: review?(martijn.martijn) → review+
Comment 66•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Attachment #8660300 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8657635 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8658411 -
Attachment is obsolete: true
Reporter | ||
Comment 67•9 years ago
|
||
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
Reporter | ||
Updated•9 years ago
|
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Comment 68•9 years ago
|
||
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.
Reporter | ||
Comment 69•9 years ago
|
||
(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.
Reporter | ||
Updated•9 years ago
|
Reporter | ||
Comment 70•9 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•