821579 - B2G 3G: Add Marionette test for Data Connection

Status: RESOLVED FIXED

Description

[Shian-Yow Wu [:swu]]

This bug is created for adding data connection test cases on Marionette.

Comment 1

[Jessica Jong [:jessica] (inactive)]

Created attachment 798449 [details] [diff] [review]
data-tests-for-marionette-2.patch

I have added somes marionette tests for data connection.
However, when I tested locally, I do get "ScriptTimeoutException: timed out" sometimes (like 2 out of 10 times), and the reason is:

I/Gecko   (   45): MARIONETTE TEST RESULT:TEST-PASS | test_data_call.js | set 'ril.data.enabled' to true - true was true, expected true
I/Gecko   (   45): -*- RadioInterface[0]: 'ril.data.enabled' is now true
I/Gecko   (   45): -*- RadioInterface[0]: We haven't gotten completely the APN data.

I haven't found out why we didn't get "ril.data.apnSettings" correctly, but should we set "ril.data.apnSettings" manually (I mean in test_data_call.js) before the test to make sure it runs properly?

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

I think not.  I can find following line in Marionette log:

  I/Gecko   (   45): -*- RadioInterface[0]: 'ril.data.apnSettings' is now [[{"carrier":"T-Mobile US",
  "apn":"epc.tmobile.com","mmsc":"http://mms.msg.eng.t-mobile.com/mms/wapenc",
  "types":["default","supl","mms"]}]]

Comment 3

[Jessica Jong [:jessica] (inactive)]

Yes, it only happens randomly. As discussed, I will try to test and see if it's a race condition or not.

This patch is causing a strange error in the try server: https://tbpl.mozilla.org/?tree=Try&rev=c56762b37032
So I will need to check it again, thank you.

Comment 4

[Jessica Jong [:jessica] (inactive)]

Created attachment 799245 [details] [diff] [review]
data-tests-for-marionette-4.patch

The APN issue didn't look like a race condition, I let the test waited for 5 mins and still empty ril.data.apnSettings.
In this patch, I set the ril.data.apnSettings before the test and it works fine...

This one passes on try server:
https://tbpl.mozilla.org/?tree=Try&rev=fdf3f89a8841

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 799245 [details] [diff] [review]
data-tests-for-marionette-4.patch

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

Besides the comments below, I'd really like to sale task based test script layout[1].  It helps for distinguishing test cases from utility functions and gives you a clear trace of the progress.

[1]: https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_filter_mixed.js#28

::: dom/system/gonk/tests/marionette/manifest.ini
@@ +6,5 @@
>  [test_geolocation.js]
>  disabled = Bug 808783
>  [test_fakevolume.js]
>  [test_ril_code_quality.py]
> +[test_data_call.js]

Please move this test script to dom/network/tests/marionette/test_mobile_data_connection.js

::: dom/system/gonk/tests/marionette/test_data_call.js
@@ +9,5 @@
> +
> +let gSettingsEnabled = SpecialPowers.getBoolPref("dom.mozSettings.enabled");
> +if (!gSettingsEnabled) {
> +  SpecialPowers.setBoolPref("dom.mozSettings.enabled", true);
> +}

You can just have |SpecialPowers.setBoolPref(key, value)| and call |SpecialPowers.clearUserPref(key)| at the end.

