603517 - Allow mochitest to run in loops for debugging intermittent tests

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[cmtalbert]

Attachment: First WIP for looping mochitests

This will allow mochitest to run in loops without restarting the browser so that it can run for long periods of time over a set of tests in hopes of causing intermittent failures to surface.

Works for mochi-plain and mochi-chrome (about to test, but need to build first).  I think I'll have to add some more crazy sauce to get this to work for b-c tests.

Once I have something that works in all three harness variations I'll ask for review, right now, feedback on the approach is appreciated.

Comment 1

[cmtalbert]

Attachment: And now for reftests!

And now we have a WIP for reftest looping.  Still needs testing on JSReftest before I ask for review.

Comment 2

[Johnny Stenback (:jst)]

FWIW, these patches work wonders for me! Did exactly what I needed, and found a way to reproduce the very intermittent bug 603270! Thanks again Clint for coming up with these patches with no time to prepare!

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 482413 [details] [diff] [review]
First WIP for looping mochitests

>diff --git a/testing/mochitest/tests/SimpleTest/TestRunner.js b/testing/mochitest/tests/SimpleTest/TestRunner.js
>--- a/testing/mochitest/tests/SimpleTest/TestRunner.js
>+++ b/testing/mochitest/tests/SimpleTest/TestRunner.js
> /**
>+ * Used for running a set of tests in a loop for debugging purposes
>+ * Takes an array of URLs
>+**/
>+TestRunner.resetTests = function(listURLs) {
>+  TestRunner._currentTest = 0;
>+  // Reset our "Current-test" line - functionality depends on it
>+  $("current-test").innerHTML = '<b>Currently Executing: <span id="current-test-path">_</span></b>';

I worry a little bit that this could get out of sync with the other definition. Can you either a) fix the other code's assumptions, b) save the original value of this and just restore it instead of duplicating the contents here, or c) move this into a common function that regular startup and this can both call?

>-            TestRunner.logger.log("SimpleTest FINISHED");
>+            // If we are looping, don't send this cause it closes the log file
>+            if (!TestRunner.loops) 

I'd prefer if (TestRunner.loops == 0) here, since it's an integer.

>+              TestRunner.logger.log("SimpleTest FINISHED");

Looks fine otherwise.

Comment 4

[Johnny Stenback (:jst)]

Clint, this is a very helpful change to our test infrastructure, what's left to get this landed?

Comment 5

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

