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

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: rwood, Assigned: rwood)

Tracking

unspecified
mozilla20
All
Gonk (Firefox OS)
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox19 fixed, firefox20 fixed, b2g18 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
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
Assignee: nobody → rwood
Flags: needinfo?(philipp)
Ping Philipp
(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!
Flags: needinfo?(philipp)
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.
Attachment #689904 - Flags: review?(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.
Attachment #689904 - Flags: review?(jgriffin) → review+
Thanks Jonathan! Updated patch attached - I just made that one change, to get rid of the global variable.
Attachment #689904 - Attachment is obsolete: true
Attachment #690916 - Flags: review?(jgriffin)
Comment on attachment 690916 [details] [diff] [review]
Updated MobileConnection.data WebAPI tests

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

Thanks Rob.
Attachment #690916 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dffd2d825c9
Whiteboard: [automation-needed-in-aurora][automation-needed-in-beta]
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.
Whiteboard: [automation-needed-in-aurora][automation-needed-in-beta]
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...
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.
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).
Attachment #690916 - Attachment is obsolete: true
Attachment #691932 - Flags: review?(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);
});
Attachment #691932 - Flags: review?(jgriffin) → review+
Thanks Jonathan! I tried out your suggestion above, and yes that works fine, not sure why my original implementation didn't work.
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e70b718903a
Whiteboard: [automation-needed-in-aurora][automation-needed-in-b2g18]
Ah yes, makes complete sense... (I should have realized that), thanks Jonathan!
https://hg.mozilla.org/mozilla-central/rev/2e70b718903a
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.