811580 - [WebAPI] WebMobileConnection: Develop test to verify MobileConnection.data

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Robert Wood [:rwood]]

Develop a marionette test (to run on the B2G device emulator) that will verify MobileConnection.data.  Currently a test exists to verify the default MobileConnection.data value:

http://mxr.mozilla.org/mozilla-central/source/dom/network/tests/marionette/test_mobile_networks.js

However a test is needed to switch to the various data states and verify the corresponding values - similar to this test but for data instead:

http://mxr.mozilla.org/mozilla-central/source/dom/network/tests/marionette/test_mobile_voice_state.js

This new test for MobileConnection.data should also verify the 'ondatachange' event which fires when the data state is changed.

Comment 1

[Robert Wood [:rwood]]

Hi Philipp,

I notice that the following is the current behaviour concerning the WebMobileConnection.data.state attribute and the emulator.  Could you please confirm that this is as expected? I will develop a test to verify this behaviour if it is correct.

I am using the 'gsm data <state>' emulator command to change the data.state, and am using the WebAPI mobileConnection.data.state attribute to get the corresponding value.

Emulator data state ==> corresponding mobileConnection.data.state value:

home ==> registered
unregistered ==> notsearching
roaming ==> registered
searching ==> registered
denied ==> denied
off (same as unregistered) ==> notsearching
on (same as home) ==> registered

Comment 2

[Robert Wood [:rwood]]

Ping Philipp

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to Rob Wood [:rwood] from comment #1)
> Emulator data state ==> corresponding mobileConnection.data.state value:
> 
> home ==> registered
> unregistered ==> notsearching

Should be 'notSearching' (camelCased). If not, please file a bug.

> roaming ==> registered
> searching ==> registered

Should be 'searching'. I *think* the phone gets this right, but I'm not sure. So it could just be an emulator bug. Can you please file a bug? Thanks!

Comment 4

[Robert Wood [:rwood]]

Thanks Philipp!

> Should be 'notSearching' (camelCased). If not, please file a bug.

Actually it is 'notSearching', sorry my mistake above.

>> searching ==> registered
> Should be 'searching'. I *think* the phone gets this right, but I'm not sure. So it could just be an emulator bug. Can you please file a bug? Thanks!

Created Bug 819533. Will have this test expect 'registered' for now, so it is not failing all the time on TBPL, then it will be updated once bug 819533 is fixed.

Comment 5

[Robert Wood [:rwood]]

Attachment: Marionette webAPI tests for MobileConnection.data

Comment 6

[Jonathan Griffin (:jgriffin)]

Comment on attachment 689904 [details] [diff] [review]
Marionette webAPI tests for MobileConnection.data

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

::: dom/network/tests/marionette/test_mobile_data_location.js
@@ +5,5 @@
> +
> +SpecialPowers.addPermission("mobileconnection", true, document);
> +
> +let mobileConnection = navigator.mozMobileConnection;
> +let gotCallback = false;

You should be able to define gotCallback in testChangeCellLocation, rather than as a global, since it's only used in that context.

::: dom/network/tests/marionette/test_mobile_data_state.js
@@ +77,5 @@
> +  // Bug 819533: WebMobileConnection data/voice state incorrect when emulator
> +  // data state is 'searching'. So until fixed, expect 'registered'.
> +
> +  // changeDataStateAndVerify("searching", "searching", testDenied);
> +  log("* When Bug 819533 is fixed, change this test to expect 'searching' *");

I've filed bug 820154 for a more elegant way of dealing with expected failures, but this is fine in the meantime, especially as dealing with that bug will not be a high priority.

Comment 7

[Robert Wood [:rwood]]

Attachment: Updated MobileConnection.data WebAPI tests

Thanks Jonathan! Updated patch attached - I just made that one change, to get rid of the global variable.

Comment 8

[Jonathan Griffin (:jgriffin)]

