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

RESOLVED FIXED

Status

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

People

(Reporter: Hermes Cheng (inactive after July 27, 2015), Assigned: shinglyu)

Tracking

unspecified
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(2 attachments)

1.04 MB, text/html
Details
46 bytes, text/x-github-pull-request
Martijn Wargers (dead)
: review+
Hermes Cheng (inactive after July 27, 2015)
: review+
Details | Review | Splinter Review
When performed "test_sms_notification.py", 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 | test_sms_notification.py 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/marionette_test.py", line 268, in run
    testMethod()
  File "/home/hermesc/workspace/test-space/gaia-2.2/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/messages/test_sms_notification.py", 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?

https://github.com/mozilla-b2g/gaia/blob/v2.2/tests/python/gaia-ui-tests/gaiatest/apps/system/app.py
status-b2g-v2.2: --- → affected
status-b2g-master: --- → unaffected
Flags: needinfo?(slyu)
Created attachment 8585299 [details]
non-smoketest report.html

Please see the error in "test_sms_notification.TestSmsNotification"
(Assignee)

Comment 3

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

Comment 4

3 years ago
Created attachment 8585836 [details] [review]
[gaia] shinglyu:bug1149024 > mozilla-b2g:v2.2
(Assignee)

Updated

3 years ago
Attachment #8585836 - Flags: review?(hcheng)
(Assignee)

Comment 5

3 years ago
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+
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 9

3 years ago
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)
(Assignee)

Comment 12

3 years ago
@Martijn: Thanks for your effort. I'll keep an eye on this until it landed.
Flags: needinfo?(slyu)

Updated

3 years ago
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)
(Assignee)

Comment 14

3 years ago
I've rebased the branch, rerunning the tests now. Thanks!
Flags: needinfo?(slyu)
(Assignee)

Comment 15

3 years ago
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: https://github.com/mozilla-b2g/gaia/commit/a69f8eb5c3815289afac45d2a55d868547b17f00
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)
status-b2g-v2.2: affected → fixed
(Assignee)

Comment 18

3 years ago
(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.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.