710863 - Changes for bug 652494 break single-test mochitest execution

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I finally took the day and a half to bisect this problem that's been hitting me for a month...

The patch in bug 652494 adds these two bits to SimpleTest.js:

  var isPrimaryTestWindow = !!parent.TestRunner;
....

  if (isPrimaryTestWindow) {
    addLoadEvent(function() {
      if (SimpleTest._stopOnLoad) {
        SimpleTest.finish();
      }
    });
  }

which means that something like this:

python ../obj-firefox/_tests/testing/mochitest/runtests.py --test-path=content/base/test/test_DOMException.html 

never calls SimpleTest.finish() on the test, since there is no TestRunner in parent.  Which means that running single tests is a huge PITA.

I don't understand why that last part was made conditional on isPrimaryTestWindow.  Why was that done?

Comment 1

[Cameron McCormack (:heycam)]

That was done because there were a bunch of tests that were invoking SimpleTest.finish() more than once because they included SimpleTest.js inside windows they opened, and inside these other windows and were not calling SimpleTest.waitForExplicitFinish() to avoid the automatic SimpleTest.finish() call.  This was resulting in individual test results being assigned to the wrong test and other oddities.

Obviously we don't want to avoid calling SimpleTest.finish() for the main window of a single test run, so we should fix up the condition there somehow.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> because they included SimpleTest.js inside windows they opened

Ah, ok.  I see.

Can you check for window.opener, perhaps?

Comment 3

[Cameron McCormack (:heycam)]

Yes, checking some combination of window.opener and window.parent I think should work.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Finish mochitests when they are run in single test mode.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 581858 [details] [diff] [review]
Finish mochitests when they are run in single test mode.

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

r=me if you have tested on some of these corner cases.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +19,5 @@
> +
> +// In normal test runs, the window that has a TestRunner in its parent is
> +// the primary window.  In single test runs, if there is no parent and there
> +// is no opener then it is the primary window.
> +var isPrimaryTestWindow = !!parent.TestRunner || (parent == window && !opener);

actually there are a few exceptions to this rule.  Specifically in tests where we do a window.open (mostly found in browser-chrome) from the test case itself.  When we do that we specifically include files like SimpleTest.js, for example:
http://mxr.mozilla.org/mozilla-central/source/widget/tests/test_native_mouse_mac.xul

I think this actually resolves those cases, but the comment is a big confusing.

Comment 6

[Cameron McCormack (:heycam)]

But we don't include TestRunner.js in that opened window, right?  I'm not sure if the comment is incorrect.  Can you clarify?

I just tried test_native_mouse_mac.xul and it finishes properly in single test run mode.  (I did a try run yesterday for all mochitests and they came out green.)

While I'm here I have a question: when running a single plain mochitest, the expected result is for the small summary table to be shown in the browser once the test finishes, yes?  And for the browser to stay open displaying that.  I notice that chrome mochitests don't do this, instead the browser quits and it outputs the summary to the console.  Is that the expected behaviour, or should it be doing something like single plain mochitests?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> the expected result is for the small summary table to be shown in the browser once the
> test finishes, yes? And for the browser to stay open displaying that. 

Yes.  Chrome tests are all sorts of fucked up.

Comment 8

[Cameron McCormack (:heycam)]

Joel, per IRC explanation of my comment 6: we do not have any tests that include TestRunner.js in them (either in their main window, or in any window they open).  So I believe the comment is correct.  Let me know if I can land without changing the comment.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

yes, lets land this

Comment 10

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe9f1fea02b8

Comment 11

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

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