Closed Bug 932723 Opened 6 years ago Closed 6 years ago

Add a test to verify that received SMS are notified to the user

Categories

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

defect
Not set

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed)

RESOLVED FIXED
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: rik, Assigned: viorela)

References

Details

Attachments

(1 file, 6 obsolete files)

46 bytes, text/x-github-pull-request
zcampbell
: review+
bsilverberg
: review-
Bebe
: review+
julienw
: feedback+
Details | Review
We had a regression in bug 930296 that was not caught by any existing text.

When we receive a text message, a notification message should appear.
Component: Gaia::SMS → Gaia::UI Tests
We have a similar test already; test_sms.py. I think we can try to modify the existing test to take in this case. 

In the existing test after the SMS is sent, tap the 'Home' button to hide the messages app, switch to System frame and wait for the notification. 

Then tap the notification which will open the Messages app and resume the final assertion in the test.
I can work on this one
Assignee: nobody → viorelaioia
pointer to pull request https://github.com/mozilla-b2g/gaia/pull/13290
Attachment #825890 - Flags: review?(zcampbell)
Attachment #825890 - Flags: review?(trifandreialin)
Attachment #825890 - Flags: review?(zcampbell)
Attachment #825890 - Flags: review?(trifandreialin)
Sorry Viorela, the test case for this is not correct.

It needs to be a separate test case. I will write the new test case in this bug.
This test needs to make sure the Messages app is not running which rules out modifying the existing test case.

Thus we need to use the MozSMS Api to send the message to the device rather than using Messages app.

This should be a brand new test case:

1. Start Firefox OS
2. Lock the screen
3. Turn the screen off
4. Use MozSMS API/atom to send a message
5. Wait for message received
6. Assert that screen turns on
7. Assert that notification is on the lockscreen

You will need to add methods and properties into the GaiaDevice class to enable the screen on and off.
You can use the ScreenManager: 
window.wrappedJSObject.ScreenManager.turnScreenOn();
Assignee: viorelaioia → nobody
Depends on: 934402
I have done some testing with
window.ScreenManager.turnScreenOff();

and fortunately we can still send Marionette commands while the screen is off!

This test is no longer blocked so somebody feel free to write it :)
No longer depends on: 934402
Assignee: nobody → bob.silverberg
Oops, maybe Viorela would like to take the new version of this. Would you?
Assignee: bob.silverberg → nobody
Flags: needinfo?(viorela.ioia)
Yes, I will work on this test :)
Flags: needinfo?(viorela.ioia)
Assignee: nobody → viorela.ioia
This is still dependent on bug 934402, which has been fixed on master and awaiting an uplift to v1.2.
Depends on: 934402
pointer to Github pull request https://github.com/mozilla-b2g/gaia/pull/13701
Attachment #832188 - Flags: review?(zcampbell)
Attachment #832188 - Flags: review?(trifandreialin)
Attachment #832188 - Flags: review?(moz.teodosia)
Attachment #832188 - Flags: review?(florin.strugariu)
Attachment #832188 - Flags: review?(andrei.hutusoru)
Attachment #832188 - Flags: review?(florin.strugariu) → review-
Attached file updated the PR (obsolete) —
Attachment #832260 - Flags: review?(zcampbell)
Attachment #832260 - Flags: review?(florin.strugariu)
Attachment #832260 - Flags: review?(bob.silverberg)
Comment on attachment 832260 [details] [review]
updated the PR

Comments in the PR.
Attachment #832260 - Flags: review?(bob.silverberg) → review-
Attachment #825890 - Attachment is obsolete: true
Attachment #832188 - Attachment is obsolete: true
Attachment #832188 - Flags: review?(zcampbell)
Attachment #832188 - Flags: review?(trifandreialin)
Attachment #832188 - Flags: review?(moz.teodosia)
Attachment #832188 - Flags: review?(andrei.hutusoru)
Attached file updated the pull request (obsolete) —
pointer to Github PR https://github.com/mozilla-b2g/gaia/pull/13701
Attachment #8333781 - Flags: review?(zcampbell)
Attachment #8333781 - Flags: review?(florin.strugariu)
Attachment #8333781 - Flags: review?(bob.silverberg)
Comment on attachment 832260 [details] [review]
updated the PR

>https://github.com/mozilla-b2g/gaia/pull/13701
Attachment #832260 - Attachment is obsolete: true
Attachment #832260 - Flags: review?(zcampbell)
Attachment #832260 - Flags: review?(florin.strugariu)
Attachment #8333781 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8333781 [details] [review]
updated the pull request

