875463 - Support --run-until-failure option for mochitests

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

This is basically a smarter --repeat which automatically sets --repeat to 30 but stops running a test on the first failure to avoid unnecessary work and also cascaded errors that are irrelevant.

Any objections? Does it sound like a good idea? I've got the patches ready ;)

When implementing this I found that --repeat was broken in various ways so the first two patches are fixes to it, and the third patch is the actual implementation of the feature.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Fix single reloop

TestRunner would throw an error while relooping a single file because in that case the condition

>    if (TestRunner._currentTest == TestRunner._lastTestFinished) {

would match. It worked properly when looping through a folder with more than 1 test

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Remove plain-loop.html

TestRunner.loopTest (which was used by plain-loop.html) was very broken and could enter in an infinite loop for async tests.

plain-loop was a bit silly in the first place, there is no need to go through a different path when running a single test multiple times. In that case, we now open the folder index and set gTestList to the test wanted.

A single test with no --repeat stills runs directly from its file without the frame used by the folder index

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Run until failure

This adds the --run-until-failure option in runtests.py (and mach), and implements it in the mochitest runners

It automatically sets repeat to 29 (meaning 30 iterations), which can be manually increased or decreased by setting the wanted value through the --repeat option

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Watching try to make sure I didn't break anything: https://tbpl.mozilla.org/?tree=Try&rev=532de64362bf

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 753434 [details] [diff] [review]
Fix single reloop

See Comment 1 for the reason for this patch. Also, _currentLoop now starts from 1 instead of 0 to properly display the two info msgs:
"START Loop 2"
"Ran 3 Loops"

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 753443 [details] [diff] [review]
Run until failure

gps for the mach changes: verifyOptions was being called way too early, while most of the options weren't assigned yet. I'm guessing that wasn't intentional, but just double checking..  Without this a lot of the logic from runtests.py's verifyOptions is skipped

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 753443 [details] [diff] [review]
Run until failure

Mach commands are mostly under the purview of the module they are affiliated with. I trust jmaher to conduct an adequate review of the mach_commands.py changes.

Comment 8

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

Comment on attachment 753439 [details] [diff] [review]
Remove plain-loop.html

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

Lots of code removal, great!

::: testing/mochitest/runtests.py
@@ +527,5 @@
>      """ Build the url path to the specific test harness and test file or directory """
>      testHost = "http://mochi.test:8888"
>      testURL = ("/").join([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.repeat > 0:
> +       testURL = ("/").join([testHost, self.TEST_PATH, os.path.dirname(options.testPath)])

I believe we can just remove the entire if clause here.

Comment 9

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

Comment on attachment 753443 [details] [diff] [review]
Run until failure

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

::: testing/mochitest/runtests.py
@@ +384,5 @@
>          self.error("%s not found, cannot launch immersive tests." %
>                     mochitest.immersiveHelperPath)
>  
> +    if options.runUntilFailure:
> +      if not os.path.isfile(os.path.join(mochitest.oldcwd, os.path.dirname(__file__), mochitest.TEST_PATH, options.testPath)):

does this only work for mochitest-plain?  mochitest.TEST_PATH = 'tests'

Comment 10

[:Felipe Gomes (needinfo for replies!)]

(In reply to Joel Maher (:jmaher) from comment #9) 
> does this only work for mochitest-plain?  mochitest.TEST_PATH = 'tests'

hmm indeed, that's true. I added this "enforce it's a single file" condition at the end and only tested it with m-plain. Is there any simple way that I can test that in a way that covers all types of tests?

Comment 11

[:Felipe Gomes (needinfo for replies!)]

(In reply to Joel Maher (:jmaher) from comment #8)
> > +       testURL = ("/").join([testHost, self.TEST_PATH, os.path.dirname(options.testPath)])
> 
> I believe we can just remove the entire if clause here.

This clause is now used to make the testURL stop at the directory instead of pointing direct to the file, so that server.js can kick in with its directory listing.

Comment 12

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

I still think there is a problem with the single file check, if we have repeat we will not fail if it is a directory.  I think there is some logic to clean up there.

to work on all test types we might want to call getTestRoot() and that should get what you need!

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Attachment: Run until failure

(In reply to Joel Maher (:jmaher) from comment #12)
> I still think there is a problem with the single file check, if we have
> repeat we will not fail if it is a directory.  I think there is some logic
> to clean up there.

Oh yeah, it will not fail, but that's intentional. Repeat works fine for directories too
 
> to work on all test types we might want to call getTestRoot() and that
> should get what you need!

Thanks, worked perfectly!

Comment 14

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

Comment on attachment 753887 [details] [diff] [review]
Run until failure

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

thanks!

Comment 15

[:Felipe Gomes (needinfo for replies!)]

I merged the two first patches in the first commit:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6e62a0998e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca68e7cec507

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a6e62a0998e6
https://hg.mozilla.org/mozilla-central/rev/ca68e7cec507