Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1188926 - Make all the apps have an manifestURL as well as a name and check for that #31259

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@mermi
Copy link
Contributor

commented Aug 5, 2015

No description provided.

@@ -12,6 +12,7 @@
class Contacts(Base):

name = "Contacts"
origin_app_name = 'communications/contacts'

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 5, 2015

Contributor

I don't think this is going to work. I think this should be origin_app_name = 'communications'

@@ -10,6 +10,7 @@
class Phone(Base):

name = "Phone"
origin_app_name = 'communications/dialer'

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 5, 2015

Contributor

I don't think this is going to work. I think this should be origin_app_name = 'communications'

@mwargers

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2015

edit: this was obsolete

@@ -10,6 +10,7 @@
class Search(Base):

name = 'Browser'
origin_app_name = 'search'

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 6, 2015

Contributor

I think this is still 'browser', actually.
However, this stuff doesn't seem to be used at all, currently.

Edit: oh, perhaps this is correct.

@@ -32,7 +33,7 @@ def launch(self):
*self._settings_button_locator))))

def switch_to_contacts_frame(self):
Wait(self.marionette).until(lambda m: self.apps.displayed_app.name == self.name)
Wait(self.marionette).until(lambda m: self.apps.displayed_app.origin_app_name == self.origin_app_name)

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 6, 2015

Contributor

No, you should use .origin here (and everywhere else).
So like this:
Wait(self.marionette).until(lambda m: self.apps.displayed_app.origin == self.origin)

Wait(self.marionette).until(
lambda m: self.apps.displayed_app.name == 'Contacts')
lambda m: self.apps.displayed_app.region == Contacts.region)

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 6, 2015

Contributor

Uh, why are you using .region here? You need to use .origin.

This comment has been minimized.

Copy link
@mermi

mermi Aug 7, 2015

Author Contributor

oh, sorry I don't know why I wrote it wrong :/ I'll fix it now

@mermi mermi changed the title Bug 1188926 - Make all the apps have an origin as well as a name and check for that Bug 1188926 - Make all the apps have an manifestURL as well as a name and check for that Aug 25, 2015

@@ -373,12 +373,14 @@ var GaiaApps = {
}

let origin = app.origin;
console.log('app with origin \'' + origin + '\' is displayed');
let manifest_url = app.manifest_url;
console.log('app with origin and manifest url \'' + origin + manifest_url + '\' is displayed');

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 25, 2015

Contributor

I think I would remove the origin part here.
And I would remove the variable declarations of origin here and just use app.origin and app.manifest_url directly.

@property
def origin(self):
app_name_to_use = self.origin_app_name if self.origin_app_name else self.name.lower()
return 'app://{}.gaiamobile.org'.format(app_name_to_use)

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 25, 2015

Contributor

I don't think we need origin anymore.

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 26, 2015

Contributor

Like I said, I think you should remove this, because it's not used.

This comment has been minimized.

Copy link
@mermi

mermi Aug 26, 2015

Author Contributor

Yea, I missed to remove it

let result = {
frame: (app.browser) ? app.browser.element : app.frame.firstChild,
src: (app.browser) ? app.browser.element.src : app.iframe.src,
name: app.name,
origin: origin
origin: app.manifest_url,

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 26, 2015

Contributor

This should be app.origin

@@ -19,8 +19,11 @@ def __init__(self, marionette):
self.apps = GaiaApps(self.marionette)
self.accessibility = Accessibility(self.marionette)
self.frame = None
self.manifest_url = hasattr(self, 'manifest_url') and self.manifest_url or None

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 26, 2015

Contributor

Why are you removing this? Isn't this necessary?

This comment has been minimized.

Copy link
@mermi

mermi Aug 26, 2015

Author Contributor

Still wait for @JohanLorenzo review for this file to be sure, what I did is right or need to add something for that I comment it, but probably it will be removed we need to define manifest_url as null at the first that's I guess

This comment has been minimized.

Copy link
@JohanLorenzo

JohanLorenzo Aug 26, 2015

Contributor

That's the only occurrence of this variable in the whole patch. I don't see a reason of keeping it :)

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 26, 2015

Contributor

In def launch it's being referenced, no?

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 26, 2015

Contributor

If you really thin this can go, then I guess this line can be removed too?
self.entry_point = hasattr(self, 'entry_point') and self.entry_point or None