The screen is not being turned off. Full comments in the PR.
Attachment #8333781 - Flags: review?(bob.silverberg) → review-
Attachment #8333781 - Flags: review+ → review-
Attached file updated PR (obsolete) —
pointer to Gihtub PR: https://github.com/mozilla-b2g/gaia/pull/13701
Attachment #8335175 - Flags: review?(zcampbell)
Attachment #8335175 - Flags: review?(trifandreialin)
Attachment #8335175 - Flags: review?(florin.strugariu)
Attachment #8335175 - Flags: review?(bob.silverberg)
Attachment #8335175 - Flags: review?(andrei.hutusoru)
Attachment #8335175 - Flags: review?(andrei.hutusoru) → review+
Attachment #8335175 - Flags: review?(trifandreialin) → review+
Comment on attachment 8335175 [details] [review]
updated PR

merge conflicts again
Attachment #8335175 - Flags: review+ → review-
Comment on attachment 8335175 [details] [review]
updated PR

still js wait issues
Attachment #8335175 - Flags: review?(florin.strugariu) → review-
Comment on attachment 8335175 [details] [review]
updated PR

Intermittent failures that should be addressed.
Attachment #8335175 - Flags: review?(bob.silverberg) → review-
Attached file updated PR (obsolete) —
pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/13701
Attachment #8333781 - Attachment is obsolete: true
Attachment #8335175 - Attachment is obsolete: true
Attachment #8333781 - Flags: review?(zcampbell)
Attachment #8335175 - Flags: review?(zcampbell)
Attachment #8335319 - Flags: review?(zcampbell)
Attachment #8335319 - Flags: review?(florin.strugariu)
Attachment #8335319 - Flags: review?(dave.hunt)
Attachment #8335319 - Flags: review?(bob.silverberg)
Attachment #8335319 - Flags: review?(zcampbell)
Attachment #8335319 - Flags: review?(florin.strugariu)
Attachment #8335319 - Flags: review?(dave.hunt)
Attachment #8335319 - Flags: review?(bob.silverberg)
Attachment #8335319 - Flags: review-
Attached file updated PR
Attachment #8336015 - Flags: review?(zcampbell)
Attachment #8336015 - Flags: review?(florin.strugariu)
Attachment #8336015 - Flags: review?(dave.hunt)
Attachment #8336015 - Flags: review?(bob.silverberg)
Comment on attachment 8336015 [details] [review]
updated PR

Still intermittent failures :(

See the PR
Attachment #8336015 - Flags: review?(bob.silverberg) → review-
Attachment #8335319 - Attachment is obsolete: true
Comment on attachment 8336015 [details] [review]
updated PR

Just one nit, otherwise I think this looks great. Please note that I have not run the test myself.
Attachment #8336015 - Flags: review?(dave.hunt) → review-
Attachment #8336015 - Flags: review- → review?
Attachment #8336015 - Flags: review- → review?
Comment on attachment 8336015 [details] [review]
updated PR

r- but only two very small nits. We can merge this soon :)
Attachment #8336015 - Flags: review?(zcampbell) → review-
Attachment #8336015 - Flags: review- → review?
Comment on attachment 8336015 [details] [review]
updated PR

as Zac said still seeing issues
Let's wait for a dev response
Attachment #8336015 - Flags: review?(florin.strugariu) → review-
Attachment #8336015 - Flags: review?(zcampbell)
Attachment #8336015 - Flags: review?(dave.hunt)
Attachment #8336015 - Flags: review?(bob.silverberg)
Attachment #8336015 - Flags: review?
Attachment #8336015 - Flags: review-
Attachment #8336015 - Flags: review?(dave.hunt) → review-
Attachment #8336015 - Flags: review- → review?
Comment on attachment 8336015 [details] [review]
updated PR

