Oliver Nelson [:oliverthor] Reporter
	Description • 9 years ago

Description:
test_settings_bluetooth.py is failing consistently today, 7/20 via local automation runs. The case appears to be setting it's visibility to to all to 'off' directly after renaming the device; this causes it to hang and timeout.

Repro Steps:
gaiatest --testvars=gaiatest/testvars_smoke.json --address=localhost:2828 --timeout=30000 --repeat=9 --restart --html-output=feb18_bluetooth2.html gaiatest/tests/functional/settings/test_settings_bluetooth.py

Traceback (most recent call last):
  File "/home/flash/Desktop/oliverthor/git/gaia/tests/python/gaia-ui-tests/.env/local/lib/python2.7/site-packages/marionette_client-0.8.7-py2.7.egg/marionette/marionette_test.py", line 283, in run
    testMethod()
  File "/home/flash/Desktop/oliverthor/git/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/settings/test_settings_bluetooth.py", line 37, in test_toggle_bluetooth_settings
    bluetooth_settings.enable_visible_to_all()
  File "/home/flash/Desktop/oliverthor/git/gaia/tests/python/gaia-ui-tests/gaiatest/apps/settings/regions/bluetooth.py", line 54, in enable_visible_to_all
    Wait(self.marionette).until(expected.element_selected(checkbox))
  File "/home/flash/Desktop/oliverthor/git/gaia/tests/python/gaia-ui-tests/.env/local/lib/python2.7/site-packages/marionette_client-0.8.7-py2.7.egg/marionette/wait.py", line 143, in until
    cause=last_exc)
TimeoutException: TimeoutException: Timed out after 30.0 seconds

Environmental Variables:
Device firmware (base)	L1TC100118D0
Device firmware (date)	18 Feb 2015 01:45:09
Device firmware (incremental)	eng.cltbld.20150218.044458
Device firmware (release)	4.4.2
Device identifier	flame
Gaia date	17 Feb 2015 15:32:21
Gaia revision	82f286f10a41
Gecko build	20150218010226
Gecko revision	9696d1c4b3ba
Gecko version	38.0a1

Reproducible manually: No

Repro frequency: 7/20

	Oliver Nelson [:oliverthor] Reporter
	Comment 1 • 9 years ago

Build Failure:
http://jenkins1.qa.scl3.mozilla.com/job/flame-kk-319.mozilla-central.nightly.ui.functional.smoke/37/

	Martijn Wargers (dead)
	Comment 2 • 9 years ago

I changed this file recently in bug 1118219.
I've run this locally 11 times and it fails with this failure 2 times.
I noticed when this failure happens, the "Visible to all" checkbox is getting checked almost immediately after the Bluetooth checkbox is checked.
So when enable_visible_to_all() is called, the "Visible to all" checkbox is getting unchecked again, because that function does manually tap on it, which then causes it to get unchecked.

I don't see how that can be a bug in the test. When bluetooth is enabled, bluetooth visibility shouldn't suddenly be enabled too.

	Martijn Wargers (dead)
	Comment 3 • 9 years ago

I wonder about this code:
http://mxr.mozilla.org/gaia/source/apps/bluetooth/js/settings.js#173
When lock().get fails, the "Visible to all" checkbox is checked, why is that? Although I can't see in what situation a lock().get() would fail.

	Jamin Liu [:jaliu][:Jamin]
	Comment 4 • 9 years ago

