676147 - looping single mochitests with make command doesn't work

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

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

You cannot currently run single plain mochitests in loops using the following make command:

TEST_PATH=some test EXTRA_TEST_ARGS='--loops=1' make mochitest-plain

chrome and browser-chrome tests are fine. This bug is due to an assumption in runtests.py. For single, plain mochitests, the code assumes cwd is the same directory that runtests.py is currently in.

Comment 1

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

Attachment: Allows plain mochitests to run in loops from the make command

This patch changes the assumption made in runtests.py's buildTestPath that cwd is the same directory as plain-loop.html. Now it will take the directory (which will be the obj-dir if called by make, or the current directory when calling runtests.py directly) and adds the path to the directory where plain-loop.html lives.

Also, I changed TestRunner.js so that it will call onComplete() at the end of loopTest if an onComplete method was provided. This allows the '--close-when-done' argument to be respected. I added quit.js to plain-loop.html so that it will register the goQuitApplication function so that '--close-when-done' will work.

Comment 2

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

Comment on attachment 555183 [details] [diff] [review]
Allows plain mochitests to run in loops from the make command

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

::: testing/mochitest/runtests.py
@@ +440,5 @@
>    def buildTestPath(self, options):
>      """ Build the url path to the specific test harness and test file or directory """
>      testHost = "http://mochi.test:8888"
>      testURL = testHost + self.TEST_PATH + options.testPath
> +    if os.path.isfile(os.path.join(self.oldcwd, os.path.dirname(__file__)) + self.TEST_PATH + options.testPath) and options.loops > 0:

can you make this:
if os.path.isfile(os.path.join(self.oldcwd, os.path.dirname(__file__), self.TEST_PATH, options.testPath)) and options.loops > 0:

@@ +578,5 @@
>        if "MOZ_HIDE_RESULTS_TABLE" in env and env["MOZ_HIDE_RESULTS_TABLE"] == "1":
>          self.urlOpts.append("hideResultsTable=1")
>        if options.loops:
>          self.urlOpts.append("loops=%d" % options.loops)
> +      if os.path.isfile(os.path.join(self.oldcwd, os.path.dirname(__file__)) + self.TEST_PATH + options.testPath) and options.loops > 0:

same here?

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +256,5 @@
>          testWindow.close();
> +        if (TestRunner.loops == 0 && TestRunner.onComplete) {
> +          TestRunner.onComplete();
> +        }
> +        --TestRunner.loops;

I don't understand how this worked before.  Where was TestRunner.loops decremented?  Why are we doing this while numLoops >= 0, not >0?  Where is numLoops changed?

Comment 3

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

