Closed Bug 1455400 Opened 2 years ago Closed 2 years ago

Upload per-test coverage reports as artifacts


(Testing :: Code Coverage, enhancement)

Not set


(Not tracked)



(Reporter: marco, Assigned: marco)




(1 file, 1 obsolete file)

With bug 1454629 we have the infrastructure to generate them, but we are not uploading them yet.
Depends on: 1431753
No longer depends on: 1454629
Assignee: nobody → mcastelluccio
Attached patch Patch (obsolete) — Splinter Review
For now, the output format is the coveralls one. I think it will be faster to handle than jsdcov, as we don't need to do any translation (we already have the coveralls format as it's the output of grcov).

I could add jsdcov support to grcov, but I think that the coveralls format will also make it easier to diff with the baseline (also because the coveralls format contains hit counts, the jsdcov format doesn't).

This will need some changes on ActiveData (namely, adding a new reader), but it should be pretty easy as it's just JSON.
Attachment #8970384 - Flags: review?(gmierz2)
Comment on attachment 8970384 [details] [diff] [review]

Review of attachment 8970384 [details] [diff] [review]:

Good work! Just some questions/comments that need to be addressed and changes made accordingly if needed.

::: testing/mozharness/mozharness/mozilla/testing/
@@ +260,5 @@
> +        with open(grcov_file, 'r') as f:
> +            report = json.load(f)
> +        os.remove(grcov_file)
> +
> +        # TODO: Diff this coverage report with the baseline one

I think we should do the diffing in the final phase - during the post-action listener. Otherwise, we will have to enforce that the baseline test is always run first so that we can diff it with all the next tests. And we might also have different suites running in a single chunk that would require a specific baseline to be subtracted.

@@ +265,5 @@
> +
> +        dest = os.path.join('per-test-coverage-reports', str(uuid.uuid4()) + '.json')
> +
> +        with open(dest, 'w') as f:
> +            json.dump({'test': test, 'report': report}, f)

Is it possible to add the flavor (suite/sub-suite) of the test to this JSON? At least as an intermediate data point before the diffing. I think it'll make it much easier to find the correct baseline's for each test when multiple suites are run in the same TC chunk.

@@ +291,5 @@
>          if not self.code_coverage_enabled:
>              return
>          if self.per_test_coverage:

In here is where I think we should diff the baseline's with the reports.

::: testing/mozharness/scripts/
@@ +853,5 @@
>                  # full coverage of the test and the baseline coverage and only get
>                  # the coverage data specific to the test.
>                  if self.per_test_coverage:
>                      gcov_dir, jsvm_dir = self.set_coverage_env(env)
>                      # TODO: Run basic startup/shutdown test to collect baseline coverage.

If we remove the baseline later in the post-action listener, we can remove this TODO as well. Same for 347-349 in
Attachment #8970384 - Flags: review?(gmierz2) → review-
Attached patch PatchSplinter Review
Attachment #8970384 - Attachment is obsolete: true
Attachment #8970757 - Flags: review?(gmierz2)
With these changes test-coverage should now output some per-test JSONs, even though they are not diffed. The patch looks great to me, but can you test it out on try before we land it?
Flags: needinfo?(mcastelluccio)
Attachment #8970757 - Flags: review?(gmierz2) → review+
(In reply to Greg Mierzwinski [:sparky] from comment #4)
> With these changes test-coverage should now output some per-test JSONs, even
> though they are not diffed. The patch looks great to me, but can you test it
> out on try before we land it?

Here's the (green) try build:
It's only on Linux, but I wouldn't expect the Windows build to be different.
Flags: needinfo?(mcastelluccio)
This looks great to me! Two tests are run in that try push in test-coverage (TC) and two JSONs are uploaded in the artifact as well.
Landed with the wrong bug number...
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.