Bug 948472 - Test for Bug 938219: "add to an existing contact" activity #16835
Conversation
def tap_header(self): | ||
self.wait_for_element_displayed(*self._message_header_locator) | ||
self.marionette.find_element(*self._message_header_locator).tap() | ||
|
||
from gaiatest.apps.system.regions.activities import Activities | ||
return Activities(self.marionette, switch_frame=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of confusing but there's already another Activities region at gaiatest.apps.messages.regions.activities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove that class and the confusion and use the main system activities class
@zacc ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it and I thought it was a bit confusing to have one System class that's not actually operating in the system so I think we should reuse the region in messages/region/activities.py
LGTM now, the test runs nice on device :) |
@@ -143,12 +143,13 @@ def __init__(self, marionette): | |||
update = self.wait_for_element_present(*self._update_locator) | |||
self.wait_for_condition(lambda m: update.location['y'] == 0) | |||
|
|||
def tap_update(self): | |||
def tap_update(self, switch_to_contact_details=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that in this class we already have a method that has return_details
. Let's make them the same, for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zacc which method? the only other place we return details is in the tap_cancel
method but that's not used in our test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry my mistake, it's in the parent to this (Contacts class): tap(return_details=False)
I don't like this because switch_to
reminds me of switch to frame, but this doesn't do that. so let's use return_details
which is more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I ran the test locally and it failed once with the following error:
I was not watching the device when the error occurred so I'm not sure what happened. Maybe start an adhoc run with the test and see if it is reproduced. |
Updated |
def tap_header(self): | ||
self.wait_for_element_displayed(*self._message_header_locator) | ||
self.marionette.find_element(*self._message_header_locator).tap() | ||
|
||
from gaiatest.apps.messages.regions.activities import Activities | ||
return Activities(self.marionette) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think tap_call
below this should be moved into the activities region if you make this change. but funcitonally it's OK without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be done in a different task
we don't use this method in my test and moving it would affect other tests
lgtm |
@@ -35,10 +35,21 @@ def received_messages(self): | |||
def all_messages(self): | |||
return [Message(self.marionette, message) for message in self.marionette.find_elements(*self._all_messages_locator)] | |||
|
|||
def wait_for_message_header_visible(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we bothering to define a method for this that is only used once in header_text
? Why not just put self.wait_for_element_displayed(*self._message_header_locator)
as the first line in header_text()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point done
The test works well. Just a few comments in the PR. |
def test_sms_add_number_to_existing_contact(self): | ||
|
||
# open the message thread screen | ||
self.message_thread = self.messages.tap_first_received_message() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the first message in the list, it can be either the first received message, or the first sent one. We should rename this method to tap_first_message().
But, as it is used in some other test too(https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/messages/test_sms_to_dialer.py#L30), maybe it is better to leave it as it is for now.
Besides the extra space, lgtm |
Test that fails on Travis is not related to this pull |
Bug 948472 - Test for Bug 938219: "add to an existing contact" activity
No description provided.