(In reply to comment #4)
> Clint, this is a very helpful change to our test infrastructure, what's left
> to get this landed?

This patch does not yet allow the running of single chrome tests, and still has some bugs regarding the gathering and displaying all the test summaries of looped single tests for mochiplain. I'm testing to see if any other functionality is missing and will be fixing it accordingly.

Further work is required for browser-chrome and JSReftests to work, alongside testing to make sure looping works for all cases (ex: looping the tests in a directory, looping a single test)

Comment 6

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

Attachment: Enable looping for mochitests!

This patch allows plain, chrome and browser-chrome Mochitests to loop using the '--loops' option. It works with both directories of tests as well as single test files. Test summaries are also provided upon failure or if a todo test had a result. The summary table will be generated at the end of the main test page and will be populated with what test failed, the test name, and the error message.

In response to Ted's suggestion about the current-test, I think the correct thing to do would be to change the update mechanism because having a listener on a div sounds like the wrong approach to start with (suggestion a) ). Obviously that's outside the scope of this patch, so I went with suggestion b, since suggestion c would be nice, but it would be going less than halfway since it would be nicer if we can refactor our html generation so that both server.js, TestRunner.js, harness-overlay.xul and the new plain-loop.html file can share table generation code, but again, I felt this was outside the scope.

The reftests patch have not yet been verified or updated.

Comment 7

[cmtalbert]

Comment on attachment 544885 [details] [diff] [review]
Enable looping for mochitests!

This is a really well done first patch.  

My only nit is that if I specify --loops=3 I'd expect it to run three times.  But as implemented it will run 4 because it's zero based rather than 1 based.  That's quite minor though, and could be solved either through a help message or removing the = on the loop controls for the loops. (loops >= 0) {...} etc.

Everything passed (so far) on try, but I don't expect any problems.  I think this looks good. I think we can land this monday after the rest of the try results come in.

Great job!

Comment 8

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

Attachment: Enable looping for mochitests! (Updated after review)

Added a more detailed help message for the "--loops" option

Comment 9

[cmtalbert]

Attachment: take 3

Additional patch fixing a onComplete issue we found.

Comment 10

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

Attachment: Take 4 - fix Makefile conflict

Comment 11

[cmtalbert]

Landed attachment 545747 [details] [diff] [review] on m-c:
http://hg.mozilla.org/mozilla-central/rev/52617959b48e

Leaving bug open to track landing the reftest changes next.

Comment 12

[(not currently active) Ted Mielczarek]

Let's move the reftest stuff to a new bug, since this bug explicitly talks about Mochitest.

Comment 13

[Nicholas Nethercote [inactive]]

This is causing big problems for me.  If I run this:

  python runtests.py --chrome --test-path=toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul

Instead of getting the usual single-test view with "toggle passed checks" et al at the top, I get the multi-test view with status/passed/failed/todo at the top along with the "Run Chrome Tests".  This is kind of annoying, but I can live with it.

The *much* bigger problem is that when I alter test_aboutmemory.xul so it should fail, the multi-test view does *not* say that I failed.  That test has three checks, instead of eg. saying "1 passed, 2 failed" it just says "1 passed"!  But if I then click on the "chrome://mochitests/content/chrome/toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul" link at the bottom it runs the test again, this time it shows the single-test view and says "1 passed, 2 failed".

So something is badly broken in the failure detection when you run a single test.  If you want to reproduce this, modify the value of |amExpectedText| in toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul, and run with the command above.  You should get one failure, but you won't.

Should this patch be backed out because of this problem?

Comment 14

[Nicholas Nethercote [inactive]]

FWIW, when I run like this:

  TEST_PATH=toolkit/components/aboutmemory make -C $OBJ mochitest-chrome

The failure is detected as expected.  So it seems to be only a problem in the run-one-test mode of the chrome harness.

Comment 15

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

I think the problem is that when we run a single test we execute a much different code path than as a directory.  This patch adds a TestRunner dependency to the single test case (which is good), but we need to update all the other pieces to work as it was inside the harness wrapper.

Comment 16

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

We will be fixing this bug by uniting the codepaths taken for single and directory-run chrome and browser-chrome tests. Instead of returning the single file path separately during processing, we will return the directory it contains and then run the requested test from the file listing, and this would allow us to get the correct test data.

Mochiplain tests will undergo this change following Bug 508664.

Comment 17

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

Attachment: Let chrome single tests and test dirs go through the same code path

This should fix the reporting problem by having the chrome single tests follow the same path as test directories. 

Updating the last comment: Browser-chrome already neglects single test paths and treats them all like directories (http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-harness.xul#201)

I'm currently testing this patch in try.

Comment 18

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

Attachment: Let chrome single tests and test dirs go through the same code path

Comment 19

[Cameron McCormack (:heycam)]

(Sorry, bug fumbling.)

Comment 20

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

Comment on attachment 546915 [details] [diff] [review]
Let chrome single tests and test dirs go through the same code path

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

Comment 21

[cmtalbert]

Comment on attachment 546915 [details] [diff] [review]
Let chrome single tests and test dirs go through the same code path

r+ wow, that's a pretty straight forward patch.  Thanks.

Comment 22

[cmtalbert]

Landed: http://hg.mozilla.org/mozilla-central/rev/5c7a49dfa7f7

Comment 23

[(not currently active) Ted Mielczarek]

I was reading the changeset and noticed this gem in the context:
http://hg.mozilla.org/mozilla-central/annotate/6df31af4cca6/testing/mochitest/chrome-harness.js#l218

:)

Comment 24

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

I think you meant fossil ;) I'll clear that old comment in an upcoming change that touches a lot of mochitests.

Comment 25

[Nicholas Nethercote [inactive]]

A follow-up to comment 13:  when I run a single test I now get the multi-test summary screen (with status/passed/failed/todo) but no tests run automatically.  I then have to click on my tests link at the bottom of the page, or the "run chrome tests" button at the top of the page, to actually run the test.  And if the test fails, it's detected correctly.

So it's a change in behaviour but a reasonable one.  Thanks for fixing!

Comment 26

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

(In reply to comment #25)
> A follow-up to comment 13:  when I run a single test I now get the
> multi-test summary screen (with status/passed/failed/todo) but no tests run
> automatically.  I then have to click on my tests link at the bottom of the
> page, or the "run chrome tests" button at the top of the page, to actually
> run the test.  And if the test fails, it's detected correctly.
> 
> So it's a change in behaviour but a reasonable one.  Thanks for fixing!

Glad to hear it's working. If you don't want to hit the 'Run Chrome Tests' button manually, you can always pass the --autorun parameter to runtests.py and the test will run automatically.