Closed
Bug 821579
Opened 8 years ago
Closed 8 years ago
B2G 3G: Add Marionette test for Data Connection
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: swu, Assigned: jessica)
References
Details
Attachments
(1 file, 3 obsolete files)
10.46 KB,
patch
|
jessica
:
review+
|
Details | Diff | Splinter Review |
This bug is created for adding data connection test cases on Marionette.
Assignee | ||
Comment 1•8 years ago
|
||
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?
Attachment #798449 -
Flags: feedback?(vyang)
Comment 2•8 years ago
|
||
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"]}]]
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Comment 4•8 years ago
|
||
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
Attachment #798449 -
Attachment is obsolete: true
Attachment #798449 -
Flags: feedback?(vyang)
Attachment #799245 -
Flags: feedback?(vyang)
Comment 5•8 years ago
|
||
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.
Attachment #799245 -
Flags: feedback?(vyang)
Assignee | ||
Comment 6•8 years ago
|
||
Address comments in Comment 5. Try results: https://tbpl.mozilla.org/?tree=Try&rev=ee7d45fdebaf
Attachment #799245 -
Attachment is obsolete: true
Attachment #800628 -
Flags: feedback?(vyang)
Comment 7•8 years ago
|
||
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.
Attachment #800628 -
Flags: feedback?(vyang) → review+
Assignee | ||
Comment 8•8 years ago
|
||
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
Assignee: nobody → jjong
Attachment #800628 -
Attachment is obsolete: true
Attachment #801465 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 9•8 years ago
|
||
Remove bug 821578 from prerequisite list, test cases here don't touch the different APN names issue.
No longer depends on: 821578
Updated•8 years ago
|
Component: General → RIL
Keywords: checkin-needed
Comment 11•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b8c3080fc0e5
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•