1471575 - Reset gcov counters before every test and dump them before shutdown in per-test coverage mode (for reftest)

Status: RESOLVED FIXED

(Testing :: Code Coverage, enhancement)

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Patch

I've removed the suite_category check in desktop_unittest.py, as now we support all suites supported by per-test (right?).

Comment 2

[Marco Castelluccio [:marco]]

I've tested the patch with a crashtest, it reduces the covered lines by around 50%.
A full try run is green.

Comment 3

[Greg Mierzwinski [:sparky]]

Comment on attachment 8992586 [details] [diff] [review]
Patch

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

Other than these small issues, it looks good to me.

I am pretty sure we can do everything normal linux debug builds do and that this will be fine: https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/test/test-platforms.yml#41-43,111-114
Unless something is disabled completely from somewhere other than here: https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/tests.py?q=enable_code_coverage&redirect_type=single#705

Can you test the normal linux ccov build with reftest and/or crashtest with these changes and post the treeherder link?

::: layout/tools/reftest/bootstrap.js
@@ +62,4 @@
>      // Close pre-existing window
>      orig.close();
>  
> +    ChromeUtils.import("chrome://reftest/content/PerTestCoverageUtils.jsm");

Does this import need to be here or can it be moved up to line 8-9? It looks like this will be imported all the time any way.

::: layout/tools/reftest/reftest.jsm
@@ +517,5 @@
> +        PerTestCoverageUtils.beforeTest()
> +        .then(StartCurrentTest)
> +        .catch(e => {
> +            logger.error("EXCEPTION: " + e);
> +            DoneTests();

It looks like this catch is doing the same thing as the one below (starting at line 524). Is there a reason for catching the error here?

@@ +765,1 @@
>      if (g.manageSuite) {

This (line 765 to line 786) should be indented by 4 spaces.

Comment 4

[Marco Castelluccio [:marco]]

(In reply to Greg Mierzwinski [:sparky] from comment #3)
> Can you test the normal linux ccov build with reftest and/or crashtest with
> these changes and post the treeherder link?

I've done a full try build with all debug configs and it was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3584b033ad907610cdf7cf1e59b2ff6923bba363 (ignore the Windows failure, it was due to another patch).

> 
> ::: layout/tools/reftest/bootstrap.js
> @@ +62,4 @@
> >      // Close pre-existing window
> >      orig.close();
> >  
> > +    ChromeUtils.import("chrome://reftest/content/PerTestCoverageUtils.jsm");
> 
> Does this import need to be here or can it be moved up to line 8-9? It looks
> like this will be imported all the time any way.

For some reason we can't import it at the beginning, I haven't figured out why. Maybe we have to import it after reftest.jsm is imported, I'm not sure.

> 
> ::: layout/tools/reftest/reftest.jsm
> @@ +517,5 @@
> > +        PerTestCoverageUtils.beforeTest()
> > +        .then(StartCurrentTest)
> > +        .catch(e => {
> > +            logger.error("EXCEPTION: " + e);
> > +            DoneTests();
> 
> It looks like this catch is doing the same thing as the one below (starting
> at line 524). Is there a reason for catching the error here?

It isn't the same kind of catch. The other one is catching JavaScript exceptions, this one is to handle Promise rejections (https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Promise/catch).

> 
> @@ +765,1 @@
> >      if (g.manageSuite) {
> 
> This (line 765 to line 786) should be indented by 4 spaces.

The diff is with "-w" to avoid whitespace changes to show up, but I actually changed the indentation here (see https://hg.mozilla.org/try/rev/e3600a180b780dae2d0dafd28cac458eefb9a474#l3.53 from my try push).

Comment 5

[Greg Mierzwinski [:sparky]]

Comment on attachment 8992586 [details] [diff] [review]
Patch

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

Ok, sounds good, this is good to go for me.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1de786134710
Reset/dump gcov counters before/after reftest tests. r=sparky

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/1de786134710