Closed Bug 756966 Opened 10 years ago Closed 10 years ago

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


(Testing :: Marionette, defect)

Not set


(Not tracked)



(Reporter: philikon, Assigned: jgriffin)




(1 file, 1 obsolete file)

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.
Assignee: nobody → jgriffin
Attached patch patch v0.1 (obsolete) — Splinter Review
Attachment #626614 - Flags: review?(philipp)
Comment on attachment 626614 [details] [diff] [review]
patch v0.1

Actually this causes a test failure, investigating.
Attachment #626614 - Flags: review?(philipp)
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[]) {
> +    // 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.
Attached patch patch v0.2Splinter Review
Moved the callbacks back out of the sandbox.  This version passes all tests.
Attachment #626614 - Attachment is obsolete: true
Attachment #626636 - Flags: review?(philipp)
Attachment #626636 - Flags: review?(philipp) → review+
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.