Still seeing an intermittent failure. :(
Attachment #8336015 - Flags: review?(bob.silverberg) → review-
Rik, we've tried to get this to work using an incoming SMS but it causes the lockscreen to refresh for some reason and that causes the notification to disappear before we can validate it reliably.

It doesn't seem to occur when we just send an SMS manually.

If you don't have a clue why this is occurring (see the above attachment for the test code), would the test case be valid if we switched to just forcing a notification through the API?

This could probably be run on Travis/TBPL, too.

self.marionette.execute_script('navigator.mozNotification.createNotification("%s", "%s").show();'
                                       % (self._notification_title, self._notification_body))
Flags: needinfo?(anthony)
I'll defer to someone working on SMS. Julien, see questions from comment 27.
Flags: needinfo?(anthony) → needinfo?(felash)
The original bug was about receiving system messages so we definitely need a real SMS (and several ones even).

You don't need to turn screen off for this test, you don't even need to be locked (but it's may be easier to check the notification than in the notification panel? or maybe not ;) ).

So you can try sending a SMS to an unlocked phone and check the notification in the notification panel. Just make sure the SMS app is not launched.
Flags: needinfo?(felash)
We definitely can do that! 
Perhaps the generic notification/wake lock test is still a good test case, but done in a separate test (Viorela, preserve that code snippet, we may still use it).

We can potentially send several SMS but I'd rather not as it increases the dependency on external service which are occasionally slow to respond.

Viorela, can you update the test case here?

1. Send an SMS to the phone
2. Check that a notifications is shown (see test_system_notification_bar for examples)
3. Validate that SMS app has not been launched by searching for the iframe in the DOM
Let me correct myself:
* we need only one SMS (I checked this after writing the first sentence, sorry)
* we need to check the SMS app is not launched _before_ sending the SMS. It will probably be launched after receiving the SMS.
What might cause the SMS app to launch *before* sending the SMS?

We're sending the SMS using the SMS API rather than the app (because Marionette has the powers) and this is as close as we can get to a SMS just coming in from another device.
(In reply to Zac C (:zac) from comment #32)
> What might cause the SMS app to launch *before* sending the SMS?

if the device is at a clean state before then it should be fine. I just want to make sure that it reproduces the bug correctly.

That said, you can do 2 tests: one with the app closed, one with the app launched in the background, both are interessant and real use cases.
We already have one test that checks that a system message launches the app in the background, I think that one is more interessant ;) because it can run on all 3 CI environments. It is 'cheaper' in several ways.

We can do another one for SMS as it's quite an important user-facing event.
(In reply to Zac C (:zac) from comment #30)
> We definitely can do that! 
> Perhaps the generic notification/wake lock test is still a good test case,
> but done in a separate test (Viorela, preserve that code snippet, we may
> still use it).
> 
> We can potentially send several SMS but I'd rather not as it increases the
> dependency on external service which are occasionally slow to respond.
> 
> Viorela, can you update the test case here?
> 
> 1. Send an SMS to the phone
> 2. Check that a notifications is shown (see test_system_notification_bar for
> examples)
> 3. Validate that SMS app has not been launched by searching for the iframe
> in the DOM

Zac, I updated the test. I just have one incertitude, related to the 3rd step of the test case. Once I send the SMS, the Messages app starts running in the background. So if I check for the Messages frame not present, I'll always get an errorr. Instead, If I check for the Messages not displayed, then I'll get a pass. 
The validation would be: self.assertFalse(self.is_element_displayed(*self._messages_frame_locator)).
Is this check ok? Or it is not what you meant?
I'm confused too. In c#31 julien said it will "probably be launched" but I'm not clear if that is regarded as a passing or failing test case.
Attachment #8336015 - Flags: review?(florin.strugariu)
Attachment #8336015 - Flags: review?(felash)
Attachment #8336015 - Flags: review?
I said "probably" because it will be launched for sure but I don't remember if it's really closed. I think it's not, I can check if you want, but I don't think this is meaningful to add this to the test since it does not impact the user-faced behavior.
Comment on attachment 8336015 [details] [review]
updated PR

looks good
(won't give a r+ because I don't know the framework well enough)
Attachment #8336015 - Flags: review?(felash) → feedback+
I think this is OK


 SUMMARY
 -------
 passed: 51
 failed: 0
 todo: 0
Comment on attachment 8336015 [details] [review]
updated PR

Small tweak to checking that the frame is loaded but not showing.
Attachment #8336015 - Flags: review?(zcampbell) → review-
Attachment #8336015 - Flags: review- → review?(zcampbell)
1.3+ for uplift
blocking-b2g: --- → 1.3+
Comment on attachment 8336015 [details] [review]
updated PR

Let's refine to use the new method that was created!
Attachment #8336015 - Flags: review?(zcampbell) → review-
Attachment #8336015 - Flags: review- → review?(zcampbell)
Comment on attachment 8336015 [details] [review]
updated PR

r+
Attachment #8336015 - Flags: review?(zcampbell) → review+
Merged!
https://github.com/mozilla-b2g/gaia/commit/0ef52554acf8cc115e40a95b2eab0117df3dff45
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Uplifted 0ef52554acf8cc115e40a95b2eab0117df3dff45 to:
v1.3: 9b63ffa668df9535a7e93677c861cc6b71b21ab2
You need to log in before you can comment on or make changes to this bug.