1357520 - Add a test verification mode to the mochitest harness

Status: RESOLVED FIXED

(Testing :: General, enhancement)

Description

[Geoff Brown [:gbrown]]

I see this as a simple harness flag that triggers a "test verification" mode of operation. Ideas for how to verify a test include:

 - start the browser, repeat N times(run specified test), shutdown
 - repeat N times(start the browser, run specified test, shutdown)
 - repeat N times(start the browser, run the directory or manifest of tests, shutdown)
 - run the test in different modes, or with additional run-time checks enabled
 - check how long the test runs and warn or fail if the test duration exceeds some threshold

There's value in all these ideas (and others), and there is value in larger values of N, but that needs to be balanced against run time: If it takes too long, it won't be useful. Some experimentation is required.

We already have flags like --repeat and --run-until-failure and I'm not convinced those are being used very often, so I prefer a simple, one-size-fits-all flag over a bunch of separate options: "--verify <test-name>", perhaps.

(We'll eventually want a similar feature for all test harnesses, but let's start with mochitest.)

Comment 1

[Geoff Brown [:gbrown]]

Attachment: work in progress - add --verify to 'mach mochitest'

This is just an initial attempt at this. It needs more testing, refinement, etc. but I'm relatively happy with the simplest use case:

$ ./mach mochitest caps/tests/mochitest/test_bug246699.html --verify
 0:02.13 #=================================================
 0:02.13 ### Now verifying /home/gbrown/objdirs/firefox/_tests/testing/mochitest/tests/caps/tests/mochitest/test_bug246699.html...
 0:02.13 #=================================================
 0:02.13 #=================================================
 0:02.13 # Running test 10 times, each in a separate browser...
 0:02.13 #=================================================
...
 0:42.46 #=================================================
 0:42.46 # Running test 100 times, in one browser...
 0:42.46 #=================================================
...
 0:50.69 #=================================================
 0:50.69 # Running directory caps/tests/mochitest 10 times, in one browser...
 0:50.69 #=================================================
...
0 INFO TEST-START | Shutdown
1 INFO Passed:  4686
2 INFO Failed:  0
3 INFO Todo:    0
4 INFO Mode:    e10s
5 INFO SimpleTest FINISHED
Buffered messages finished
SUITE-END | took 8s
 1:00.25 #=================================================
 1:00.25 ### Test verification complete. All tests passed.
 1:00.25 #=================================================

Thoughts?

Comment 2

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

Comment on attachment 8861209 [details] [diff] [review]
work in progress - add --verify to 'mach mochitest'

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

this is fairly simple, and sort of nice.  I would prefer this be outside of mach so that it would be easier to run as a job in treeherder when we detect test files have changed.  With that said, maybe we should avoid adding this to runtests.py and wrap that with a script?  Lastly what if there are >1 file changed how would we handle that (ok to handle in the second round)?

A good start, I really like to 3 different tests to do and some of the bounds around there.

Comment 3

[Geoff Brown [:gbrown]]

(In reply to Joel Maher ( :jmaher) from comment #2)
> I would prefer this be outside of
> mach so that it would be easier to run as a job in treeherder

I initially tried to do this in runtests.py, but I found the mach TestResolver was quite useful.

I was thinking that the taskcluster job could invoke mach. We have tc mach support now, for lint jobs and such. Unfortunately, that doesn't do everything we need for mochitest --verify: mach is available, and the source checkout is available, but without a build, there is no mach "context" and no build to test against. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cc12041605a58c917f7bcef86df75749a89886b

> Lastly what if there are >1
> file changed how would we handle that (ok to handle in the second round)?

That's handled, but perhaps not fully. For instance, the max time applies to the whole run rather than a test, so it is possible for a long-running initial test to use up all the time and prevent verification of later tests. (On the other hand, this could be seen as a useful safeguard, preventing the verify job from running forever when, say, hundreds of tests are modified in one push.)

Comment 4

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #3)
> I was thinking that the taskcluster job could invoke mach. We have tc mach
> support now, for lint jobs and such. Unfortunately, that doesn't do
> everything we need for mochitest --verify: mach is available, and the source
> checkout is available, but without a build, there is no mach "context" and
> no build to test against.

On the other, other hand, maybe that could be fixed: Consider that the "run-wizard" accessed from the one-click loaner successfully sets up a "limited mach environment" based on the build artifact and test zips.

Comment 5

[Geoff Brown [:gbrown]]

Attachment: support --verify in runtests.py

Comment 6

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #4)
> On the other, other hand, maybe that could be fixed: Consider that the
> "run-wizard" accessed from the one-click loaner successfully sets up a
> "limited mach environment" based on the build artifact and test zips.

I had a closer look at that and realized that environment is *very* limited, and I don't see an easy way of overcoming that.

I have come around to agreeing that test verification should be supported in runtests.py itself, and be independent of mach.

Comment 7

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

Comment on attachment 8885514 [details] [diff] [review]
support --verify in runtests.py

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

I would be happy to see this land as is- my comments in this review would only be concerning if we are in a situation where we don't back the patch out for failing.

::: testing/mochitest/runtests.py
@@ +2339,5 @@
> +            stepOptions.keep_open = False
> +            for i in xrange(VERIFY_REPEAT_SINGLE_BROWSER):
> +                result = self.runTests(stepOptions)
> +                self.message_logger.finish()
> +                if result != 0:

do we want to run it the full set of 10 times instead of stopping on first failure?  If the expectation is 100% pass, then it doesn't matter- if the expectation is fix this soon like a P1, then running the full 10 times would give feedback that this is failing more or less frequent.

@@ +2396,5 @@
> +            self.log.info(':::')
> +            result = step()
> +            if result != 0:
> +                finalResult = "FAILED!"
> +                break

same as above- should we run all tests, or fail on the first

Comment 8

[Geoff Brown [:gbrown]]

I think/hope that, once all the test verification pieces come together, we would back out a patch for failing test verification on treeherder.

I consciously wrote the patch as fail-fast, so that, when a test modification introduces a problem, test-verification finds it and reports a failure promptly. I am also a little concerned about the case where a test times out with high frequency; we risk exceeding the task timeout, for little benefit.

Comment 10

[Geoff Brown [:gbrown]]

Attachment: minor changes

Without these changes, I get a timeout in step 2 when I verify a single plain mochitest: The --keep-open=False option is lost, so the browser remains open until timeout. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7aa706fa40ac46063ecd5e5f559fc46a49cce57

With these changes, all is well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05eaa08f562dec01201d8f215da112e31d657ef7

Comment 11

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

Comment on attachment 8890527 [details] [diff] [review]
minor changes

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

thanks for making this easy to understand.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04d632a827e9
Support --verify option in mochitest harness; r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/170df6e06a4c
Minor changes for mochitest verifications; r=jmaher

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/04d632a827e9
https://hg.mozilla.org/mozilla-central/rev/170df6e06a4c