919414 - Refactor telephony test: Clear existing calls in setup stage

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

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.

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: Clear existing calls in test setup

Comment 2

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: Modify tests

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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?

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

(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.

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

(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(...);

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#2: Clear existing calls in test setup

Comment 8

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#2: Modify tests

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;

  ...
});

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 10

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#3: Clear existing calls in test setup

Comments addressed.

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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 :)

Comment 13

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#3: Modify tests

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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?

Comment 15

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 1: Clear existing calls in test setup. r=hsinyi

Comment 16

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 2: Modify tests. r=hsinyi

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

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

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8ee956154703
https://hg.mozilla.org/integration/b2g-inbound/rev/6ac79ac36ee3

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8ee956154703
https://hg.mozilla.org/mozilla-central/rev/6ac79ac36ee3