Closed Bug 1357520 Opened 3 years ago Closed 3 years ago

Add a test verification mode to the mochitest harness

Categories

(Testing :: General, enhancement)

enhancement
Not set

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: gbrown, Assigned: gbrown)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.)
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?
Attachment #8861209 - Flags: feedback?(jmaher)
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.
Attachment #8861209 - Flags: feedback?(jmaher) → feedback-
(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.)
(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.
(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 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
Attachment #8885514 - Flags: review+
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.
Duplicate of this bug: 1371782
Attached patch minor changesSplinter Review
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
Attachment #8890527 - Flags: review?(jmaher)
Comment on attachment 8890527 [details] [diff] [review]
minor changes

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

thanks for making this easy to understand.
Attachment #8890527 - Flags: review?(jmaher) → review+
Pushed by gbrown@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/04d632a827e9
https://hg.mozilla.org/mozilla-central/rev/170df6e06a4c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.