Allow mochitest to run in loops for debugging intermittent tests

RESOLVED FIXED

Status

Testing
Mochitest
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: cmtalbert, Assigned: mdas)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 482413 [details] [diff] [review]
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.
Attachment #482413 - Flags: feedback?(ted.mielczarek)
(Reporter)

Comment 1

8 years ago
Created attachment 482751 [details] [diff] [review]
And now for reftests!

And now we have a WIP for reftest looping.  Still needs testing on JSReftest before I ask for review.
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 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.
Attachment #482413 - Flags: feedback?(ted.mielczarek) → feedback+
Clint, this is a very helpful change to our test infrastructure, what's left to get this landed?
Assignee: nobody → mdas
(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)
No longer blocks: 669445
Created attachment 544885 [details] [diff] [review]
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.
Attachment #482413 - Attachment is obsolete: true
Attachment #544885 - Flags: review?(ctalbert)
(Reporter)

Comment 7

7 years ago
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!
Attachment #544885 - Flags: review?(ctalbert) → review+
Created attachment 545241 [details] [diff] [review]
Enable looping for mochitests! (Updated after review)

Added a more detailed help message for the "--loops" option
Attachment #544885 - Attachment is obsolete: true
Attachment #545241 - Flags: review+
(Reporter)

Comment 9

7 years ago
Created attachment 545723 [details] [diff] [review]
take 3

Additional patch fixing a onComplete issue we found.
Attachment #545241 - Attachment is obsolete: true
Attachment #545723 - Flags: review+
Created attachment 545747 [details] [diff] [review]
Take 4 - fix Makefile conflict
Attachment #545723 - Attachment is obsolete: true
(Reporter)

Comment 11

7 years ago
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.
Let's move the reftest stuff to a new bug, since this bug explicitly talks about Mochitest.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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?
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.
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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 546914 [details]
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.
Created attachment 546915 [details] [diff] [review]
Let chrome single tests and test dirs go through the same code path
Attachment #546914 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
(Sorry, bug fumbling.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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]:
-----------------------------------------------------------------
Attachment #546915 - Flags: review?(ctalbert)
(Reporter)

Comment 21

7 years ago
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.
Attachment #546915 - Flags: review?(ctalbert) → review+
(Reporter)

Comment 22

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/5c7a49dfa7f7
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
I think you meant fossil ;) I'll clear that old comment in an upcoming change that touches a lot of mochitests.
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!
(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.
You need to log in before you can comment on or make changes to this bug.