1064228 - [v2.2][v2.1] gaiatest atoms: Make settings api test functions wait on transaction

Status: RESOLVED FIXED

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

Description

[Robert Chira [:RobertC]]

Attachment: logcat.txt

In test_unlock_to_camera_with_passcode we add a passcode and enable the lockscreen via javascript, but the changes do not work as expected.
When I navigate to Settings the passcode lock option appears as being enabled, but when I unlock the device I am not prompted for a passcode.

This issue cannot be reproduced with manual testing, only with automation. The reproduction rate with automation is 100%.
Link to test: https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/lockscreen/test_lockscreen_unlock_to_camera_with_passcode.py

I tried to test the bellow build locally, but I had issues starting the phone after flashing. The regression window is done with the help of the Jenkins inbound test runs.
Last working build:
Device firmware (date) 	31 Aug 2014 13:31:23
Device firmware (incremental) 	eng.cltbld.20140831.163113
Device firmware (release) 	4.3
Device identifier 	flame
Gaia date 	04 Sep 2014 10:32:56
Gaia revision 	2bac78bdd095
Gecko build 	20140904110502
Gecko revision 	b0411338116f
Gecko version 	35.0a1

First broken:
Device firmware (date) 	31 Aug 2014 13:31:23
Device firmware (incremental) 	eng.cltbld.20140831.163113
Device firmware (release) 	4.3
Device identifier 	flame
Gaia date 	04 Sep 2014 10:32:56
Gaia revision 	2bac78bdd095
Gecko build 	20140904111729
Gecko revision 	be51c20e5b0f
Gecko version 	35.0a1

I ran the test locally step by step in debug mode. After the settings were applied I manually locked the device the tried to unlock to camera, but was not asked for a passcode (logcat attached)

Comment 1

[Jason Smith [:jsmith]]

Gaia commits are the same, so this is a gecko regression.

Comment 2

[Jason Smith [:jsmith]]

http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=b0411338116f&tochange=be51c20e5b0f

Comment 3

[Jason Smith [:jsmith]]

Broken by bug 1061510.

Kyle - Any ideas?

Comment 4

[Jason Smith [:jsmith]]

[Blocking Requested - why for this release]:

QA blocking regression from a 2.1+ blocker. Needed to be fixed to uplift bug 1061510 without regressions.

Comment 5

[Jason Smith [:jsmith]]

Actually I'll hold off on the nomination until I hear back from Kyle if this requires a test update or if it's a settings API regression.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yup, I've got an idea, that may be the reason we're seeing a lot of automation failure outside of the READ_ONLY issue.

Python tests move really, really fast through the UI. As we can see from the code in the lockscreen test, they're usually using data layer to inject settings changes. The problem is, the data layer setSettings function only waits for set to return.

https://github.com/mozilla-b2g/gaia/blob/2b3c6ae2e3d85b49e7dcfb60e7065c2df41dda61/tests/atoms/gaia_data_layer.js#L208

This used to be fine before we OOP'd settings, but now waiting on set success means we only make sure the settings API got the request to set, not that we've actually finished the transaction the set was in.

For user driven UI, this is ok because people usually can't click faster than our IPC system. For automation, however, this is race condition time because we can try to do things before the transaction to change the setting and updating of observers may've finished.

Waiting on the transaction to end, which has its own event handler, should fix this.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Make settings api test functions wait on transaction

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8485836 [details] [review]
Patch 1 (v1) - Make settings api test functions wait on transaction

Assigning multiple reviewers just cause this needs to land ASAP. First one to get to it wins.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

[Blocking Requested - why for this release]: This will cause intermittents on tests that move to quickly through UI.

Comment 10

[Robert Chira [:RobertC]]

Comment on attachment 8485836 [details] [review]
Patch 1 (v1) - Make settings api test functions wait on transaction

I ran test_unlock_to_camera_with_passcode with this patch and it still failed with the same behavior as described in comment 0

Comment 11

[Viorela Ioia [:viorela]]

Test test_power_button_long_press.py is failing on master because of a similar issue. 
The sleep menu shows different items if the volume is set to 0 or to a higher value.
If the volume is set to 0 we should expect ["Turn on airplane mode", "Ring incoming calls", "Restart", "Power off"] menu items.
If the volume is set to a value higher than 0, we should expect ["Turn on airplane mode", "Silence incoming calls", "Restart", "Power off"] menu items.
The set volume script we use to set the volume to 0 in setup is not working as expected, the volume remains set to its default value: https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L878. In order to set the volume, we use the setSetting script: https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L261.
As the default volume is higher than 0, the test fails when checking the sleep menu items. 

The regression range of this failure is the same as the one described in comment 0.  
Link to the test: https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/system/test_power_button_long_press.py

Jenkins report of the test failure: http://jenkins1.qa.scl3.mozilla.com/view/UI/job/flame.b2g-inbound.ui.functional.smoke/1338/HTML_Report/

Comment 12

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

Comment on attachment 8485836 [details] [review]
Patch 1 (v1) - Make settings api test functions wait on transaction

Clearing review request based on RobertC's findings.

Comment 13

[Florin Strugariu [:Bebe]]

Comment on attachment 8485836 [details] [review]
Patch 1 (v1) - Make settings api test functions wait on transaction

