Closed Bug 1149024 Opened 9 years ago Closed 9 years ago

[v2.2] wait_for_notification_toaster_displayed needs 'message' as its argument


(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Not set


(b2g-v2.2 fixed, b2g-master unaffected)

Tracking Status
b2g-v2.2 --- fixed
b2g-master --- unaffected


(Reporter: hcheng, Assigned: shinglyu)




(2 files)

When performed "", it failed when calling system.wait_for_notification_toaster_displayed(...).

The failed reason is putting a incorrect parameter "message" to wait_for_notification_toaster_displayed(...).

It is only 

* Error:
TEST-UNEXPECTED-ERROR | TestSmsNotification.test_sms_notification | TypeError: wait_for_notification_toaster_displayed() got an unexpected keyword argument 'message'

Traceback (most recent call last):
  File "/home/hermesc/workspace/test-space/gaia-2.2/gaia/2.2/local/lib/python2.7/site-packages/marionette_client-0.8.6-py2.7.egg/marionette/", line 268, in run
  File "/home/hermesc/workspace/test-space/gaia-2.2/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/messages/", line 26, in test_sms_notification
    message="Notification did not appear. SMS database dump: %s " % self.data_layer.get_all_sms())
TEST-INFO took 15861ms
Shing, you have a fixed for wait_for_notification_toaster_displayed(...) in the latest PR. Could you check it?
Flags: needinfo?(slyu)
Please see the error in "test_sms_notification.TestSmsNotification"
Oh yes, the "message" parameter was removed during the review, because it was not passed down and the reviewer thought it was useless. I'll create another PR to add it back and maybe pass it down.
Flags: needinfo?(slyu)
Attachment #8585836 - Flags: review?(hcheng)
Comment on attachment 8585836 [details] [review]
[gaia] shinglyu:bug1149024 > mozilla-b2g:v2.2

Hi Geo, th "message" argument was removed in Bug 1132479. Now other tests need it, so I added it back and passed along. Thanks.
Attachment #8585836 - Flags: review?(gmealer)
Comment on attachment 8585836 [details] [review]
[gaia] shinglyu:bug1149024 > mozilla-b2g:v2.2

It looks good. Thanks!
Attachment #8585836 - Flags: review?(hcheng) → review+
Attachment #8585836 - Flags: review?(gmealer) → review?(martijn.martijn)
The pull request from bug 1132479 caused this bug, so marking blocking that bug.
Blocks: 1132479
Summary: wait_for_notification_toaster_displayed needs 'message' as its argument → [v2.2] wait_for_notification_toaster_displayed needs 'message' as its argument
Comment on attachment 8585836 [details] [review]
[gaia] shinglyu:bug1149024 > mozilla-b2g:v2.2

I'm fine with the chang, it looks good and I assume it makes the test pass.

But I'm concerned this change with introduced in the first place. I understand the reasoning in comment 3, but then you should try to get that change back into master too.
I'd prefer to keep the most minimum difference between the tests between v2.2 and master, unless absolutely necessary.
Attachment #8585836 - Flags: review?(martijn.martijn) → review+
Thanks Martjin, I'll send another pull request on master to make the two branches in sync.

Could you help me merge the pull request? I don't have the permission for merging. Thanks!
Flags: needinfo?(martijn.martijn)
Yes, I can do that. Unfortunately, the Treeherder Try server seems to be very unstable, the results are orange. It's probably not from this pull request, but afaik, I can only merge the pull request with a succesful Tryserver run.
I have the same issue in bug 1149886.
Flags: needinfo?(jlal)
Shing, something with Treeherder try and v2.2 is very wrong, where a retrigger doesn't give new Gip results. I talked with James Lal about this, but I don't really know what needs to be done to get the v2.2 pull request in. I suggest you chat with James Lal about this.
There are more v2.2 pull requests waiting on this v2.2 Treeherder try bustage to be fixed.
Flags: needinfo?(martijn.martijn) → needinfo?(slyu)
@Martijn: Thanks for your effort. I'll keep an eye on this until it landed.
Flags: needinfo?(slyu)
Blocks: 1155747
Flags: needinfo?(jlal)
Shing, it looks like v2.2 Treeherder try is green now, so this can be checked in?
Flags: needinfo?(slyu)
I've rebased the branch, rerunning the tests now. Thanks!
Flags: needinfo?(slyu)
Looks like it's all green, can you help me check this in, Martijn? Thank you!
Flags: needinfo?(martijn.martijn)
Treeheder Try was green, checked into v2.2:
Assignee: nobody → slyu
Flags: needinfo?(martijn.martijn)
Shing, does the thing in comment 9 still need to happen (sync the code in v2.2 and master)?
Flags: needinfo?(slyu)
(In reply to Martijn Wargers [:mwargers] (QA) from comment #17)
The master branch now uses "Wait()" directly, without going through the "self.wait_for_element_displayed()" helper. I'm not sure how we are going to make them in sync.

 38 <<<<<<< HEAD                                                                     
 39     # A lot of tests, like mail or call received, need a longer timeout here     
 40     def wait_for_notification_toaster_displayed(self, timeout=30, message=None): 
 41     ¦   Wait(self.marionette, timeout).until(                                    
 42     ¦   ¦   expected.element_displayed(*self._notification_toaster_locator), message=message)
 43 =======                                                                          
 44     def wait_for_notification_toaster_displayed(self, timeout=10, message=None): 
 45     ¦   self.wait_for_element_displayed(*self._notification_toaster_locator,     
 46     ¦   ¦   ¦   ¦   ¦   ¦   ¦   ¦   ¦   timeout=timeout, message=message)        
 47 >>>>>>> bd81b49... Bug 1149024 - wait_for_notification_toaster_displayed needs 'message' as its argument
Flags: needinfo?(slyu)
Ok, just leave it as it is then, thanks.
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.