756966 - Fail hard when runEmulatorCmd callbacks still exist at the end of a test

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Philipp von Weitershausen [:philikon]]

If a test isn't written properly, it may call finish() before a runEmulatorCmd has returned. In that case we should just fail hard. Syncing a few async events together -- in the worst case -- shouldn't be too hard for the test author.

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: patch v0.1

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 626614 [details] [diff] [review]
patch v0.1

Actually this causes a test failure, investigating.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 626614 [details] [diff] [review]
patch v0.1

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

::: testing/marionette/marionette-listener.js
@@ +290,5 @@
>  
> +  // emulator callbacks
> +  sandbox._emu_cb_id = 0;
> +  sandbox._emu_cbs = {};
> +

I don't understand why we have to leak these into the sandbox. sandbox.asyncComplete closes over the global scope and should be able to access them as global variables just fine.

@@ +714,5 @@
>  function emulatorCmdResult(msg) {
>    let message = msg.json;
> +  if (!sandbox || !sandbox._emu_cbs[message.id]) {
> +    // we fail silently since an error should have been generated in finish()
> +    return;

Not a failure per se. It should be totally valid to not pass a callback at all. In fact, I think that's how we should write most runEmulatorCmd() invocations, after musing a bit about event racing. I'll post more in bug 756607.

Comment 4

[Jonathan Griffin (:jgriffin)]

Attachment: patch v0.2

Moved the callbacks back out of the sandbox.  This version passes all tests.

Comment 5

[Jonathan Griffin (:jgriffin)]

http://hg.mozilla.org/mozilla-central/rev/646cbeae01a0