911063 - [FMRadio] Write marionette tests for WebFM

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::FMRadio, defect)

Description

[Pin Zhang [:pzhang] (inactive)]

Here is the review notes given by Justin about the tests patch:

(In reply to Justin Lebar [:jlebar] from comment #149)
> Comment on attachment 796409 [details] [diff] [review]
> Part III: Marionette tests
> 
> We need to make sure that all of these tests run twice, once in the main
> process and once in a child process.
> 
> >diff --git a/dom/fmradio/test/marionette/manifest.ini b/dom/fmradio/test/marionette/manifest.ini
> >new file mode 100644
> >--- /dev/null
> >+++ b/dom/fmradio/test/marionette/manifest.ini
> 
> >@@ -0,0 +1,13 @@
> >+[DEFAULT]
> >+b2g = true
> >+browser = false
> >+qemu = false
> 
> Nit: Maybe add a comment here explaining why we can't use QEMU?
> 
> >diff --git a/dom/fmradio/test/marionette/test_bug862672.js b/dom/fmradio/test/marionette/test_bug862672.js
> >new file mode 100644
> >--- /dev/null
> >+++ b/dom/fmradio/test/marionette/test_bug862672.js
> 
> >@@ -0,0 +1,46 @@
> >+/* Any copyright is dedicated to the Public Domain.
> >+ * http://creativecommons.org/publicdomain/zero/1.0/ */
> >+
> >+MARIONETTE_TIMEOUT = 10000;
> >+
> >+SpecialPowers.addPermission("fmradio", true, document);
> >+
> >+let FMRadio = window.navigator.mozFMRadio;
> >+
> >+function verifyInitialState() {
> >+  log("Verifying initial state.");
> >+  ok(FMRadio, "FMRadio");
> >+  is(FMRadio.enabled, false, "FMRadio.enabled");
> 
> In our other test harnesses, when a test fails it tells us which line it
> failed
> on.  If that's true in Marionette, I'm not sure that the strings here which
> just repeat the condition are useful.
> 
> >+  enableThenDisable();
> >+}
> >+
> >+function enableThenDisable() {
> >+  log("Enable FM Radio and disable it immediately.");
> >+  var request = FMRadio.enable(90);
> >+  ok(request, "request should not be null.");
> >+
> >+  request.onsuccess = function() {
> >+    ok(false, "Enable request should fail.");
> >+  };
> >+
> >+  var failedToEnable = false;
> >+  request.onerror = function() {
> >+    failedToEnable = true;
> >+  };
> >+
> >+  var disableReq = FMRadio.disable();
> >+  ok(disableReq, "Disable request should not be null.");
> >+
> >+  disableReq.onsuccess = function() {
> >+    ok(failedToEnable, "Enable request failed.");
> >+    ok(!FMRadio.enabled, "FMRadio.enabled is false.");
> >+    finish();
> >+  };
> >+
> >+  disableReq.onerror = function() {
> >+    ok(false, "Disable request should not fail.");
> >+  };
> >+}
> 
> I still have the concern about these tests from bug 862672 comment 26. 
> Unless
> the implementation has changed significantly, I don't think this is correct.
> 
> Last time I looked at this code, the following ordering was possible:
> 
>   website: FMRadio.enable();
>   gecko: Start enabling FM radio on separate thread.
>   gecko: Finish enabling FM radio on separate thread, post "enable
> successful" message to the main thread's event loop.
>   website: FMRadio.disable();
>   gecko: Post "disable successful" message to main thread's event loop.
> 
> I think this is also possible if this test runs in a child process:
> 
>   website: FMRadio.enable();
>   gecko child: Send "enable FM radio" message to parent
>   gecko parent: Send "FM radio enabled" message to child
>   gecko child: Post "enable successful" message to the main thread's event
> loop.
>   website: FMRadio.disable();
>   (and so on)
> 
> If this ordering occurs, this test will fail.
> 
> It seems that the only thing we can check here is that either
> 
>   - enable fails (as tested here), or
>   - enable's onsuccess fires before disable's onsuccess
> 
> But this is still tricky, because when enable's onsuccess fires, should
> FMRadio.enabled be true?  As currently written, it might not be.  I'm not
> sure
> whether we care.
> 
> >diff --git a/dom/fmradio/test/marionette/test_bug876597.js b/dom/fmradio/test/marionette/test_bug876597.js
> >new file mode 100644
> >--- /dev/null
> >+++ b/dom/fmradio/test/marionette/test_bug876597.js
> 
> This one looks good.
> 
> >+function cleanUp() {
> 
> We should ask the marionette folks if there's a way to ensure that this code
> gets run, even when the test fails or times out.  As is, we won't run this
> code, and that creates a situation where many FM radio tests after this one
> might also fail.
> 
> >diff --git a/dom/fmradio/test/marionette/test_cancel_seek.js b/dom/fmradio/test/marionette/test_cancel_seek.js
> >new file mode 100644
> >--- /dev/null
> >+++ b/dom/fmradio/test/marionette/test_cancel_seek.js
> 
> I think this has a similar race condition.
> 
> >diff --git a/dom/fmradio/test/marionette/test_enable_disable.js b/dom/fmradio/test/marionette/test_enable_disable.js
> >new file mode 100644
> >--- /dev/null
> >+++ b/dom/fmradio/test/marionette/test_enable_disable.js
> 
> >@@ -0,0 +1,97 @@
> >+/* Any copyright is dedicated to the Public Domain.
> >+ * http://creativecommons.org/publicdomain/zero/1.0/ */
> >+
> >+MARIONETTE_TIMEOUT = 10000;
> >+
> >+SpecialPowers.addPermission("fmradio", true, document);
> >+
> >+let FMRadio = window.navigator.mozFMRadio;
> >+
> >+function verifyInitialState() {
> >+  log("Verifying initial state.");
> >+  ok(FMRadio, "FMRadio");
> >+  is(FMRadio.enabled, false, "FMRadio.enabled");
> >+  verifyAttributesWhenDisabled();
> >+  enableFMRadio();
> >+}
> >+
> >+function verifyAttributesWhenDisabled() {
> 
> This function is used only once; maybe it should be inlined?
> 
> >+  log("Verifying attributes when disabled.");
> >+  is(FMRadio.frequency, null, "FMRadio.frequency == null");
> >+  ok(FMRadio.frequencyLowerBound, "FMRadio.frequencyLowerBound");
> >+  ok(FMRadio.frequencyUpperBound, "FMRadio.frequencyUpperBound");
> >+  ok(FMRadio.frequencyUpperBound > FMRadio.frequencyLowerBound,
> >+    "FMRadio.frequencyUpperBound > FMRadio.frequencyLowerBound");
> >+  ok(FMRadio.channelWidth, "FMRadio.channelWidth")
> >+}
> 
> >+function enableFMRadio() {
> >+  log("Verifying behaviors when enabled.");
> >+  var request = FMRadio.enable(90);
> >+  ok(request, "FMRadio.enable(90) returns request");
> >+
> >+  request.onsuccess = function() {
> >+    ok(FMRadio.enabled, "FMRadio.enabled");
> >+    ok(!isNaN(FMRadio.frequency), "FMRadio.frequency is a number");
> 
> This doesn't quite get what you want, because isNaN(null) is false.
> 
> >+    ok(FMRadio.frequency > FMRadio.frequencyLowerBound,
> >+      "FMRadio.frequency > FMRadio.frequencyLowerBound");
> 
> >= ?
> 
> >+  var seekRequest = FMRadio.seekUp();
> >+
> >+  seekRequest.onerror = function() {
> >+    results.seekErrorFired = true;
> >+    checkResults();
> >+  };
> >+
> >+  seekRequest.onsuccess = function() {
> >+    ok(false, "Seek request should fail after FMRadio.disable is called.");
> >+  };
> >+
> >+  FMRadio.disable();
> >+  FMRadio.ondisabled = function() {
> >+    FMRadio.ondisabled = null;
> >+    results.onDisabledEventFired = true;
> >+    ok(!FMRadio.enabled, "FMRadio.enabled is false");
> >+    checkResults();
> >+  };
> >+}
> 
> Similarly I think this part with the seek is racy.
> 
> I'm going to stop reviewing here, since it seems there's enough to work
> through.

Comment 1

[Pin Zhang [:pzhang] (inactive)]

> It seems that the only thing we can check here is that either
> 
>   - enable fails (as tested here), or
>   - enable's onsuccess fires before disable's onsuccess
> 
I agree that it's almost the only thing we can check here, but I still have questions about this, first, since we don't support multi-process (Bug 867831) yet, how can we verify the logics for the potential race condition? second, if enable's onsuccess fires before disable's onsuccess, i don't think it means we pass the test, because it might happen when the tests runs in parent process which I think it means we failed to run the test on the contrary, how can we handle such a case?

Comment 2

[Pin Zhang [:pzhang] (inactive)]

> >+
> >+  request.onsuccess = function() {
> >+    ok(FMRadio.enabled, "FMRadio.enabled");
> >+    ok(!isNaN(FMRadio.frequency), "FMRadio.frequency is a number");
> 
> This doesn't quite get what you want, because isNaN(null) is false.
Since FMRadio.frequency is null when it's disabled, and it might be null too even when it's enabled if something is wrong, so what I want here is to make sure we can get a number from FMRadio.frequency instead of null or something else when FM radio enabled.

Comment 3

[Pin Zhang [:pzhang] (inactive)]

> 
> >+function cleanUp() {
> 
> We should ask the marionette folks if there's a way to ensure that this code
> gets run, even when the test fails or times out.  As is, we won't run this
> code, and that creates a situation where many FM radio tests after this one
> might also fail.

Hi, Malini, can we ensure that |cleanUp| runs even the test fails? In all of my tests, I assume the FM radio is disabled before the tests run, and disable the FM radio in |cleanUp| when the tests finished, so if we can't ensure |cleanUp| runs, then I need to check if it's enabled or disabled before the new test starts.

Comment 4

[Justin Lebar (not reading bugmail)]

> since we don't support multi-process (Bug 867831) yet, how can we verify the logics for 
> the potential race condition?

Like I said in the bit copy-pasted in comment 0, I think this race occurs even if the test runs in-process:

>   website: FMRadio.enable();
>   gecko: Start enabling FM radio on separate thread.
>   gecko: Finish enabling FM radio on separate thread, post "enable
> successful" message to the main thread's event loop.
>   website: FMRadio.disable();
>   gecko: Post "disable successful" message to main thread's event loop.

I could be misremembering how this code works, though.

Comment 5

[Pin Zhang [:pzhang] (inactive)]

Attachment: marionette tests

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 798350 [details] [diff] [review]
marionette tests

My last day was Friday.  You'll need to find someone else to review this; sorry!

Comment 7

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar (not reading bugmail) from comment #6)
> Comment on attachment 798350 [details] [diff] [review]
> marionette tests
> 
> My last day was Friday.  You'll need to find someone else to review this;
> sorry!

Is there any reviewer you would recommend?

Comment 8

[Justin Lebar (not reading bugmail)]

If it were me, I'd ask khuey, and if he didn't want to do it, I'd ask him to find someone.  :)  Thankfully this test might as well be a mochitest; there's no marionette magic here.

Comment 9

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar (not reading bugmail) from comment #8)
> If it were me, I'd ask khuey, and if he didn't want to do it, I'd ask him to
> find someone.  :)  Thankfully this test might as well be a mochitest;
> there's no marionette magic here.

Hi, :khuey, could you help to find someone to review the tests' patch?

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Pin Zhang [:pzhang] from comment #9)
> (In reply to Justin Lebar (not reading bugmail) from comment #8)
> > If it were me, I'd ask khuey, and if he didn't want to do it, I'd ask him to
> > find someone.  :)  Thankfully this test might as well be a mochitest;
> > there's no marionette magic here.
> 
> Hi, :khuey, could you help to find someone to review the tests' patch?

I'm still not qualified to review these patches but since Justin has left nobody is so I may as well review them.

Comment 11

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Pin Zhang [:pzhang] from comment #3)
> > 
> > >+function cleanUp() {
> > 
> > We should ask the marionette folks if there's a way to ensure that this code
> > gets run, even when the test fails or times out.  As is, we won't run this
> > code, and that creates a situation where many FM radio tests after this one
> > might also fail.
> 
> Hi, Malini, can we ensure that |cleanUp| runs even the test fails? In all of
> my tests, I assume the FM radio is disabled before the tests run, and
> disable the FM radio in |cleanUp| when the tests finished, so if we can't
> ensure |cleanUp| runs, then I need to check if it's enabled or disabled
> before the new test starts.

I've filed Bug 916015 to track this feature.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 798350 [details] [diff] [review]
marionette tests

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

r-, but only because of the possible race in test_enable_disable.js.  Other than that this looks pretty good.

We should probably redesign this API at some point in the future so it's not so racy ...

::: dom/fmradio/test/marionette/test_bug862672.js
@@ +26,5 @@
> +  };
> +
> +  var enableCompleted = false;
> +  request.onsuccess = function() {
> +    of(!failedToEnable);

Is this supposed to be 'ok'?

@@ +37,5 @@
> +  disableReq.onsuccess = function() {
> +    // There are two possibilities which depends on the system
> +    // process scheduling (bug 911063 comment 0):
> +    //   - enable fails
> +    //   - enable's onsuccess fires before disable's onsucess

It seems like unfortunate API Design that the result here is non-deterministic, but I guess we don't need to worry about that here.

::: dom/fmradio/test/marionette/test_bug876597.js
@@ +55,5 @@
> +    enableFMRadioWithAirplanModeEnabled();
> +  };
> +
> +  mozSettings.createLock().set({
> +    "ril.radio.disabled": true

You may as well use KEY here.

@@ +72,5 @@
> +}
> +
> +function cleanUp() {
> +  let req = mozSettings.createLock().set({
> +    "ril.radio.disabled": false

And here.

::: dom/fmradio/test/marionette/test_enable_disable.js
@@ +12,5 @@
> +  ok(FMRadio);
> +  is(FMRadio.enabled, false);
> +
> +  log("Verifying attributes when disabled.");
> +  is(FMRadio.frequency, null, "FMRadio.frequency == null");

As jlebar said, strings that duplicate the condition aren't very useful.

@@ +30,5 @@
> +  ok(request, "FMRadio.enable(r" + frequency + ") returns request");
> +
> +  request.onsuccess = function() {
> +    ok(FMRadio.enabled, "FMRadio.enabled");
> +    ok(!isNaN(FMRadio.frequency), "FMRadio.frequency is a number");

isNaN(null) is false, so this doesn't test what you want.  How about 'typeof FMRadio.frequency == "number"'?

@@ +82,5 @@
> +  seekRequest.onsuccess = function() {
> +    ok(false, "Seek request should fail after FMRadio.disable is called.");
> +  };
> +
> +  FMRadio.disable();

Why doesn't this have the same race condition?  Can seek request complete before we get to call FMRadio.disable()?

::: dom/fmradio/test/marionette/test_seek_up_and_down.js
@@ +45,5 @@
> +
> +  request.onerror = function() {
> +    ok(false, "Seekdown request should not fail.");
> +  };
> +

nit: extra newline

Comment 13

[Pin Zhang [:pzhang] (inactive)]

Attachment: Patch V2

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 805894 [details] [diff] [review]
Patch V2

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

::: dom/fmradio/test/marionette/test_bug876597.js
@@ +59,5 @@
> +  settings[KEY] = true;
> +  mozSettings.createLock().set(settings);
> +}
> +
> +function enableFMRadioWithAirplanModeEnabled() {

you're missing an 'e' on the end of Airplane in this function name

Comment 15

[Pin Zhang [:pzhang] (inactive)]

Attachment: Patch V3

Added the missing 'e' mentioned in comment 14, r=khuey.

Hi, @khuey, thanks for your help!

Comment 16

[Pin Zhang [:pzhang] (inactive)]

Attachment: Patch V3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

For the future, please use your full name in the hg commit information.

Comment 18

[Ed Morley [:emorley]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

These tests were landed but never actually enabled. Looking at the logs, indeed they are *not* running on trunk.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Try run to see if they timeout when enabled like they do on v1.1hd.
https://tbpl.mozilla.org/?tree=Try&rev=8a7ec1685530

Comment 21

[Ryan VanderMeulen [:RyanVM]]

These fail when enabled on Try the same as how they did when enabled on b2g18_v1_1_0hd.
https://tbpl.mozilla.org/?tree=Try&rev=dd5034864253

Comment 22

[Pin Zhang [:pzhang] (inactive)]

These tests only work on b2g, because we don't support fm radio emulation yet, see bug 872417.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Gotcha, thanks.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8ba2e5166ed3