This comment has been minimized.

Copy link
@mermi

mermi Aug 30, 2015

Author Contributor

I don't think @mwargers because we use it on the launch function :
def launch(self, launch_timeout=None):
self.app = self.apps.launch(self.name, self.manifest_url, self.entry_point, launch_timeout=launch_timeout)

@@ -38,7 +38,7 @@ def test_call_log_all_calls(self):
self.phone.make_call_and_hang_up(test_phone_number)

# Wait for fall back to phone app
self.wait_for_condition(lambda m: self.apps.displayed_app.name == self.phone.name)
self.wait_for_condition(lambda m: self.apps.displayed_app.manifest_url == self.phone.manifest_url)

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 26, 2015

Contributor

you can use self.phone.wait_to_be_displayed() here.

@@ -40,7 +40,8 @@ def tap_back_button(self):
header.tap(x=20)

# wait for the frame to close
Wait(self.marionette).until(lambda m: self.apps.displayed_app.name != 'Gallery')
from gaiatest.apps.Gallery.app import Gallery

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 26, 2015

Contributor

You don't need this line, I think.

@@ -32,7 +32,7 @@ def launch(self):
*self._settings_button_locator))))

def switch_to_contacts_frame(self):
Wait(self.marionette).until(lambda m: self.apps.displayed_app.name == self.name)
self.wait_to_be_displayed()
self.apps.switch_to_displayed_app()

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 26, 2015

Contributor

The contacts app does have app://communications.gaiamobile.org/manifest.webapp as origin, no?
In that case, the name doesn't fit here, so you need to add a manifest_url property.
Same goes for the phone app.

This comment has been minimized.

Copy link
@mermi

mermi Aug 26, 2015

Author Contributor

if I'll add the manifest_url property, it would be the same for both Phone and Contacts have the same manifest_url and origin: manifestURL: "app://communications.gaiamobile.org…", origin: "app://communications.gaiamobile.org"
For that we need to search for another solution and it's using the entry_point but for I have n idea, also I think entry_point is useless if they think to ride it :/

This comment has been minimized.

Copy link
@mermi

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 28, 2015

Contributor

I would just ignore the entry_point part for now. From what I understood from the other bug, they want to separate Phone and Contacts anyway and get rid of entry_point all together, I think.

@@ -91,7 +91,8 @@ def launch(self):
expected.element_present(*self._bluetooth_l10n_locator))

