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

Status: RESOLVED FIXED

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

Description

[Anthony Ricaud (:rik)]

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.

Comment 1

[Zac C (:zac)]

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.

Comment 2

[Viorela Ioia [:viorela]]

I can work on this one

Comment 3

[Viorela Ioia [:viorela]]

Attachment: added test to verify received message notification

pointer to pull request https://github.com/mozilla-b2g/gaia/pull/13290

Comment 4

[Zac C (:zac)]

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.

Comment 5

[Zac C (:zac)]

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();

Comment 6

[Zac C (:zac)]

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 :)

Comment 7

[Bob Silverberg [:bsilverberg]]

Oops, maybe Viorela would like to take the new version of this. Would you?

Comment 8

[Viorela Ioia [:viorela]]

Yes, I will work on this test :)

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This is still dependent on bug 934402, which has been fixed on master and awaiting an uplift to v1.2.

Comment 10

[Viorela Ioia [:viorela]]

Attachment: added test to check for message notification

pointer to Github pull request https://github.com/mozilla-b2g/gaia/pull/13701

Comment 11

[Viorela Ioia [:viorela]]

Attachment: updated the PR

Comment 12

[Bob Silverberg [:bsilverberg]]

Comment on attachment 832260 [details] [review]
updated the PR

Comments in the PR.

Comment 13

[Viorela Ioia [:viorela]]

Attachment: updated the pull request

pointer to Github PR https://github.com/mozilla-b2g/gaia/pull/13701

Comment 14

[Florin Strugariu [:Bebe]]

Comment on attachment 832260 [details] [review]
updated the PR

>https://github.com/mozilla-b2g/gaia/pull/13701

Comment 15

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8333781 [details] [review]
updated the pull request

The screen is not being turned off. Full comments in the PR.

Comment 16

[Viorela Ioia [:viorela]]

Attachment: updated PR

pointer to Gihtub PR: https://github.com/mozilla-b2g/gaia/pull/13701

Comment 17

[Trif Andrei-Alin[:AlinT]]

Comment on attachment 8335175 [details] [review]
updated PR

merge conflicts again

Comment 18

[Florin Strugariu [:Bebe]]

Comment on attachment 8335175 [details] [review]
updated PR

still js wait issues

Comment 19

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8335175 [details] [review]
updated PR

Intermittent failures that should be addressed.

Comment 20

[Viorela Ioia [:viorela]]

Attachment: updated PR

pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/13701

Comment 21

[Viorela Ioia [:viorela]]

Attachment: updated PR

Comment 22

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8336015 [details] [review]
updated PR

Still intermittent failures :(

See the PR

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 24

[Zac C (:zac)]

Comment on attachment 8336015 [details] [review]
updated PR

r- but only two very small nits. We can merge this soon :)

Comment 25

[Florin Strugariu [:Bebe]]

Comment on attachment 8336015 [details] [review]
updated PR

as Zac said still seeing issues
Let's wait for a dev response

Comment 26

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8336015 [details] [review]
updated PR

Still seeing an intermittent failure. :(

Comment 27

[Zac C (:zac)]

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))

Comment 28

[Anthony Ricaud (:rik)]

I'll defer to someone working on SMS. Julien, see questions from comment 27.

Comment 29

[Julien Wajsberg [:julienw]]

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.

Comment 30

[Zac C (:zac)]

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

Comment 31

[Julien Wajsberg [:julienw]]

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.

Comment 32

[Zac C (:zac)]

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.

Comment 33

[Julien Wajsberg [:julienw]]

(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.

Comment 34

[Zac C (:zac)]

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.

Comment 35

[Viorela Ioia [:viorela]]

(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?

Comment 36

[Zac C (:zac)]

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.

Comment 37

[Julien Wajsberg [:julienw]]

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 38

[Julien Wajsberg [:julienw]]

Comment on attachment 8336015 [details] [review]
updated PR

looks good
(won't give a r+ because I don't know the framework well enough)

Comment 39

[Florin Strugariu [:Bebe]]

Comment on attachment 8336015 [details] [review]
updated PR

It looks OK 
But to make sure:

http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/63/console

Comment 40

[Florin Strugariu [:Bebe]]

I think this is OK


 SUMMARY
 -------
 passed: 51
 failed: 0
 todo: 0

Comment 41

[Zac C (:zac)]

Comment on attachment 8336015 [details] [review]
updated PR

Small tweak to checking that the frame is loaded but not showing.

Comment 42

[Preeti Raghunath(:Preeti)]

1.3+ for uplift

Comment 43

[Zac C (:zac)]

Comment on attachment 8336015 [details] [review]
updated PR

Let's refine to use the new method that was created!

Comment 44

[Zac C (:zac)]

Comment on attachment 8336015 [details] [review]
updated PR

r+

Comment 45

[Zac C (:zac)]

Merged!
https://github.com/mozilla-b2g/gaia/commit/0ef52554acf8cc115e40a95b2eab0117df3dff45

Comment 46

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 0ef52554acf8cc115e40a95b2eab0117df3dff45 to:
v1.3: 9b63ffa668df9535a7e93677c861cc6b71b21ab2