1380659 - Support dumping/resetting coverage counters from JavaScript

Status: RESOLVED FIXED

(Testing :: Code Coverage, enhancement)

Description

[Marco Castelluccio [:marco]]

Attachment: Patch

This will make it possible to support custom analyses for subperiods of the Firefox execution like the one in bug 1372211.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Part 1: Add new IPC messages to dump/reset coverage counters

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Part 2: Introduce code coverage component to dump/reset coverage counters

Comment 3

[Marco Castelluccio [:marco]]

Attachment: Part 3: Set Bugzilla component for /tools/code-coverage

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Part 4: Add basic xpcshell test for the code coverage component

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Part 5: Add xpcshell test with child and parent process

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Part 6: Add SpecialPowers API to dump/reset coverage counters

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Part 7: Test SpecialPowers API to dump/reset coverage counters

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8886515 [details] [diff] [review]
Part 3: Set Bugzilla component for /tools/code-coverage

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

thanks for making this bugzilla component association

Comment 9

[Marco Castelluccio [:marco]]

Attachment: Part 8: Add TalowPowersService commands to reset/dump coverage counters

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8886518 [details] [diff] [review]
Part 6: Add SpecialPowers API to dump/reset coverage counters

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

looks good!

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8886519 [details] [diff] [review]
Part 7: Test SpecialPowers API to dump/reset coverage counters

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

I enjoy seeing test cases, just one ask!

::: tools/code-coverage/tests/test_coverage_specialpowers.html
@@ +12,5 @@
> +
> +  /** Test for Bug 1380659 **/
> +
> +  SimpleTest.waitForExplicitFinish()
> +  SpecialPowers.requestDumpCoverageCounters();

this tests request, can we test reset as well?  Just calling the api (as this test is currently doing) is enough to get reasonable coverage (no pun intended)

Comment 12

[Marco Castelluccio [:marco]]

(In reply to Joel Maher ( :jmaher) (UTC-8) from comment #11)
> ::: tools/code-coverage/tests/test_coverage_specialpowers.html
> @@ +12,5 @@
> > +
> > +  /** Test for Bug 1380659 **/
> > +
> > +  SimpleTest.waitForExplicitFinish()
> > +  SpecialPowers.requestDumpCoverageCounters();
> 
> this tests request, can we test reset as well?  Just calling the api (as
> this test is currently doing) is enough to get reasonable coverage (no pun
> intended)

In order to do this we would need to run the test in isolation to avoid resetting the coverage counters that were updated by other tests. Is there a way to do that?

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

could you |skip-if = coverage| in the manifest?  Would these tests only be run in coverage mode?  

this set of tests will run in an isolated browser session with a fresh profile- all tests in that mochitest.ini will be isolated.

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Part 6: Add SpecialPowers API to dump/reset coverage counters

Fixed commit message, carrying r+.

Comment 15

[Marco Castelluccio [:marco]]

Attachment: Part 7: Test SpecialPowers API to dump/reset coverage counters

Added call for resetting code coverage to the mochitest. Since the tests are run in isolation, it's fine to do it.

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Part 4: Add basic xpcshell test for the code coverage component

Comment 17

[Marco Castelluccio [:marco]]

Attachment: Part 5: Add xpcshell test with child and parent process

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8887940 [details] [diff] [review]
Part 7: Test SpecialPowers API to dump/reset coverage counters

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

nice and simple!

Comment 19

[Eric Rahm [:erahm]]

Comment on attachment 8886513 [details] [diff] [review]
Part 2: Introduce code coverage component to dump/reset coverage counters

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

This seems fine maybe just some expanded comments. I'm not sure I'm the right person to review but given it's new code and it's kind of xpcom-y I don't mind.

::: ipc/glue/moz.build
@@ -215,5 @@
>  ]
>  
> -if CONFIG['MOZ_CODE_COVERAGE']:
> -    SOURCES += [
> -        'CodeCoverageHandler.cpp',

I guess you moved these? Odd it didn't show up in the patch.

::: tools/code-coverage/moz.build
@@ +27,5 @@
> +    ]
> +
> +    include('/ipc/chromium/chromium-config.mozbuild')
> +
> +    XPCSHELL_TESTS_MANIFESTS += ['tests/xpcshell.ini']

Does this actually exist?

::: tools/code-coverage/nsICodeCoverage.idl
@@ +5,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(57d92056-37b4-4d0a-a52f-deb8f6dac8bc)]
> +interface nsICodeCoverage : nsISupports

Maybe add some comments in here, it's not clear to me what nsICodeCoverage is nor what counters we're messing with.

Comment 20

