Closed Bug 1488849 Opened Last year Closed 11 months ago

Chunks with no tests fail when run by 'mach try coverage'

Categories

(Testing :: Code Coverage, enhancement)

Version 3
enhancement
Not set

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)

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
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.
Blocks: 1507108
No longer blocks: 1429463
(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?
Flags: needinfo?(ahal)
(I was also thinking I could use TRY_COMMIT_MSG, but it seems to be empty for both fuzzy and coverage)
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)
Attached patch PatchSplinter Review
(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).
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #9028243 - Flags: review?(ahal)
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
Depends on: 1512082
https://hg.mozilla.org/mozilla-central/rev/9883f547e280
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Depends on: 1512146
You need to log in before you can comment on or make changes to this bug.