Closed
Bug 1488849
Opened 6 years ago
Closed 6 years ago
Chunks with no tests fail when run by 'mach try coverage'
Categories
(Testing :: Code Coverage, enhancement)
Tracking
(firefox65 fixed)
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: gabriel-v, Assigned: marco)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
ahal
:
review+
|
Details | Diff | Splinter Review |
When running single tests from some suites on multiple platforms, some chunks turn up empty (because the requested test is skipped or disabled on that respective platform). Running them will fail with "no tests to run" or similar messages. They are harmless, but checking each one of them each time is a manual process.
Should we turn all of these "no tests to run outcomes" green?
We could probably avoid scheduling these tasks before running them, by checking the manifests in 'mach try coverage'.
Failing try push made with 'mach try coverage': https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c1d25b7e725baa5ec9525bee31310448271dcff&selectedJob=196921111
Comment 1•6 years ago
|
||
Checking the manifests locally for every suite isn't going to work because the keys used to filter tests depends on things like platform, build type, e10s, etc.
I'd suggest we try to detect when we're running in 'code coverage' mode and ignore that error if we are. I also think this is somewhat less important and can probably be left as a follow-up bug if you want.
Assignee | ||
Comment 2•6 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #1)
> I'd suggest we try to detect when we're running in 'code coverage' mode and
> ignore that error if we are. I also think this is somewhat less important
> and can probably be left as a follow-up bug if you want.
I'm thinking of just adding a new environment variable that says how a task was scheduled (TRYSELECTOR_NAME), and then check it in mozharness and fail with `No tests run ...` only if TRYSELECTOR_NAME is not "coverage".
Sounds good? Do you have any other option in mind?
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(ahal)
Assignee | ||
Comment 3•6 years ago
|
||
(I was also thinking I could use TRY_COMMIT_MSG, but it seems to be empty for both fuzzy and coverage)
Comment 4•6 years ago
|
||
Yeah, I like that solution! It would be pretty simple to implicitly add that env for every |mach try| push. We could put the commit message in the env as well if we like. Though could we just call it TRY_SELECTOR?
Is TRY_COMMIT_MSG populated when using ./mach try syntax? How about a vanilla `hg push try`? If so, then yeah, maybe it would be a good idea to have this env populated for all our |mach try| selectors anyway.
Flags: needinfo?(ahal)
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #4)
> Yeah, I like that solution! It would be pretty simple to implicitly add that
> env for every |mach try| push. We could put the commit message in the env as
> well if we like. Though could we just call it TRY_SELECTOR?
Done!
> Is TRY_COMMIT_MSG populated when using ./mach try syntax? How about a
> vanilla `hg push try`? If so, then yeah, maybe it would be a good idea to
> have this env populated for all our |mach try| selectors anyway.
It is set for `mach try syntax` to the try syntax string (e.g. "try: -b o -p linux64 -u none -t none").
Anyway, thinking about it more, I feel better to have a separate TRY_SELECTOR env rather than relying on checking for substrings in the commit message (which could contain the substring for other reasons).
Comment 6•6 years ago
|
||
Comment on attachment 9028243 [details] [diff] [review]
Patch
Review of attachment 9028243 [details] [diff] [review]:
-----------------------------------------------------------------
r+ with nits addressed
::: testing/mozharness/mozharness/mozilla/building/buildbase.py
@@ +149,5 @@
> levels=TBPL_WORST_LEVEL_TUPLE)
>
> # Account for the possibility that no test summary was output.
> + if (self.pass_count == 0 and self.fail_count == 0 and
> + 'coverage' != os.environ.get('TRY_SELECTOR', '""')):
nit: this reads a bit awkwardly to me, also there's no need to specify a default value to 'get'. How about:
os.environ.get('TRY_SELECTOR') != 'coverage'
Ditto for the other two locations.
Attachment #9028243 -
Flags: review?(ahal) → review+
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9883f547e280
Don't fail when a chunk is not running any test when the try selector is 'coverage'. r=ahal
Comment 8•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in
before you can comment on or make changes to this bug.
Description
•