754216 - Control the emulator from within Marionette JS scripts

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Philipp von Weitershausen [:philikon]]

Along with SpecialPowers (bug 754178), this will vastly simplify how we can write WebAPI tests. Because we can then just put the entire test in one neat JS file.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: v1

Here's an initial version. It's a bit rough around the edges, and in some parts perhaps even a bit hacky. But look at the beautiful sample test I added!!11!

Srsly though, I'm already very happy with how this turned out, but I fully expect this to go through a few iterations before it's ready. Punting for a follow-up: 2nd emulator support.

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 623085 [details] [diff] [review]
v1

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

Looking good!

::: dom/telephony/test/marionette/test_incoming_call.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +MARIONETTE_TIMEOUT = 10000;

Are you OK with this method of defining a timeout?  We could alternately add another sandbox function so a test could set it using a JS call.  I don't have much of an opinion either way.

@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +MARIONETTE_TIMEOUT = 10000;
> +
> +//TODO navigate to a known test page? or handle in harness?

Navigating during an async test is dangerous, because it upsets some of the Marionette event handlers (e.g., the unload handler).  We'd have to change this to let async tests do their own navigation.

Alternately, we could define a MARIONETTE_TESTURL constant, handled similarly to MARIONETTE_TIMEOUT, which would cause the harness to perform some navigation before starting the test.

Or, we could have a test.properties file like we discussed last week, and set these things there.

@@ +92,5 @@
> +}
> +
> +function finish() {
> +  marionetteScriptFinished(generate_results());
> +}

Do we need this?  We already define a sandbox.finish() which does just this.

::: testing/marionette/client/marionette/marionette.py
@@ +180,5 @@
> +    def _handle_emulator_cmd(self, response):
> +        cmd = response.get("emulator_cmd")
> +        if not cmd or not self.emulator:
> +            #TODO shoudn't silently drop this, should we?
> +            return

Let's raise a MarionetteException for this.

