bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Make all the apps have an manifestURL as well as a name and check for that

RESOLVED FIXED

Status

Firefox OS
Gaia::UI Tests
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: manel)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

47 bytes, text/x-github-pull-request
Martijn Wargers (zombie)
: review+
Details | Review | Splinter Review
(Reporter)

Description

3 years ago
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

3 years ago
Summary: Make → Make all the apps have an origin as well as a name and check for that
(Assignee)

Updated

3 years ago
Blocks: 1190562
(Reporter)

Updated

3 years ago
No longer blocks: 1190562
Created attachment 8642916 [details] [review]
[gaia] mwargers:1188926 > mozilla-b2g:master
(Reporter)

Comment 2

3 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

3 years ago
Comment on attachment 8642916 [details] [review]
[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 on attachment 8642916 [details] [review]
[gaia] mwargers:1188926 > mozilla-b2g:master

minor comment added, but it looks good
Attachment #8642916 - Flags: review?(npark) → review+
(Reporter)

Comment 5

3 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

3 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

3 years ago
Assignee: martijn.martijn → manel.rhaiem92
(Reporter)

Updated

3 years ago
Attachment #8642916 - Attachment is obsolete: true
Attachment #8642916 - Flags: review?(jlorenzo)
Created attachment 8643785 [details] [review]
[gaia] mermi:bug-1188926 > mozilla-b2g:master
(Assignee)

Comment 8

3 years ago
Created attachment 8643788 [details] [review]
make all apps have origin
Attachment #8643788 - Flags: review?(martijn.martijn)
Attachment #8643788 - Flags: review?(jlorenzo)
(Reporter)

Comment 9

3 years ago
Comment on attachment 8643788 [details] [review]
make all apps have origin

See comments in pull request.
Attachment #8643788 - Flags: review?(martijn.martijn) → review-
(Assignee)

Comment 10

3 years ago
Created attachment 8644387 [details] [review]
Make all apps have manifestURL

I added the other file and this PR need review, Thanks
Attachment #8644387 - Flags: review?(martijn.martijn)
(Reporter)

Comment 11

3 years ago
Comment on attachment 8644387 [details] [review]
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

3 years ago
Attachment #8643788 - Attachment is obsolete: true
Attachment #8643788 - Flags: review?(jlorenzo)
(Reporter)

Updated

3 years ago
Attachment #8643785 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
Comment on attachment 8644387 [details] [review]
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

3 years ago
Comment on attachment 8644387 [details] [review]
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

3 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

3 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

3 years ago
Attachment #8644387 - Flags: review- → review?(jlorenzo)
(Assignee)

Comment 16

3 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

3 years ago
Comment on attachment 8644387 [details] [review]
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

3 years ago
(In reply to Martijn Wargers [:mwargers] (QA) from comment #17)
> Comment on attachment 8644387 [details] [review]
> 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)

Updated

3 years ago
Depends on: 1192628
(Reporter)

Comment 19

3 years ago
Created attachment 8645479 [details] [diff] [review]
base.diff

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] [review]
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

3 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

3 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

3 years ago
Comment on attachment 8644387 [details] [review]
Make all apps have manifestURL

Martijin, I updated the PR need review thanks
(Reporter)

Comment 24

3 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

3 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

3 years ago
Created attachment 8648407 [details] [diff] [review]
all.diff

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

3 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'.
(Reporter)

Comment 29

3 years ago
According to bug 1192628, comment 5, we should use .manifestURL because multiple apps can have the same origin.
(Assignee)

Updated

3 years ago
Attachment #8644387 - Flags: review+ → review?(jlorenzo)
(Assignee)

Comment 30

3 years ago
Comment on attachment 8644387 [details] [review]
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

3 years ago
Comment on attachment 8644387 [details] [review]
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

3 years ago
Comment on attachment 8644387 [details] [review]
Make all apps have manifestURL

yea, I mean make all app have manifest_url
Attachment #8644387 - Flags: review- → review?(martijn.martijn)
(Assignee)

Updated

3 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

3 years ago
Attachment #8644387 - Flags: review?(martijn.martijn)
Attachment #8644387 - Flags: review?(jlorenzo)
Attachment #8644387 - Flags: review-
(Assignee)

Comment 33

3 years ago
Comment on attachment 8644387 [details] [review]
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

3 years ago
Comment on attachment 8644387 [details] [review]
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

3 years ago
Attachment #8644387 - Attachment description: Make all apps have origin → Make all apps have manifestURL
Comment on attachment 8644387 [details] [review]
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

3 years ago
Attachment #8644387 - Flags: review?(martijn.martijn)
Attachment #8644387 - Flags: review?(jlorenzo)
(Reporter)

Comment 36

3 years ago
Comment on attachment 8644387 [details] [review]
Make all apps have manifestURL

