Closed
Bug 919414
Opened 11 years ago
Closed 11 years ago
Refactor telephony test: Clear existing calls in setup stage
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: aknow, Assigned: aknow)
Details
Attachments
(2 files, 6 obsolete files)
3.48 KB,
patch
|
Details | Diff | Splinter Review | |
92.02 KB,
patch
|
Details | Diff | Splinter Review |
Using the feature of 'head.js' of marionette test case. We could move the code block of clearing calls to head.js and execute them in setup stage. We don't have to repeat it in every cases.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #816476 -
Flags: review?(htsai)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #816477 -
Flags: review?(htsai)
Comment 3•11 years ago
|
||
Comment on attachment 816476 [details] [diff] [review]
Part 1: Clear existing calls in test setup
Review of attachment 816476 [details] [diff] [review]:
-----------------------------------------------------------------
The overall architecture with using Promise looks good, but we should avoid redundant code.
::: dom/telephony/test/marionette/head.js
@@ +89,5 @@
> function startTest(test) {
> + function permissionSetUp() {
> + SpecialPowers.setBoolPref("dom.mozSettings.enabled", true);
> + SpecialPowers.setBoolPref("dom.promise.enabled", true);
> + SpecialPowers.addPermission("telephony", true, document);
In some tests like test_outgoing_radio_off.js, we need more permissions, such as mobileconnection and settings-write. We would definitely improve permissionSetup() and permissionTearDown() if we'd like to have a central control.
@@ +108,2 @@
> // Extend finish() with tear down.
> finish = (function() {
The extended finish() covers something we already have in cleanUp() in every test, such as removePermission(). I am fine to move common parts into this finish(), but let's be sure to not do redundant things among them.
@@ +122,5 @@
> }());
>
> // Start the test.
> + setUp()
> + .then(function onSuccess() {
nit: indention
Attachment #816476 -
Flags: review?(htsai)
Comment 4•11 years ago
|
||
Comment on attachment 816477 [details] [diff] [review]
Part 2: Modify tests
Review of attachment 816477 [details] [diff] [review]:
-----------------------------------------------------------------
General concerns for every test:
1) We have "let telephony = window.navigator.mozTelephony" in head.js. We should remove the code from each test.
2) We have 'permissionSetUp() and 'permissionTearDown()' in head.js. We should remove corresponding code from each test, if the enhanced 'permissionSetUp' and 'permissionTearDown' could handle that.
::: dom/telephony/test/marionette/test_conference.js
@@ +683,3 @@
>
> function cleanUp() {
> SpecialPowers.removePermission("telephony", document);
We have 'permissionTearDown()' in head.js. Do we still need this?
::: dom/telephony/test/marionette/test_crash_emulator.js
@@ +5,3 @@
> MARIONETTE_HEAD_JS = 'head.js';
>
> SpecialPowers.addPermission("telephony", true, document);
We have 'permissionSetUp()' in head.js. Do we still need this?
@@ +5,5 @@
> MARIONETTE_HEAD_JS = 'head.js';
>
> SpecialPowers.addPermission("telephony", true, document);
>
> let telephony = window.navigator.mozTelephony;
And this?
Attachment #816477 -
Flags: review?(htsai)
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> Comment on attachment 816476 [details] [diff] [review]
> Part 1: Clear existing calls in test setup
>
> Review of attachment 816476 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> The overall architecture with using Promise looks good, but we should avoid
> redundant code.
>
> ::: dom/telephony/test/marionette/head.js
> @@ +89,5 @@
> > function startTest(test) {
> > + function permissionSetUp() {
> > + SpecialPowers.setBoolPref("dom.mozSettings.enabled", true);
> > + SpecialPowers.setBoolPref("dom.promise.enabled", true);
> > + SpecialPowers.addPermission("telephony", true, document);
>
> In some tests like test_outgoing_radio_off.js, we need more permissions,
> such as mobileconnection and settings-write. We would definitely improve
> permissionSetup() and permissionTearDown() if we'd like to have a central
> control.
>
> @@ +108,2 @@
> > // Extend finish() with tear down.
> > finish = (function() {
>
> The extended finish() covers something we already have in cleanUp() in every
> test, such as removePermission(). I am fine to move common parts into this
> finish(), but let's be sure to not do redundant things among them.
>
The permission setup and teardown here is used to add/remove the permission needed in head.js (ex: clearCalls). I don't think that the duplication in the head.js and each test case is a problem. haad.js is like a test suite, so we are doing test suite scope setup/teardown here. And in the test cases, they could has their own setup/teardown.
> @@ +122,5 @@
> > }());
> >
> > // Start the test.
> > + setUp()
> > + .then(function onSuccess() {
>
> nit: indention
It will be better to align '.then' to the beginning.
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> Comment on attachment 816477 [details] [diff] [review]
> Part 2: Modify tests
>
> Review of attachment 816477 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> General concerns for every test:
> 1) We have "let telephony = window.navigator.mozTelephony" in head.js. We
> should remove the code from each test.
> 2) We have 'permissionSetUp() and 'permissionTearDown()' in head.js. We
> should remove corresponding code from each test, if the enhanced
> 'permissionSetUp' and 'permissionTearDown' could handle that.
>
> ::: dom/telephony/test/marionette/test_conference.js
> @@ +683,3 @@
> >
> > function cleanUp() {
> > SpecialPowers.removePermission("telephony", document);
>
> We have 'permissionTearDown()' in head.js. Do we still need this?
>
> ::: dom/telephony/test/marionette/test_crash_emulator.js
> @@ +5,3 @@
> > MARIONETTE_HEAD_JS = 'head.js';
> >
> > SpecialPowers.addPermission("telephony", true, document);
>
> We have 'permissionSetUp()' in head.js. Do we still need this?
>
> @@ +5,5 @@
> > MARIONETTE_HEAD_JS = 'head.js';
> >
> > SpecialPowers.addPermission("telephony", true, document);
> >
> > let telephony = window.navigator.mozTelephony;
>
> And this?
Agree to remove the line of
1. let telephony ...
2. telephony permission add/remove
How could we enhanced the permission setup? One method is to provide a list of permissions we need to startTest():
ex:
startTest(permissionList, function() {
// test here.
});
or just provide another help function
setPermission(['telephony', 'mobileconnection', ...]);
startTest(...);
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #816476 -
Attachment is obsolete: true
Attachment #821592 -
Flags: review?(htsai)
Assignee | ||
Comment 8•11 years ago
|
||
Comments addressed.
Found a drawback. See test_outgoing_radio_off.js
If we handle the permission through the test macro 'startTest()'. We have to make sure all the mozXXX objects are retrieved after adding the permission. So the code should be write as follows. connection is initialized in test macro.
let connection;
...
startTestWithPermissions(permissions, function() {
connection = navigator.mozMobileConnection;
icc = navigator.mozIccManager;
...
});
Attachment #816477 -
Attachment is obsolete: true
Attachment #821597 -
Flags: review?(htsai)
Comment 9•11 years ago
|
||
Comment on attachment 821592 [details] [diff] [review]
Part 1#2: Clear existing calls in test setup
Review of attachment 821592 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/telephony/test/marionette/head.js
@@ +107,5 @@
> + log('== Test SetUp ==');
> + permissionSetUp();
> + // Make sure that we get the telephony after adding permission.
> + telephony = window.navigator.mozTelephony;
> + return clearCalls().then(checkInitialState);
Kinda tricky. We need to check |ok(telephony)| before clearCalls() as |telephony.calls| would be called there.
Except this, the patch looks good to me.
@@ +117,5 @@
>
> function tearDown() {
> log('== Test TearDown ==');
> emulator.waitFinish()
> + .then(permissionTearDown)
Indention, please.
@@ +128,5 @@
> }());
>
> + function mainTest() {
> + setUp()
> + .then(function onSuccess() {
Indention, please.
Attachment #821592 -
Flags: review?(htsai)
Assignee | ||
Comment 10•11 years ago
|
||
Comments addressed.
Attachment #821592 -
Attachment is obsolete: true
Attachment #822242 -
Flags: review?(htsai)
Comment 11•11 years ago
|
||
Comment on attachment 821597 [details] [diff] [review]
Part 2#2: Modify tests
Review of attachment 821597 [details] [diff] [review]:
-----------------------------------------------------------------
It generally looks good though minor changes needed. Thank you.
::: dom/telephony/test/marionette/test_conference.js
@@ +685,2 @@
> startTest(function() {
> + conference = telephony.conferenceGroup;
ok(conference);
::: dom/telephony/test/marionette/test_outgoing_emergency_in_airplane_mode.js
@@ +10,3 @@
> let number = "112";
> let outgoing;
> +let skipSetup = true;
The variable isn't referenced. Remove it.
@@ +100,5 @@
> finish();
> }
> +
> +startTestWithPermissions(['settings-write'], function() {
> + settings = window.navigator.mozSettings;
ok(settings)
@@ +101,5 @@
> }
> +
> +startTestWithPermissions(['settings-write'], function() {
> + settings = window.navigator.mozSettings;
> + setAirplaneMode();
There seems some chance that we call dial() before setReq.onsuccess/setReq.onerror is fired. We shall be more careful here.
Attachment #821597 -
Flags: review?(htsai)
Comment 12•11 years ago
|
||
Comment on attachment 822242 [details] [diff] [review]
Part 1#3: Clear existing calls in test setup
Review of attachment 822242 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you :)
Attachment #822242 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #821597 -
Attachment is obsolete: true
Attachment #823126 -
Flags: review?(htsai)
Comment 14•11 years ago
|
||
Comment on attachment 823126 [details] [diff] [review]
Part 2#3: Modify tests
Review of attachment 823126 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the comment addressed/explained. Thank you. :)
::: dom/telephony/test/marionette/test_outgoing_emergency_in_airplane_mode.js
@@ +28,3 @@
> });
> setReq.addEventListener("error", function onSetError() {
> ok(false, "cannot set '" + KEY + "'");
Don't we need to have |deferred.reject()| and with |cleanUp| being the reject callback?
Attachment #823126 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #822242 -
Attachment is obsolete: true
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #823126 -
Attachment is obsolete: true
Assignee | ||
Comment 17•11 years ago
|
||
Keywords: checkin-needed
Comment 18•11 years ago
|
||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 19•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8ee956154703
https://hg.mozilla.org/mozilla-central/rev/6ac79ac36ee3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•