::: testing/marionette/client/marionette/tests/unit/test_specialpowers.py
@@ +13,5 @@
>          return SpecialPowers.getCharPref("testing.marionette.charpref")
>          """);
>          self.assertEqual(result, "blabla")
>  
> +    # XXX TODO this doesn't really belong here, but whatevs...

We should move this to a separate file, since we'll want to run the SpecialPowers tests on all builds (incl. Firefox), but the emulator command will only be run for cases where qemu=true.

::: testing/marionette/marionette-actors.js
@@ +1198,5 @@
> +
> +    let cb = this._emu_cbs[message.id];
> +    delete this._emu_cbs[message.id];
> +    if (typeof cb != "function") {
> +      return;

We should probably throw an exception here.

::: testing/marionette/marionette-listener.js
@@ +700,5 @@
> +  let message = msg.json;
> +  let cb = _emu_cbs[message.id];
> +  delete _emu_cbs[message.id];
> +  if (typeof cb != "function") {
> +    return;

We should probably throw an exception here.

::: testing/marionette/marionette-simpletest.js
@@ +4,5 @@
>  /*
>   * The Marionette object, passed to the script context.
>   */
>  
> +function Marionette(actor, window, context, logObj) {

The first parameter here is only an actor when Marionette is instantiated from marionette-actors; in marionette-listener 'this' is the global scope.  Maybe we should call this variable 'scope' instead of 'actor'?

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to Jonathan Griffin (:jgriffin) from comment #2)
> ::: dom/telephony/test/marionette/test_incoming_call.js
> @@ +1,4 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +MARIONETTE_TIMEOUT = 10000;
> 
> Are you OK with this method of defining a timeout?

Meh, sure. Is there a sensible default timeout?

> We could alternately add another sandbox function so a test could set it using a
> JS call.

What would that look like? Would it have to cycle back to Python via the socket? Or would it just set a timer in the JS?

> @@ +2,5 @@
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +MARIONETTE_TIMEOUT = 10000;
> > +
> > +//TODO navigate to a known test page? or handle in harness?
> 
> Navigating during an async test is dangerous, because it upsets some of the
> Marionette event handlers (e.g., the unload handler).  We'd have to change
> this to let async tests do their own navigation.

That's what I meant, yes. Provide a helper in the test scope that sets window.location, listens for the "load" event, etc. and then calls us back. That would work right?

The only thing I couldn't figure out was how to compute the absolute URL of provided test pages. So maybe we should transmit the base URL of the test server along with the initial test parameters and test code, and we expose that or a marionette.absoluteUrl helper in the test's scope? I guess that's similar to your MARIONETTE_TESTUR idea.

But the more I think about it, I feel like every ".js" test should get its own pristine blank page by default, freshly loaded. I don't really see a reason for doing anything else. We can still provide means for navigating away to a different page, but I don't see a use case for that yet. Also, I feel like in that case you'll probably want to provide the page yourself, so we'd have to make Marionette host them via the web server.


As for your other comments, 100% agreed. Thanks for the thorough review. It was 4am and I wanted to get stuff working, so I cut a few corners ;)

Comment 4

[Jonathan Griffin (:jgriffin)]

(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> (In reply to Jonathan Griffin (:jgriffin) from comment #2)
> > ::: dom/telephony/test/marionette/test_incoming_call.js
> > @@ +1,4 @@
> > > +/* Any copyright is dedicated to the Public Domain.
> > > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > > +
> > > +MARIONETTE_TIMEOUT = 10000;
> > 
> > Are you OK with this method of defining a timeout?
> 
> Meh, sure. Is there a sensible default timeout?

No, we should add a sensible default timeout.  Is 10s sensible?

> 
> > We could alternately add another sandbox function so a test could set it using a
> > JS call.
> 
> What would that look like? Would it have to cycle back to Python via the
> socket? Or would it just set a timer in the JS?

It wouldn't go to Python, it would just set a timer in the JS.  It would cause us to cancel the existing timeout timer in marionette-listeners, and add a new one with the specified timeout.

> 
> > @@ +2,5 @@
> > > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > > +
> > > +MARIONETTE_TIMEOUT = 10000;
> > > +
> > > +//TODO navigate to a known test page? or handle in harness?
> > 
> > Navigating during an async test is dangerous, because it upsets some of the
> > Marionette event handlers (e.g., the unload handler).  We'd have to change
> > this to let async tests do their own navigation.
> 
> That's what I meant, yes. Provide a helper in the test scope that sets
> window.location, listens for the "load" event, etc. and then calls us back.
> That would work right?
> 
> The only thing I couldn't figure out was how to compute the absolute URL of
> provided test pages. So maybe we should transmit the base URL of the test
> server along with the initial test parameters and test code, and we expose
> that or a marionette.absoluteUrl helper in the test's scope? I guess that's
> similar to your MARIONETTE_TESTUR idea.
> 
> But the more I think about it, I feel like every ".js" test should get its
> own pristine blank page by default, freshly loaded. I don't really see a
> reason for doing anything else. We can still provide means for navigating
> away to a different page, but I don't see a use case for that yet. Also, I
> feel like in that case you'll probably want to provide the page yourself, so
> we'd have to make Marionette host them via the web server.
> 

Since these are handled a bit different than Selenium-style tests (I don't want to break Selenium compatibility), I'm fine with making them load a default test.html page at test startup, unless there is a test_foo.html page to match the test_foo.js script, in which case it could load that page instead.  This could all be handled by the Python testrunner.

Currently, all static files are served from the 'www' folder under marionette/client, which acts as the root for the MozHttpd instance.  But we could change the document root to the gecko root so that it could serve files from anywhere within gecko, and thus allow us to store the .html and .js files for a test side-by-side in the tree.

Comment 5

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

Comment on attachment 623085 [details] [diff] [review]
v1

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

Looks great, the new test is much cleaner!

::: dom/telephony/test/marionette/test_incoming_call.js
@@ +35,5 @@
> +    incoming = event.call;
> +    ok(incoming);
> +    is(incoming.number, number);
> +    is(incoming.state, "incoming");
> +    //is(incoming, telephony.active);  //TODO what should this be?

We should set up the telephony.onincoming function before we call runEmulatorCmd("gsm call "...), unless it isn't possible for runEmulatorCmd("gsm call "...) to fire the event before the set up.

@@ +65,5 @@
> +  incoming.answer();
> +};
> +
> +function hangUp() {
> +  log("Answering the incoming call.");

should be "hanging up the call" so the logs don't confuse people

::: testing/marionette/marionette-actors.js
@@ +1201,5 @@
> +    if (typeof cb != "function") {
> +      return;
> +    }
> +    try {
> +      cb(message.result);

Shouldn't this callback be run in the sandbox, so it has access to the globals and other helpers?

::: testing/marionette/marionette-listener.js
@@ +703,5 @@
> +  if (typeof cb != "function") {
> +    return;
> +  }
> +  try {
> +    cb(message.result);

Same here

Comment 6

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

Weird, I submitted my comments after the review went to review+, and it reset the state to r?. Sorry, jgriffin, you'll have to r+ again!

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to Malini Das [:mdas] from comment #6)
> Weird, I submitted my comments after the review went to review+, and it
> reset the state to r?. Sorry, jgriffin, you'll have to r+ again!

No worries I'll submit a new patch later today anyway.

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to Malini Das [:mdas] from comment #5)
> ::: dom/telephony/test/marionette/test_incoming_call.js
> @@ +35,5 @@
> > +    incoming = event.call;
> > +    ok(incoming);
> > +    is(incoming.number, number);
> > +    is(incoming.state, "incoming");
> > +    //is(incoming, telephony.active);  //TODO what should this be?
> 
> We should set up the telephony.onincoming function before we call
> runEmulatorCmd("gsm call "...), unless it isn't possible for
> runEmulatorCmd("gsm call "...) to fire the event before the set up.

I'm not 100% sure if it's impossible, so your suggestion definitely makes it safer.

> ::: testing/marionette/marionette-actors.js
> @@ +1201,5 @@
> > +    if (typeof cb != "function") {
> > +      return;
> > +    }
> > +    try {
> > +      cb(message.result);
> 
> Shouldn't this callback be run in the sandbox, so it has access to the
> globals and other helpers?

It will. Its scope is unaffected by how we call it here. Closures FTW! :)

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to Jonathan Griffin (:jgriffin) from comment #4)
> (In reply to Philipp von Weitershausen [:philikon] from comment #3)
> > (In reply to Jonathan Griffin (:jgriffin) from comment #2)
> > > ::: dom/telephony/test/marionette/test_incoming_call.js
> > > @@ +1,4 @@
> > > > +/* Any copyright is dedicated to the Public Domain.
> > > > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > > > +
> > > > +MARIONETTE_TIMEOUT = 10000;
> > > 
> > > Are you OK with this method of defining a timeout?
> > 
> > Meh, sure. Is there a sensible default timeout?
> 
> No, we should add a sensible default timeout.  Is 10s sensible?

Yep. Should I just add it to MarionetteJSTestCase?

> > > We could alternately add another sandbox function so a test could set it using a
> > > JS call.
> > 
> > What would that look like? Would it have to cycle back to Python via the
> > socket? Or would it just set a timer in the JS?
> 
> It wouldn't go to Python, it would just set a timer in the JS.  It would
> cause us to cancel the existing timeout timer in marionette-listeners, and
> add a new one with the specified timeout.

Ok. I can provide that too. Definitely better than the source parsing (regular expressions ick!!!!)

> Since these are handled a bit different than Selenium-style tests (I don't
> want to break Selenium compatibility), I'm fine with making them load a
> default test.html page at test startup,

Ok, that's a good first step. I'll add that to MarionetteJSTestCase.

> unless there is a test_foo.html page
> to match the test_foo.js script, in which case it could load that page
> instead.

Excellent idea. Follow-up?

> Currently, all static files are served from the 'www' folder under
> marionette/client, which acts as the root for the MozHttpd instance.  But we
> could change the document root to the gecko root so that it could serve
> files from anywhere within gecko, and thus allow us to store the .html and
> .js files for a test side-by-side in the tree.

+1

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> (In reply to Jonathan Griffin (:jgriffin) from comment #4)
> > (In reply to Philipp von Weitershausen [:philikon] from comment #3)
> > > (In reply to Jonathan Griffin (:jgriffin) from comment #2)
> > > > ::: dom/telephony/test/marionette/test_incoming_call.js
> > > > @@ +1,4 @@
> > > > > +/* Any copyright is dedicated to the Public Domain.
> > > > > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > > > > +
> > > > > +MARIONETTE_TIMEOUT = 10000;
> > > > 
> > > > Are you OK with this method of defining a timeout?
> > > 
> > > Meh, sure. Is there a sensible default timeout?
> > 
> > No, we should add a sensible default timeout.  Is 10s sensible?
> 
> Yep. Should I just add it to MarionetteJSTestCase?

Actually, let's do that in a follow-up as well, ok?

Comment 11

[Jonathan Griffin (:jgriffin)]

> > unless there is a test_foo.html page
> > to match the test_foo.js script, in which case it could load that page
> > instead.
> > 
> Excellent idea. Follow-up?

> > > No, we should add a sensible default timeout.  Is 10s sensible?
> > 
> > Yep. Should I just add it to MarionetteJSTestCase?
> 
> Actually, let's do that in a follow-up as well, ok?

Yep, totally fine.  I'll file a bug about letting tests automatically load an html file if it sits side-by-side the test file with the same base name.

Comment 12

[Philipp von Weitershausen [:philikon]]

Attachment: v2

Comments addressed. Split the new test out to a separate patch.

Comment 13

[Philipp von Weitershausen [:philikon]]

Attachment: new telephony tests, v1

Addressed comments. Will add more tests later, either here or in follow ups.

Comment 14

[Jonathan Griffin (:jgriffin)]

Comment on attachment 624949 [details] [diff] [review]
v2

<3

Comment 15

[Jonathan Griffin (:jgriffin)]

Comment on attachment 624950 [details] [diff] [review]
new telephony tests, v1

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

Looks good but see below.

::: dom/telephony/test/marionette/test_incoming_call.js
@@ +102,5 @@
> +  incoming.hangUp();
> +}
> +
> +function cleanUp() {
> +  SpecialPowers.setCharPref(WHITELIST_PREF);

This line here is causing a JS error:

E/GeckoConsole(  807): [JavaScript Error: "uncaught exception: SpecialPowersException: "Invalid parameters for set in SPPrefService""]

This failure causes a cascade of other failures.

Deleting this line allows all tests to pass.

Comment 16

[Philipp von Weitershausen [:philikon]]

(In reply to Jonathan Griffin (:jgriffin) from comment #15)
> ::: dom/telephony/test/marionette/test_incoming_call.js
> @@ +102,5 @@
> > +  incoming.hangUp();
> > +}
> > +
> > +function cleanUp() {
> > +  SpecialPowers.setCharPref(WHITELIST_PREF);
> 
> This line here is causing a JS error:
> 
> E/GeckoConsole(  807): [JavaScript Error: "uncaught exception:
> SpecialPowersException: "Invalid parameters for set in SPPrefService""]
> 
> This failure causes a cascade of other failures.
> 
> Deleting this line allows all tests to pass.

Hmm, I meant to write "resetCharPref". Why the heck is it passing for me?!?

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> Hmm, I meant to write "resetCharPref". Why the heck is it passing for me?!?

And by "resetCharPref" I meant "clearUserPref"...

Comment 18

[Jonathan Griffin (:jgriffin)]

Comment on attachment 624950 [details] [diff] [review]
new telephony tests, v1

This is strange, but for me this test is causing an intermittent gecko crash sometime after the test completes.

F/libc    (  807): @@@ ABORTING: INVALID HEAP ADDRESS IN dlfree
F/libc    (  807): Fatal signal 11 (SIGSEGV) at 0xdeadbaad (code=1)

This can occur several seconds after the above test finishes.  I never see it with this test removed.

Comment 19

[Jonathan Griffin (:jgriffin)]

Another instance of the crash:

F/libc    (  809): Fatal signal 11 (SIGSEGV) at 0x0000000b (code=1)
I/DEBUG   (  799): debuggerd committing suicide to free the zombie!

I can't tell if it's due to SpecialPowers, the JS-emu code, or the telephony API's that are being exercised.

Comment 20

[Philipp von Weitershausen [:philikon]]

Well that's fun! I'm not seeing these at all... Can uyou do some bisecting to see what might be causing this?

Comment 21

[Jonathan Griffin (:jgriffin)]

(In reply to Philipp von Weitershausen [:philikon] from comment #20)
> Well that's fun! I'm not seeing these at all... Can uyou do some bisecting
> to see what might be causing this?

Yeah I'll try to see if it's SpecialPowers, JS-emu, or telephony code.

Comment 22

[Jonathan Griffin (:jgriffin)]

So I did some debugging.  It turns out that the culprit is the use of the log() method from within a runEmulatorCmd callback.  If you remove the calls to log() there, the crash goes away.

Moving the callbacks into the sandbox didn't help.

Comment 23

[Jonathan Griffin (:jgriffin)]

I tried moving the Marionette log obj into the sandbox, instead of using a globally defined one, but this didn't help.

I only see the crash when running all the tests, e.g.,

python runtests.py --emulator x86 --homedir ~/x86B2G/ --type b2g tests/unit-tests.ini 

Have you not seen it?  The crash is being reported by "libc":

F/libc    (  809): Fatal signal 11 (SIGSEGV) at 0x0000000b (code=1)

..so I wonder if this could be system specific.

Comment 24

[Philipp von Weitershausen [:philikon]]

As per IRC discussion, I landed the actual patch but not the tests. That way people can start writing tests at least. Let's investigate the intermittent crashes in a follow-up.

https://hg.mozilla.org/mozilla-central/rev/9dab33fa5ff4

Comment 25

[Philipp von Weitershausen [:philikon]]

Comment on attachment 624950 [details] [diff] [review]
new telephony tests, v1

Deferred this to a follow-up.