Closed
Bug 603517
Opened 14 years ago
Closed 13 years ago
Allow mochitest to run in loops for debugging intermittent tests
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: cmtalbert, Assigned: mdas)
Details
Attachments
(3 files, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
cmtalbert
:
review+
|
Details | Diff | Splinter Review |
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)
And now we have a WIP for reftest looping. Still needs testing on JSReftest before I ask for review.
Comment 2•14 years ago
|
||
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•14 years ago
|
||
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+
Comment 4•14 years ago
|
||
Clint, this is a very helpful change to our test infrastructure, what's left to get this landed?
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → mdas
Assignee | ||
Comment 5•13 years ago
|
||
(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
Assignee | ||
Comment 6•13 years ago
|
||
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)
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+
Assignee | ||
Comment 8•13 years ago
|
||
Added a more detailed help message for the "--loops" option
Attachment #544885 -
Attachment is obsolete: true
Attachment #545241 -
Flags: review+
Additional patch fixing a onComplete issue we found.
Attachment #545241 -
Attachment is obsolete: true
Attachment #545723 -
Flags: review+
Assignee | ||
Comment 10•13 years ago
|
||
Attachment #545723 -
Attachment is obsolete: true
Reporter | ||
Comment 11•13 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.
Comment 12•13 years ago
|
||
Let's move the reftest stuff to a new bug, since this bug explicitly talks about Mochitest.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 13•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 16•13 years ago
|
||
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 → ---
Assignee | ||
Comment 17•13 years ago
|
||
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.
Assignee | ||
Comment 18•13 years ago
|
||
Attachment #546914 -
Attachment is obsolete: true
Updated•13 years ago
|
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 20•13 years ago
|
||
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•13 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•13 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 23•13 years ago
|
||
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
:)
Assignee | ||
Comment 24•13 years ago
|
||
I think you meant fossil ;) I'll clear that old comment in an upcoming change that touches a lot of mochitests.
Comment 25•13 years ago
|
||
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!
Assignee | ||
Comment 26•13 years ago
|
||
(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.
Description
•