I still can reproduce the issue

Comment 14

[Florin Strugariu [:Bebe]]

Updating bug name as v2.1 is affected too

Comment 15

[Zac C (:zac)]

I'm re-reviewing this on the last good Flame build before we hit the crasher.

Comment 16

[Zac C (:zac)]

Comment on attachment 8485836 [details] [review]
Patch 1 (v1) - Make settings api test functions wait on transaction

r+, works nicely on a pre-bug 1062255 build.

Bebe, Robert, can you re-test this on a build that includes this backout (when time permits)?
https://tbpl.mozilla.org/?rev=31ce2eb10676

Comment 17

[Florin Strugariu [:Bebe]]

I tested this on latest b2g-i and it still fails 

I will wait for a build that includes that blackout

Comment 18

[Gregor Wagner [:gwagner]]

I would say just land as NPOTB. Do you guys want approval for those patches as well?

Comment 19

[bhavana bajaj [:bajaj]]

(In reply to Gregor Wagner [:gwagner] from comment #18)
> I would say just land as NPOTB. Do you guys want approval for those patches
> as well?

yes, as sometimes these NPOTB changes although non-user facing, end up breaking builds :) So seeking approval will be helpful to keep an eye on the changes going on and err on the side of caution ;)

Comment 20

[Zac C (:zac)]

There is some suspicion that this bug is a conflation of some other bugs, we'll look into it and see if we still need the patch on v2.1 and if ont we'll take it on master as an enhancement.

Comment 21

[Zac C (:zac)]

OK I've found a better way to demonstrate the problem.

What I am seeing is that when we push a preference from outside of the UI (ie, by our atom), the event is not picked up by Gaia and it acted upon. Previously this would work.

I have tested this with and without qDot's patch.

STR:
Pre-requisite: Install gaiatest, adb forward tcp:2828 tcp:2828
This STR will use gcli from the command line for ease, but gcli uses the exact same underlying settings atoms that gaiatest uses.

1. Restart your device... *do not start the Settings app*  (see workaround below)
2. At command line:  gcli getsetting language.current
> language.current: en-US
3. gcli setsetting language.current zh-TW
> 
4. gcli getsetting language.current
> language.current: zh-TW    (This verifies that the setting is set)
5. Observe the UI - it is still showing icon names in en-US
6. Load settings app, observe language setting.

The language setting in the Settings app main screen will show the setting as being Taiwanese but all of the Settings all will be in English.


*** Workaround:
1. Load the settings app in the background
2. Switch back to homescreen
3. gcli setsetting language.current zh-TW
4. Homescreen icons will change to Taiwanese localisation.

Are there some kind of Settings database listeners only applied when the Settings app is open?

The reason why I did not notice this when I reviewed the patch earlier is that I ran tests that only check that the setting is updated in the database.

qdot, ni? you to update you. It sounds like this is at a higher level but you might know who to ask.. (Settings app owners, perhaps).

Comment 23

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Nope. It's probably me. I bet it's bug 1065128. Something between booting and you running gcli is blowing away the observer list, so no observers are updated. Once bug 1065128 lands, that should clear up. Might be worth testing this on top of that one too. Adding that as a dependency.

Comment 24

[Zac C (:zac)]

Jason, based on Kyle's more recent update, can you set blocking+ status on this?

We have disabled QA smoketest automation because of it.

Comment 25

[Jason Smith [:jsmith]]

I talked with Kyle in IRC about this. My understanding is that the test blocker here is bug 1065128, which is already nominated to block. This bug is tracking the test update needed here, so this wouldn't be a blocker for being NPOTB.

Comment 26

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #25)
> I talked with Kyle in IRC about this. My understanding is that the test
> blocker here is bug 1065128, which is already nominated to block. This bug
> is tracking the test update needed here, so this wouldn't be a blocker for
> being NPOTB.

We are going to fix this before uplifting the regressing changeset, however.

Comment 27

[Zac C (:zac)]

Thanks Jason, yes I agree that this is not a blocker then. We don't need to take the patch in this bug, but we'd like to as it's an improvement.

I'll re-jig this one to be about applying qDot's patch, but the test failures are all because of bug 1065128.

Comment 28

[Robert Chira [:RobertC]]

Merged to master:
https://github.com/mozilla-b2g/gaia/commit/bba262357ac37b79874a7ba1cbddbcc811a7b8c4

Waiting for the fix from Bug 1065128 to be uplifted to v2.1 before merging there.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Bug resolution tracks landing on master. That said, you'll need to request Gaia v2.1 approval on this patch before it can land on v2.1 anyway.

Comment 30

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8485836 [details] [review]
Patch 1 (v1) - Make settings api test functions wait on transaction

Approval Request Comment
[Feature/regressing bug #]: Bug 900551
[User impact if declined]: Tests may break randomly
[Describe test coverage new/current, TBPL]: This is a test
[Risks and why]: None
[String/UUID change made/needed]: None

Comment 31

[Zac C (:zac)]

Tests re-enabled:
https://github.com/mozilla-b2g/gaia/commit/0779321e37da3843acce94acdb21a2db22bb4122

Comment 32

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/bcc0338a200b11a0f5c4285d366a280b3006012b