@@ +27,5 @@
> +  log("Starting mobileConnection.data.state is: '"
> +      + connection.data.state + "'.");
> +  if (connection.data.state != "registered" ||
> +      connection.voice.state != "registered") {
> +    runEmulatorCmd("gsm voice home", function(result) {

Please have a utility function for this.  See test_filter_mixed.js[1].  Or this case may, not very often, result in an intermittent failure like bug 903058.

[1]: https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_filter_mixed.js#16

@@ +30,5 @@
> +      connection.voice.state != "registered") {
> +    runEmulatorCmd("gsm voice home", function(result) {
> +      is(result[0], "OK");
> +    });
> +    runEmulatorCmd("gsm data home", function(result) {

runEmulatorCmd(..., function () {
  ...
  runEmulatorCmd(..., function () {
    ...
  });
});

Actually, you don't have to squeeze everything into a |verifyInitialState| function.

@@ +36,5 @@
> +    });
> +  }
> +  // Want to start test with data off,
> +  // This is the default state; if it is not currently this value then set it.
> +  let getReq = settings.createLock().get(DATA_KEY);

Could you make two utility functions, one for setting Settings and another for retrieving?

@@ +57,5 @@
> +    }
> +  });
> +
> +  // To make sure test can run properly.
> +  setEmulatorAPN();

Please define |setEmualtorAPN| or other utility functions before calling them.

@@ +63,5 @@
> +  waitFor(testEnableData, function() {
> +    return (connection.voice.state == "registered" &&
> +            connection.data.state == "registered" &&
> +            connection.data.connected == false &&
> +            _apnReady == true);

listen to on{voice,data}statechanged instead.

@@ +84,5 @@
> +    cleanUp();
> +  });
> +
> +  waitFor(testDisableDataRoamingWhileRoaming, function() {
> +    return (connection.data.connected == true);

ditto.  We can't verify network state by transmitting an packet out, so the two state changed events are the only things we can ensure.

@@ +106,5 @@
> +  });
> +
> +  // Wait for roaming state to change, then data connection should
> +  // be disconnected due to data roaming set to off.
> +  setEmulatorRoaming(true, function() {

Please define setEmulatorRoaming earlier.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Created attachment 800628 [details] [diff] [review]
data-tests-for-marionette-6.patch

Address comments in Comment 5.

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

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 800628 [details] [diff] [review]
data-tests-for-marionette-6.patch

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

Good job! Finally! We have test cases for data connections! r=me with some nits addressed.

::: dom/network/tests/marionette/test_mobile_data_connection.js
@@ +26,5 @@
> +    --pendingEmulatorCmdCount;
> +
> +    is(result[0], "OK", "Emulator response");
> +
> +    if (callback) callback();

nit: we always braces even there is only one line in between.

@@ +71,5 @@
> +
> +  let setReq = setLock.set(obj);
> +  setReq.addEventListener("success", function onSetSuccess() {
> +    ok(true, "set '" + key + "' to " + obj[key]);
> +    if (callback) callback();

ditto.

@@ +75,5 @@
> +    if (callback) callback();
> +  });
> +  setReq.addEventListener("error", function onSetError() {
> +    ok(false, "cannot set '" + key + "'");
> +    cleanUp();

tasks.finish();

@@ +90,5 @@
> +    callback(value);
> +  });
> +  getReq.addEventListener("error", function onGetError() {
> +    ok(false, "cannot get '" + key + "'");
> +    cleanUp();

ditto.

@@ +128,5 @@
> +          connection.removeEventListener("datachange", ondatachange);
> +          log("mobileConnection.data.roaming is now '"
> +            + connection.data.roaming + "'.");
> +          is(connection.data.roaming, roaming, "data.roaming");
> +          if (callback) callback();

ditto

@@ +210,5 @@
> +  log("Turn data on.");
> +
> +  setSetting(DATA_KEY, true);
> +
> +  connection.addEventListener("datachange", function ondatachange() {

Add event listener first.

@@ +284,5 @@
> +  });
> +});
> +
> +
> +tasks.push(function cleanUp() {

nit: please keep the warning line.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Created attachment 801465 [details] [diff] [review]
data-tests-for-marionette-7.patch

Thank you!
This patch addresses nits in Comment 7:

- use braces even with one-line statement
- call tasks.finish() instead of cleanUp() directly
- add connection listener before action
- keep warning line before last task

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

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Remove bug 821578 from prerequisite list, test cases here don't touch the different APN names issue.

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 11

[Ed Morley (Away 18th Dec-1st Jan) [:emorley]]

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