I added comments in the PR.
Attachment #8644387 - Flags: review?(martijn.martijn) → review-
(Assignee)

Comment 37

3 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

3 years ago
(In reply to Martijn Wargers [:mwargers] (QA) from comment #36)
> Comment on attachment 8644387 [details] [review]
> 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

3 years ago
Attachment #8644387 - Flags: review- → review?(martijn.martijn)
(Reporter)

Comment 39

3 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

3 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] [review]
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

3 years ago
Attachment #8644387 - Flags: review?(martijn.martijn) → review-
(Assignee)

Updated

3 years ago
Attachment #8644387 - Flags: review- → review?(jlorenzo)
(Reporter)

Updated

3 years ago
Attachment #8644387 - Flags: review?(martijn.martijn)
(Reporter)

Comment 42

3 years ago
Comment on attachment 8644387 [details] [review]
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] [review]
Make all apps have manifestURL

Agreed with Martijn.
Attachment #8644387 - Flags: review?(jlorenzo)
(Assignee)

Comment 44

3 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

3 years ago
Attachment #8644387 - Flags: review?(martijn.martijn)
Attachment #8644387 - Flags: review?(jlorenzo)
(Reporter)

Comment 45

3 years ago
Comment on attachment 8644387 [details] [review]
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] [review]
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

3 years ago
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #46)
> Comment on attachment 8644387 [details] [review]
> 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

3 years ago
Comment on attachment 8644387 [details] [review]
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

3 years ago
Attachment #8644387 - Flags: review?(martijn.martijn)
(Reporter)

Comment 49

3 years ago
Created attachment 8657635 [details] [diff] [review]
mermiall.diff

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

3 years ago
Attachment #8644387 - Flags: review?(martijn.martijn)
Created attachment 8658411 [details] [review]
[gaia] mermi:bug1188926 > mozilla-b2g:master
(Assignee)

Comment 51

3 years ago
Comment on attachment 8644387 [details] [review]
Make all apps have manifestURL

>https://github.com/mozilla-b2g/gaia/pull/31747
Attachment #8644387 - Flags: review?(jlorenzo) → review-
(Assignee)

Updated

3 years ago
Attachment #8644387 - Flags: review?(martijn.martijn)
Attachment #8644387 - Flags: review?(jlorenzo)
Attachment #8644387 - Flags: review-
(Assignee)

Updated

3 years ago
Attachment #8644387 - Flags: review?(martijn.martijn)
Attachment #8644387 - Flags: review?(jlorenzo)
Attachment #8644387 - Flags: review-
(Assignee)

Updated

3 years ago
Attachment #8658411 - Flags: review?(martijn.martijn)
Attachment #8658411 - Flags: review?(jlorenzo)
(Assignee)

Updated

3 years ago
Attachment #8644387 - Attachment is obsolete: true
(Reporter)

Comment 52

3 years ago
Comment on attachment 8658411 [details] [review]
[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] [review]
[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

3 years ago
Comment on attachment 8658411 [details] [review]
[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] [review]
[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

3 years ago
Comment on attachment 8658411 [details] [review]
[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

3 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
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Reporter)

Comment 59

3 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

3 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
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 → ---
Created attachment 8660205 [details] [review]
[gaia] mermi:bug1188926c61 > mozilla-b2g:master
Created attachment 8660216 [details] [review]
[gaia] mermi:bug1188926 > mozilla-b2g:master
(Assignee)

Comment 64

3 years ago
Created attachment 8660217 [details] [review]
PR-ManifestURL
Flags: needinfo?(manel.rhaiem92)
Attachment #8660217 - Flags: review?(martijn.martijn)
(Assignee)

Updated

3 years ago
Attachment #8660205 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8660216 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8660217 - Attachment description: hope-final PR → PR-ManifestURL
(Reporter)

Comment 65

3 years ago
Comment on attachment 8660217 [details] [review]
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+
Created attachment 8660300 [details] [review]
[gaia] mermi:bug1188926 > mozilla-b2g:master
(Reporter)

Updated

3 years ago
Attachment #8660300 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8657635 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8658411 - Attachment is obsolete: true
(Reporter)

Comment 67

3 years ago
Comment on attachment 8660217 [details] [review]
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

3 years ago
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED

Comment 68

3 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.

Updated

3 years ago
Blocks: 1203060

Updated

3 years ago
No longer blocks: 1203060

Updated

3 years ago
Blocks: 1203060
(Reporter)

Comment 69

3 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

3 years ago
No longer blocks: 1203060
Depends on: 1203060
(Reporter)

Comment 70

3 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.
(Reporter)

Updated

3 years ago
No longer depends on: 1203060
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1192628
(Reporter)

Updated

3 years ago
Blocks: 1211325
You need to log in before you can comment on or make changes to this bug.