def switch_to_settings_app(self):
Wait(self.marionette).until(lambda m: self.apps.displayed_app.name == self.name)
Wait(self.marionette).until(

This comment has been minimized.

Copy link
@JohanLorenzo

JohanLorenzo Aug 27, 2015

Contributor

Nit: self.wait_to_be_displayed()

@@ -59,8 +59,8 @@ def login(self, user, password):
self.tap_submit()

# Go back to displayed Contacts app before waiting for the picker

This comment has been minimized.

Copy link
@JohanLorenzo

JohanLorenzo Aug 27, 2015

Contributor

This comment doesn't add any value to the line below. We can remove it

@@ -87,7 +87,7 @@ def tap_select_button(self):
self.tap_element_from_system_app(select)

# Fall back to app beneath the picker
Wait(self.marionette).until(lambda m: self.apps.displayed_app.name != self.name)
self.apps.wait_to_not_be_displayed

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 28, 2015

Contributor

No, this should be self.apps.wait_to_not_be_displayed()

This comment has been minimized.

Copy link
@mermi

mermi Aug 30, 2015

Author Contributor

it's to not be, so it should be to_be @mwargers ?

This comment has been minimized.

Copy link
@mwargers

mwargers Sep 1, 2015

Contributor

No, you just need to add "()" to the method.

@@ -40,7 +39,7 @@ def tap_back_button(self):
header.tap(x=20)

# wait for the frame to close
Wait(self.marionette).until(lambda m: self.apps.displayed_app.name != 'Gallery')
slef.apps.wait_to_not_be_displayed()

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 28, 2015

Contributor

slef?
This needs to become Gallery(self.marionette).wait_to_not_be_displayed()
You need to import Gallery app too then.

This comment has been minimized.

Copy link
@mermi

mermi Aug 30, 2015

Author Contributor

oh I wrote it Wrong, okay will change this

This comment has been minimized.

Copy link
@mwargers

mwargers Sep 4, 2015

Contributor

You still need to change this. And I think this needs to be Gallery(self.marionette).wait_to_not_be_displayed(), actually.

self.tap_element_from_system_app(
self.marionette.find_elements(*self._stock_wallpapers_locator)[index])
Wait(self.marionette).until(lambda m: self.apps.displayed_app.name != self.name)
self.marionette.find_elements(*self._stock_wallpapers_locator)[index].tap()

This comment has been minimized.

Copy link
@mwargers

mwargers Aug 28, 2015

Contributor

The tap_element_from_system_app is necessary, because tapping on the button closes the app, which potentially cause failures. I don't know why you are changing this here, but you need to change it back.

This comment has been minimized.

Copy link
@mermi

mermi Aug 30, 2015

Author Contributor

YEa I changed it in the wrong way I think, It should be like this:
self.tap_element_from_system_app(
self.marionette.find_elements(*self._stock_wallpapers_locator)[index])
self.apps.wait_to_not_be_displayed()

@@ -16,9 +16,8 @@ class ViewImage(Base):

def __init__(self, marionette):
Base.__init__(self, marionette)
Wait(self.marionette).until(lambda m: self.apps.displayed_app.name == 'Gallery')
self.wait_to_be_displayed()

This comment has been minimized.

Copy link
@mwargers

mwargers Sep 4, 2015

Contributor

Shouldn't this be Gallery(self.marionette).wait_to_be_displayed()? (and do the gallery import before that then, of course).

This comment has been minimized.

Copy link
@JohanLorenzo

JohanLorenzo Sep 4, 2015

Contributor

We're already in the constructor of Gallery. Doing Gallery(self.marionette).wait_to_be_displayed() will provoke a endless loop. self.wait_to_be_displayed() is absolutely fine.

This comment has been minimized.

Copy link
@mwargers

mwargers Sep 4, 2015

Contributor

Ok, how does that work then? I don't understand.
I see mentions of view_image here: http://mxr.mozilla.org/gaia/search?string=view_image
Nothing seems related to Gallery.
But I guess because this file is in a subdirectory of galllery, it somehow is the constructor of Gallery?

This comment has been minimized.

Copy link
@JohanLorenzo

JohanLorenzo Sep 7, 2015

Contributor

Oh! I misread the class we're in. You're absolutely right, Martijn!

@@ -91,7 +91,7 @@ def launch(self):
expected.element_present(*self._bluetooth_l10n_locator))

def switch_to_settings_app(self):
Wait(self.marionette).until(lambda m: self.apps.displayed_app.name == self.name)
self.apps.wait_to_be_displayed()

This comment has been minimized.

Copy link
@mwargers

mwargers Sep 4, 2015

Contributor

This needs to be self.wait_to_be_displayed()

@@ -19,5 +19,5 @@ def tap_wallpaper_by_index(self, index):
message='%d wallpaper(s) not present after timeout' % (index + 1))
self.tap_element_from_system_app(
self.marionette.find_elements(*self._stock_wallpapers_locator)[index])
Wait(self.marionette).until(lambda m: self.apps.displayed_app.name != self.name)
self.apps.wait_to_not_be_displayed()

This comment has been minimized.

Copy link
@mwargers

mwargers Sep 4, 2015

Contributor

This needs to be self.wait_to_be_displayed()

@@ -87,7 +87,7 @@ def tap_select_button(self):
self.tap_element_from_system_app(select)

# Fall back to app beneath the picker
Wait(self.marionette).until(lambda m: self.apps.displayed_app.name != self.name)
self.apps.wait_to_not_be_displayed()

This comment has been minimized.

Copy link
@mwargers

mwargers Sep 4, 2015

Contributor

This needs to be self.wait_to_not_be_displayed()

self.entry_point = hasattr(self, 'entry_point') and self.entry_point or None
self.DEFAULT_APP_HOSTNAME = 'gaiamobile.org'

This comment has been minimized.

Copy link
@JohanLorenzo

JohanLorenzo Sep 4, 2015

Contributor

Please remove self.DEFAULT_APP_HOSTNAME = 'gaiamobile.org' and self.DEFAULT_PROTOCOL = 'app://'. We don't need them.

@mermi mermi closed this Sep 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.