(In reply to Martijn Wargers [:mwargers] (QA) from comment #2)
> I changed this file recently in bug 1118219.
> I've run this locally 11 times and it fails with this failure 2 times.
> I noticed when this failure happens, the "Visible to all" checkbox is
> getting checked almost immediately after the Bluetooth checkbox is checked.
> So when enable_visible_to_all() is called, the "Visible to all" checkbox is
> getting unchecked again, because that function does manually tap on it,
> which then causes it to get unchecked.
> 
> I don't see how that can be a bug in the test. When bluetooth is enabled,
> bluetooth visibility shouldn't suddenly be enabled too.

Hi Martijn,
Thank you for the information.
It seems like it hit Bug 1098228.

Hi Ian,
Would you mind to take a look and check if this bug is caused by Bug 1098228 ?
Thanks.

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07
	Comment 5 • 9 years ago

This started failing frequently with http://jenkins1.qa.scl3.mozilla.com/view/Mozilla%20Lab/job/flame-kk-319.mozilla-central.tinderbox.ui.functional.smoke/366/ , Feb 13, 2015 11:21:49 AM (PST)

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07
	Comment 6 • 9 years ago

Just checked back in other jobs, and failure rate is around 50%. That's noisy enough to disable while investigating, which I'm doing in bug 1141124.

	Martijn Wargers (dead)
	Comment 7 • 9 years ago

(In reply to Martijn Wargers [:mwargers] (QA) from comment #3)
> I wonder about this code:
> http://mxr.mozilla.org/gaia/source/apps/bluetooth/js/settings.js#173
> When lock().get fails, the "Visible to all" checkbox is checked, why is
> that? Although I can't see in what situation a lock().get() would fail.

Jamin, do you know the answer to this?

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07
	Comment 9 • 9 years ago

Comment on attachment 8574699 [details] [review]
[gaia] geoelectric:disable-1134244 > mozilla-b2g:master

This was placed in here in error. Will amend to go to 1141124 as intended.

	Jamin Liu [:jaliu][:Jamin]
	Comment 10 • 9 years ago

(In reply to Martijn Wargers [:mwargers] (QA) from comment #7)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #3)
> > I wonder about this code:
> > http://mxr.mozilla.org/gaia/source/apps/bluetooth/js/settings.js#173
> > When lock().get fails, the "Visible to all" checkbox is checked, why is
> > that? Although I can't see in what situation a lock().get() would fail.
> 
> Jamin, do you know the answer to this?

Ian,
Do you know why we set "visibleCheckBox.checked = true" when lock().get fails ?

> // get last user setting for device visible
> var req = settings.createLock().get('bluetooth.visible');
> ...
> req.onerror = function bt_getVisibleError() {
>   visibleCheckBox.checked = true;
> };

	iliu@mozilla.com, ianliu.moz@gmail.com
	Comment 11 • 9 years ago

(In reply to Jamin Liu [:jaliu] from comment #4)
> Hi Ian,
> Would you mind to take a look and check if this bug is caused by Bug 1098228
> ?
> Thanks.
The problem here we hit, is absolutely same with bug 1098228. I have mentioned in link(https://bugzilla.mozilla.org/show_bug.cgi?id=1098228#c6).

	iliu@mozilla.com, ianliu.moz@gmail.com
	Comment 12 • 9 years ago

(In reply to Jamin Liu [:jaliu] from comment #10)
> Ian,
> Do you know why we set "visibleCheckBox.checked = true" when lock().get
> fails ?
> 
> > // get last user setting for device visible
> > var req = settings.createLock().get('bluetooth.visible');
> > ...
> > req.onerror = function bt_getVisibleError() {
> >   visibleCheckBox.checked = true;
> > };

I trace the code twice. I still can not understand why given 'true' state to the visible check box while the request fall into onerror case. And this line is not the root cause for disable visible immediately while Bluetooth is from OFF -> ON.

This is an old code base from v1.0.(https://github.com/mozilla-b2g/gaia/commit/4ad40bcedfac4bcc29f1ffd54f0c9547d243e44e).

Only one thing I can understand that the patch wants to get user's preference 'bluetooth.visible'. Then, use the preference to restore discoverable state. For initial state, it uses platform discoverable state to init the visible check box. 


Most of the case is as following:

STR:
0) Enable Bluetooth and set visible to be true.
1) Disable Bluetooth.
2) Enable Bluetooth.

Actual result:
Will see the visible state from enabled to disabled.

The root cause is the user's preference not sync with platform value in this use case. So we can see the visible checked box be changed from checked to unchecked.

How we can do for the problem.
1). Grey out the state until we set the user's preference to platform with onsuccess(). This is risk if platform always set discoverable enabled in failed case.
2). Let the default value of visible to be false. And no longer to restore state for user's preference.

I would like solution 2) because I agree with comment 2. Shall we fix the problem immediately? My concern is having new user story in bug 1121913.

New feature: visible timer (Settings app) - https://bugzilla.mozilla.org/show_bug.cgi?id=1121913

	iliu@mozilla.com, ianliu.moz@gmail.com
	Comment 13 • 9 years ago

Per comment 12, ni Howie for decision.

	Martijn Wargers (dead)
	Comment 14 • 9 years ago

Note that this test, [test_settings_bluetooth.py, is currently not running in daily automated testing, so if things regress even further, it won't get noticed (at least, not quickly).

	howie [:howie]
	Comment 15 • 9 years ago

not high priority, and bug 1121913 will solve this

	Martijn Wargers (dead)
	Comment 16 • 9 years ago

Reopening, this bug is different than bug 1121913. I know comment 15 is saying that bug 1121913 is fixing this, but I don't see any movement in that bug.

	No-Jun Park [:njpark] Assignee
	Comment 18 • 9 years ago

I made changes to the bluetooth.py.  Basically, when visible_to_all is enabled, I did not see any changes to detect on webIDE.  The switch element is hidden, and doesn't show whether it's checked or not, so I am checking the bluetooth visibility from the data_layer to confirm the changes.

Also had to add disable_bluetooth() methods and change the locator for the Bluetooth switch.

	No-Jun Park [:njpark] Assignee
	Comment 19 • 9 years ago

BTW Comment 2 does not seem to be reproducing with the latest build

	Martijn Wargers (dead)
	Comment 20 • 9 years ago

(In reply to No-Jun Park [:njpark] from comment #19)
> BTW Comment 2 does not seem to be reproducing with the latest build

I guess you repeated the test 10 times?

	Martijn Wargers (dead)
	Comment 21 • 9 years ago

Comment on attachment 8655666 [details] [review]
[gaia] npark-mozilla:1134244 > mozilla-b2g:master

I think you also need to check for the checkboxes to ensure that they are checked/unchecked when tapping on them. I don't think you need to do this Wait(self.marionette).until(lambda m: data_layer.bluetooth_is_discoverable is True) thing you're doing.
If you think this test is then passing afterwards and running stable (after a lot of repeats, you should remove the stable = false rule from the manifest.ini file).

	No-Jun Park [:njpark] Assignee
	Comment 22 • 9 years ago

Comment on attachment 8655666 [details] [review]
[gaia] npark-mozilla:1134244 > mozilla-b2g:master

inserted the check for gaia-switch now

	Martijn Wargers (dead)
	Comment 23 • 9 years ago

Comment on attachment 8655666 [details] [review]
[gaia] npark-mozilla:1134244 > mozilla-b2g:master

Thanks for looking into this! The patch looks great.
I repeated the test 10 times and it passed every time, so this seems indeed stable again.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #16)
> Reopening, this bug is different than bug 1121913. I know comment 15 is
> saying that bug 1121913 is fixing this, but I don't see any movement in that
> bug.

I guess that with the move to bluetooth_v2, this test got stable again.
Anyway, it doesn't matter, it's important that we get this test running again.

	Johan Lorenzo [:jlorenzo]
	Comment 24 • 9 years ago

Comment on attachment 8655666 [details] [review]
[gaia] npark-mozilla:1134244 > mozilla-b2g:master

LGTM, modulo the unused function, disable_bluetooth().

I made a comment about the usage of PageRegion. That's fine if we don't implement it in this PR.

	No-Jun Park [:njpark] Assignee
	Comment 25 • 9 years ago

Unused function is removed, and PageRegion is used.  Ran 10 times with no failures.


Merged:
https://github.com/mozilla-b2g/gaia/commit/ac07d7fc6b36c5852d58f4e1bcbfca9ac664b2d3