1240997 - Enable 3 Bluetooth marionette tests(BluetoothManager/Adapter_enable/Adapter_setters) on B2G emulator-kk

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect, P2)

Description

[Will Wang [:WillWang]]

To enable following 3 solved marionette tests, we better to test them for at least 100 times (as bug 1175389 comment 39 did) first.

1. [test_dom_BluetoothManager.js]
2. [test_dom_BluetoothAdapter_enable.js]
3. [test_dom_BluetoothAdapter_setters.js]

Comment 1

[Will Wang [:WillWang]]

Attachment: mnw-log-0120.log

As bug 1175389 comment 3 mentioned, above 3 tests are PASS then.
However today I encountered assertion fails[1] caused by patches[2] after then, and now these fails are solved.

Both test Adapter_enable/Adapter_setters will encounter this issue, which was caught by my local examination.

Now changing the assertion check from empty string "" to "00:00:00:00:00:00" string will solve this issue. [3]

I am going to send the patch review request and perform a 100 times test.

====

[1] Log is as attached. Brief:

 24 TEST-START | test_dom_BluetoothAdapter_setters.js
 25 TEST-UNEXPECTED-FAIL | test_dom_BluetoothAdapter_setters.js | adapter.address ==== thispoint - got 00:00:00:00:00:00, expected
 26 TEST-UNEXPECTED-FAIL | test_dom_BluetoothAdapter_setters.js | AssertionError: 1 tests failed

[2] Please refer to bug 1207649 and bug 1220121.

[3] 
 28 TEST-PASS | test_dom_BluetoothManager.js | took 1339ms
 36 TEST-PASS | test_dom_BluetoothAdapter_enable.js | took 8353ms
 61 TEST-PASS | test_dom_BluetoothAdapter_setters.js | took 8461ms

 97 SUMMARY
 98 -------
 99 passed: 3
100 failed: 0

Comment 2

[Will Wang [:WillWang]]

Hi Ben,

Per discussion, could you suggest which is the better solution for now?
1. modify marionette test, as above comment did.
2. modify gecko: keep gecko return empty string.

Thanks!

Hi Jocelyn, 

Could you suggest for this as well? 
Thanks for your opinion!

Comment 3

[Ben Tian (inactive)]

Attachment: empty-addr.patch

(In reply to Will Wang [:WillWang] from comment #2)
> Hi Ben,
> 
> Per discussion, could you suggest which is the better solution for now?
> 1. modify marionette test, as above comment did.
> 2. modify gecko: keep gecko return empty string.

I prefer option 2 to match documentation on wiki [1][2].

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#address
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#address

Will, can you try whether attached patch fixes the marionette test problem? The patch resets address to empty string if it's 00:00:00:00:00:00.

Comment 4

[Ben Tian (inactive)]

I opened dependent bug 1241382 to solve comment 2 problem.

Comment 5

[Ben Tian (inactive)]

Comment on attachment 8710244 [details] [diff] [review]
empty-addr.patch

Move patch to bug 1241382.

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Will Wang [:WillWang] from comment #2)
> Hi Ben,
> 
> Per discussion, could you suggest which is the better solution for now?
> 1. modify marionette test, as above comment did.
> 2. modify gecko: keep gecko return empty string.
> 
> Thanks!
> 
> Hi Jocelyn, 
> 
> Could you suggest for this as well? 
> Thanks for your opinion!

Sorry for my late response, and I would prefer option2.

To me, it is about the default value we defined in our API design, and both default values seem feasible.
But since it is already documented, and application developers might already use it in their logic, I prefer not to change it and respect the default value definition in gecko if there are no strong reasons that we should use "00:00:00..." rather than empty strings.

Thanks,
Jocelyn

Comment 7

[Will Wang [:WillWang]]

Hi Ben, Jocelyn,

Thanks for your opinion!
As bug 1241382 comment 2, the patch works and I will provide mine for test enablement,
thanks!

Comment 8

[Will Wang [:WillWang]]

Attachment: Log: test 100 times for option 1

Just for reference:

Here is the test result of performing 100 tests for a patch I made for option 1(modify marionette test) last day:

100 tests all passed.

Comment 9

[Will Wang [:WillWang]]

Attachment: 0001-Bug-1240997-Enable-3-Bluetooth-marionette-tests-on-B.patch

Hi Ben,

Could you help to review this patch?
Thanks!

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8710872 [details] [diff] [review]
0001-Bug-1240997-Enable-3-Bluetooth-marionette-tests-on-B.patch

Review of attachment 8710872 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nit addressed.

::: dom/bluetooth/tests/marionette/manifest.ini
@@ +10,1 @@
>  

nit: remove extra newline.

@@ +12,1 @@
>  

Ditto.

@@ +14,1 @@
>  

Ditto.

Comment 11

[Will Wang [:WillWang]]

Attachment: 0001-Bug-1240997-Enable-3-Bluetooth-marionette-tests-on-B.patch

Revise and carry r+ from previous patch, and add reviewer/convert to HG patch.

This patch can be sent to try server and checkin after bug 1241382 is checkin-ed.

Comment 12

[Will Wang [:WillWang]]

Pushed to try server, waiting for result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5db64dcd2976

Comment 13

[Will Wang [:WillWang]]

(In reply to Will Wang [:WillWang] from comment #12)
> Pushed to try server, waiting for result:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5db64dcd2976

Result is fine. 
For better reliability, I pushed another try test which enables all Unit Test Suites:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b522d1c88afb

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/f7d3592dc0bd

Comment 15

[Ben Tian (inactive)]

Backed out due to perma orange on ICS emulator: https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=f7d3592dc0bd

https://hg.mozilla.org/integration/b2g-inbound/rev/4cb80fcf0be8

Comment 16

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

http://archive.mozilla.org/pub/b2g/tinderbox-builds/b2g-inbound-emulator/1453803748/b2g-inbound_ubuntu64_vm-b2g-emulator_test-marionette-webapi-bm114-tests1-linux64-build91.txt.gz

Comment 17

[Will Wang [:WillWang]]

Attachment: Patch(v2): Enable 3 Bluetooth marionette tests on B2G emulator-kk

Here is the revised patch which skips the marionette test if android version[1] is less than 19 [2] to avoid the inevitable[3] test fail in emulator-ICS.

And the try result for this patch is good[4]. (I especially retrigger some tests for more reliability)

Hi Ben,

Could you help to review this patch?
Thanks!


[1]
http://developer.android.com/about/dashboards/index.html

[2]
Syntax in manifest.ini for reference:
skip-if = android_version < '19'

[3]
compatibility of BT API v2 and underlying Bluez service

[4]
https://treeherder.allizom.org/#/jobs?repo=try&revision=1d042b6b120a

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/cc44f581675f

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cc44f581675f