[Marco Castelluccio [:marco]]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #19) 
> ::: ipc/glue/moz.build
> @@ -215,5 @@
> >  ]
> >  
> > -if CONFIG['MOZ_CODE_COVERAGE']:
> > -    SOURCES += [
> > -        'CodeCoverageHandler.cpp',
> 
> I guess you moved these? Odd it didn't show up in the patch.

Really strange, splinter doesn't show them, but they are in the patch (https://bug1380659.bmoattachments.org/attachment.cgi?id=8886513):
> diff --git a/ipc/glue/CodeCoverageHandler.cpp b/tools/code-coverage/CodeCoverageHandler.cpp
> rename from ipc/glue/CodeCoverageHandler.cpp
> rename to tools/code-coverage/CodeCoverageHandler.cpp
> diff --git a/ipc/glue/CodeCoverageHandler.h b/tools/code-coverage/CodeCoverageHandler.h
> rename from ipc/glue/CodeCoverageHandler.h
> rename to tools/code-coverage/CodeCoverageHandler.h

> ::: tools/code-coverage/moz.build
> @@ +27,5 @@
> > +    ]
> > +
> > +    include('/ipc/chromium/chromium-config.mozbuild')
> > +
> > +    XPCSHELL_TESTS_MANIFESTS += ['tests/xpcshell.ini']
> 
> Does this actually exist?

It's added in the xpcshell test patch. I'll move this addition to that patch for consistency.

> ::: tools/code-coverage/nsICodeCoverage.idl
> @@ +5,5 @@
> > +
> > +#include "nsISupports.idl"
> > +
> > +[scriptable, uuid(57d92056-37b4-4d0a-a52f-deb8f6dac8bc)]
> > +interface nsICodeCoverage : nsISupports
> 
> Maybe add some comments in here, it's not clear to me what nsICodeCoverage
> is nor what counters we're messing with.

I will add some comments.

Comment 21

[Marco Castelluccio [:marco]]

Attachment: Part 1: Add new IPC messages to dump/reset coverage counters

Carrying r+.

Comment 22

[Marco Castelluccio [:marco]]

Attachment: Part 2: Introduce code coverage component to dump/reset coverage counters

Comment 23

[Marco Castelluccio [:marco]]

Attachment: Part 3: Set Bugzilla component for /tools/code-coverage

Comment 24

[Marco Castelluccio [:marco]]

Attachment: Part 4: Add basic xpcshell test for the code coverage component

Comment 25

[Marco Castelluccio [:marco]]

Attachment: Part 5: Add xpcshell test with child and parent process

Comment 26

[Marco Castelluccio [:marco]]

Attachment: Part 6: Add SpecialPowers API to dump/reset coverage counters

Comment 27

[Marco Castelluccio [:marco]]

Attachment: Part 7: Test SpecialPowers API to dump/reset coverage counters

Comment 28

[Marco Castelluccio [:marco]]

Attachment: Part 8: Add TalosPowersService commands to reset/dump coverage counters

Comment 29

[Marco Castelluccio [:marco]]

Attachment: Part 9: Move mochitest and xpcshell tests in separate directories

This is a trivial patch, just moving the mochitest and xpcshell tests under separate directories, like we usually do.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5decf9441f50
Add new IPC messages to dump/reset coverage counters. r=kanru
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7686d42d8ad
Introduce code coverage component to dump/reset coverage counters. r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/d771b46e84df
Add basic xpcshell test for the code coverage component. r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9feb5891489
Add xpcshell test with child and parent process, where the parent process requests dump of coverage counters. r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a4c5050b1a6
Add SpecialPowers API to dump/reset coverage counters. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b5f72590fc2
Test SpecialPowers API to dump/reset coverage counters with a mochitest. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/97f0052a4bf9
Add TalowPowersService commands to reset/dump coverage counters. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e581da5ec7f
Set Bugzilla component for /tools/code-coverage. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8e505bfa44
Move xpcshell and mochitest in different directories. r=me

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5decf9441f50
https://hg.mozilla.org/mozilla-central/rev/c7686d42d8ad
https://hg.mozilla.org/mozilla-central/rev/d771b46e84df
https://hg.mozilla.org/mozilla-central/rev/f9feb5891489
https://hg.mozilla.org/mozilla-central/rev/9a4c5050b1a6
https://hg.mozilla.org/mozilla-central/rev/5b5f72590fc2
https://hg.mozilla.org/mozilla-central/rev/97f0052a4bf9
https://hg.mozilla.org/mozilla-central/rev/9e581da5ec7f
https://hg.mozilla.org/mozilla-central/rev/7f8e505bfa44

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

this has a talos improvement in tpaint (measuring window opening times):
== Change summary for alert #8180 (as of July 23 2017 09:28 UTC) ==

Improvements:

  9%  tsvg_static summary windows7-32 opt e10s     64.52 -> 59.02
  4%  tpaint summary osx-10-10 opt e10s            353.15 -> 339.88

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8180

Comment 33

[Marco Castelluccio [:marco]]

(In reply to Joel Maher ( :jmaher) (UTC-8) from comment #32)
> this has a talos improvement in tpaint (measuring window opening times):
> == Change summary for alert #8180 (as of July 23 2017 09:28 UTC) ==
> 
> Improvements:
> 
>   9%  tsvg_static summary windows7-32 opt e10s     64.52 -> 59.02
>   4%  tpaint summary osx-10-10 opt e10s            353.15 -> 339.88
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=8180

It has to be something else!