(In reply to Joel Maher (:jmaher) from comment #2)
> Comment on attachment 555183 [details] [diff] [review]
> Allows plain mochitests to run in loops from the make command
> 
> Review of attachment 555183 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mochitest/runtests.py
> @@ +440,5 @@
> >    def buildTestPath(self, options):
> >      """ Build the url path to the specific test harness and test file or directory """
> >      testHost = "http://mochi.test:8888"
> >      testURL = testHost + self.TEST_PATH + options.testPath
> > +    if os.path.isfile(os.path.join(self.oldcwd, os.path.dirname(__file__)) + self.TEST_PATH + options.testPath) and options.loops > 0:
> 
> can you make this:
> if os.path.isfile(os.path.join(self.oldcwd, os.path.dirname(__file__),
> self.TEST_PATH, options.testPath)) and options.loops > 0:
> 
> @@ +578,5 @@
> >        if "MOZ_HIDE_RESULTS_TABLE" in env and env["MOZ_HIDE_RESULTS_TABLE"] == "1":
> >          self.urlOpts.append("hideResultsTable=1")
> >        if options.loops:
> >          self.urlOpts.append("loops=%d" % options.loops)
> > +      if os.path.isfile(os.path.join(self.oldcwd, os.path.dirname(__file__)) + self.TEST_PATH + options.testPath) and options.loops > 0:
> 
> same here?
> 

self.TEST_PATH is defined as '/test/' and is being used to create both file paths and urls in this object, so doing what you suggest doesn't result in the expected path. I was thinking that I could remove the slashes in the definition and use join() to form the URLs, like so:
testURL = ("/").join([testHost, self.TEST_PATH ,options.testPath])

and it would enable us to use os.path.join correctly, and avoid us concatenating our own urls with '+'

Alternatively, I can do: 
os.path.isfile(os.path.join(self.oldcwd, os.path.dirname(__file__), self.TEST_PATH.strip('/'), options.testPath))

to get the expected results, but that's less pretty.

> ::: testing/mochitest/tests/SimpleTest/TestRunner.js
> @@ +256,5 @@
> >          testWindow.close();
> > +        if (TestRunner.loops == 0 && TestRunner.onComplete) {
> > +          TestRunner.onComplete();
> > +        }
> > +        --TestRunner.loops;
> 
> I don't understand how this worked before.  Where was TestRunner.loops
> decremented?  Why are we doing this while numLoops >= 0, not >0?  Where is
> numLoops changed?

This code resides in the loopTest function, which is only called by plain-loop.html, so that we could preserve the the current behaviour of running single tests outside of iframes.

Say the number of loops is n. It will create n calls to checkComplete, which launches one test at a time in a new window (or gets the window if it was already launched) and checks if the test was completed, then would update the UI and close itself, allowing a new window to be opened if there is another call to checkComplete waiting.

TestRunner.loops wasn't being used previously. numLoops was being used to create n calls to checkComplete. The reason why TestRunner.loops being used now is that numLoops was just being used to make the right number of calls to checkComplete, but it wasn't being used to check if the tests were actually completed. Instead of reusing this variable, for clearer code, I should introduce a variable that keeps track of completed tests named completed, as below:

TestRunner.loopTest = function(testPath) {
  //must set the following line so that TestHarness.updateUI finds the right div to update
  document.getElementById("current-test-path").innerHTML = testPath;
  var numLoops = TestRunner.loops;
  var completed = 0;
  while (numLoops >= 0) {
    function checkComplete() {
      var testWindow = window.open(testPath, 'test window');
      if (testWindow.document.readyState == "complete") {
        TestRunner.currentTestURL = testPath;
        TestRunner.updateUI(testWindow.SimpleTest._tests);
        testWindow.close();
        if (TestRunner.loops == completed  && TestRunner.onComplete) {
          TestRunner.onComplete();
        }   
        completed++;
      }   
      else {
        setTimeout(checkComplete, 1000);
      }   
    }   
    checkComplete();
    numLoops--;
  }
}


Lastly, the reason we use >=0 instead of >0 is because if the user passes in loops=1, that means that the test should run once and loop once, for a total of 2 runs, as explained in the runtests.py description for loops.

Comment 4

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

Attachment: URL and file path fixes, and allow mochiplain loop tests to have an onComplete function

updated as per r-

Comment 5

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

Comment on attachment 555514 [details] [diff] [review]
URL and file path fixes, and allow mochiplain loop tests to have an onComplete function

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

assuming this has passed on try, r+

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +247,5 @@
> +  //must set the following line so that TestHarness.updateUI finds the right div to update
> +  document.getElementById("current-test-path").innerHTML = testPath;
> +  var numLoops = TestRunner.loops;
> +  var completed = 0;
> +  while (numLoops >= 0) {

I still don't get the >=0 condition here.  If I put in --loops=1, this while loop will run twice when I only want it to run once.  Anyway, this was in there before, so the changes I see here are fine.

@@ +254,5 @@
>        if (testWindow.document.readyState == "complete") {
>          TestRunner.currentTestURL = testPath;
>          TestRunner.updateUI(testWindow.SimpleTest._tests);
>          testWindow.close();
> +        if (TestRunner.loops == completed  && TestRunner.onComplete) {

is there any chance TestRunner.loops < completed?  I don't see the need for this logic if we have numLoops being decremented.  Could you just explain the need for the completed variable.

Comment 6

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

Attachment: URL and file path fixes, and allow mochiplain loop tests to have an onComplete function

(In reply to Joel Maher (:jmaher) from comment #5)
> Comment on attachment 555514 [details] [diff] [review]
> URL and file path fixes, and allow mochiplain loop tests to have an
> onComplete function
> 
> Review of attachment 555514 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> assuming this has passed on try, r+
> 
> ::: testing/mochitest/tests/SimpleTest/TestRunner.js
> @@ +247,5 @@
> > +  //must set the following line so that TestHarness.updateUI finds the right div to update
> > +  document.getElementById("current-test-path").innerHTML = testPath;
> > +  var numLoops = TestRunner.loops;
> > +  var completed = 0;
> > +  while (numLoops >= 0) {
> 
> I still don't get the >=0 condition here.  If I put in --loops=1, this while
> loop will run twice when I only want it to run once.  Anyway, this was in
> there before, so the changes I see here are fine.

Think of 'loops' as 'repetitions'. --loops=1 means we will repeat the test once. The expected behavior is detailed in runtests.py with the help command to clear any confusion.

> 
> @@ +254,5 @@
> >        if (testWindow.document.readyState == "complete") {
> >          TestRunner.currentTestURL = testPath;
> >          TestRunner.updateUI(testWindow.SimpleTest._tests);
> >          testWindow.close();
> > +        if (TestRunner.loops == completed  && TestRunner.onComplete) {
> 
> is there any chance TestRunner.loops < completed?  I don't see the need for
> this logic if we have numLoops being decremented.  Could you just explain
> the need for the completed variable.

I reorganised that code so it is clearer. Say loops=n, then we use 'numLoops' just to kick off n calls to checkComplete. checkComplete will either a) create the test window or b) find the currently running test window and check if the test has completed. If so, it will close the test window and mark the test as complete using the 'completed' variable, then check if all the tests have be run, in which case it will execute the onComplete function, if it exists. If the test is not complete, it will call itself again in a second to repeat the check. TestRunner.loops should never be less than completed since there will only ever be n checkComplete calls that successfully increment the completed variable.

Comment 7

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

Comment on attachment 555772 [details] [diff] [review]
URL and file path fixes, and allow mochiplain loop tests to have an onComplete function

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

looks good for fixing single mochitest looping!

Comment 8

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/7918fee254ae

Comment 9

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

ok, cleaned up comment and user:
http://hg.mozilla.org/integration/mozilla-inbound/rev/fdf4c318a165

Comment 10

[Ed Morley [:emorley]]

Initial landing: http://hg.mozilla.org/mozilla-central/rev/7918fee254ae
Backout due to lack of author / bug number in commit message: http://hg.mozilla.org/mozilla-central/rev/77e5f71a78e4
Successful re-landing: http://hg.mozilla.org/mozilla-central/rev/fdf4c318a165

Malinas, to save accidental landings without author set, you may find it handy to set up your hgrc to set that field automatically :-)
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Comment 11

[Ed Morley [:emorley]]

Sorry, brain mal.
s/Malinas/Malini/

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 555772 [details] [diff] [review]
URL and file path fixes, and allow mochiplain loop tests to have an onComplete function

>--- a/testing/mochitest/tests/SimpleTest/TestRunner.js
>+++ b/testing/mochitest/tests/SimpleTest/TestRunner.js
>+  function checkComplete() {
>+    var testWindow = window.open(testPath, 'test window'); // kick off the test or find the active window
>+    if (testWindow.document.readyState == "complete") {

Not your code, but could you file a bug to make this use onreadystatechange?

Comment 13

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

(In reply to Ed Morley [:edmorley] from comment #11)
> Sorry, brain mal.
> s/Malinas/Malini/

Thanks Ed, I didn't know of this

Comment 14

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

(In reply to Ms2ger from comment #12)
> Comment on attachment 555772 [details] [diff] [review]
> URL and file path fixes, and allow mochiplain loop tests to have an
> onComplete function
> 
> >--- a/testing/mochitest/tests/SimpleTest/TestRunner.js
> >+++ b/testing/mochitest/tests/SimpleTest/TestRunner.js
> >+  function checkComplete() {
> >+    var testWindow = window.open(testPath, 'test window'); // kick off the test or find the active window
> >+    if (testWindow.document.readyState == "complete") {
> 
> Not your code, but could you file a bug to make this use onreadystatechange?

Sounds good. Bug 682347