Comment on attachment 690916 [details] [diff] [review]
Updated MobileConnection.data WebAPI tests

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

Thanks Rob.

Comment 9

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dffd2d825c9

Comment 10

[Jonathan Griffin (:jgriffin)]

Backed out for bustage on TBPL:

https://tbpl.mozilla.org/php/getParsedLog.php?id=17839079&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1212b861665

Rob, does this pass locally when run in combination with all the other tests?  Perhaps we need to update the version of the emulator used by buildbot.

Comment 11

[Robert Wood [:rwood]]

Thanks for backing that out Jonathan. I reproduced the issue locally. When I run all of the /network WebAPI tests, they all pass including the two new ones. However when I run the entire WebAPI test suite, they fail with the errors above. I will run the entire WebAPI suite for now when before submitting new tests.

There must be a previous test putting the emulator in a state that causes these 2 new tests to fail, I am looking into it...

Comment 12

[Robert Wood [:rwood]]

Turns out it is a problem with using waitFor and providing parameters as part of the callback; the test wasn't actually waiting and was continuing on to the next section, causing callbacks to happen at an unexpected time resulting in the test to fail. If I take out the parameters in the waitFor callback it works fine, new patch coming soon.

Comment 13

[Robert Wood [:rwood]]

Attachment: Fixed MobileConnection.data WebAPI tests

Fixed the test by removing parameters from waitFor callback function. Tested by locally running these two tests alone as well as running them along with the entire WebAPI test suite (emulator).

Comment 14

[Jonathan Griffin (:jgriffin)]

Comment on attachment 691932 [details] [diff] [review]
Fixed MobileConnection.data WebAPI tests

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

Looks good. I'll push to try to make sure.

::: dom/network/tests/marionette/test_mobile_data_location.js
@@ +82,5 @@
> +        "data.cell.gsmLocationAreaCode");
> +    is(mobileConnection.data.cell.gsmCellId, newCid, "data.cell.gsmCellId");
> +    waitFor(restoreLocation, function() {
> +      return(gotCallback);
> +    });

This looks fine.  I'm curious whether another way to fix it would be to use (based on the previous version):

waitFor(function() {
  restoreLocation(emulatorStartLac, emulatorStartCid);
},
function() {
  return(gotCallback);
});

Comment 15

[Jonathan Griffin (:jgriffin)]

https://tbpl.mozilla.org/?tree=Try&rev=ca74de4539e6

Comment 16

[Robert Wood [:rwood]]

Thanks Jonathan! I tried out your suggestion above, and yes that works fine, not sure why my original implementation didn't work.

Comment 17

[Jonathan Griffin (:jgriffin)]

(In reply to Rob Wood [:rwood] from comment #16)
> Thanks Jonathan! I tried out your suggestion above, and yes that works fine,
> not sure why my original implementation didn't work.

When you have code like:

waitFor(restoreLocation(emulatorStartLac, emulatorStartCid), function() {
  return(gotCallback);
});

the function restoreLocation() is called immediately and the result of that is passed to waitFor.  When you have code like:

waitFor(function() {
  restoreLocation(emulatorStartLac, emulatorStartCid);
},
function() {
  return(gotCallback);
});

you're passing a function reference to waitFor, which it can call.

You can see the difference easily if you remove the waitFor:

restoreLocation(emulatorStartLac, emulatorStartCid)

You'd expect the above function to be called immediately when the interpreter got to that line.  Compare with:

function() foo {
  restoreLocation(emulatorStartLac, emulatorStartCid);
}

Here's you're defining a function, but you wouldn't expect it to be called immediately when the interpreter got to this line.

Comment 18

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e70b718903a

Comment 19

[Robert Wood [:rwood]]

Ah yes, makes complete sense... (I should have realized that), thanks Jonathan!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e70b718903a

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69ef9c2b52b9
https://hg.mozilla.org/releases/mozilla-b2g18/rev/41e791c6b3ae