Closed Bug 1380659 Opened 2 years ago Closed 2 years ago

Support dumping/resetting coverage counters from JavaScript

Categories

(Testing :: Code Coverage, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: marco, Assigned: marco)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 13 obsolete files)

4.29 KB, patch
marco
: review+
Details | Diff | Splinter Review
8.28 KB, patch
marco
: review+
Details | Diff | Splinter Review
1.10 KB, patch
marco
: review+
Details | Diff | Splinter Review
2.45 KB, patch
marco
: review+
Details | Diff | Splinter Review
1.45 KB, patch
marco
: review+
Details | Diff | Splinter Review
6.47 KB, patch
marco
: review+
Details | Diff | Splinter Review
2.17 KB, patch
marco
: review+
Details | Diff | Splinter Review
2.34 KB, patch
marco
: review+
Details | Diff | Splinter Review
2.79 KB, patch
marco
: review+
Details | Diff | Splinter Review
Attached patch Patch (obsolete) — Splinter Review
This will make it possible to support custom analyses for subperiods of the Firefox execution like the one in bug 1372211.
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #8886185 - Attachment is obsolete: true
Attachment #8886512 - Flags: review?(kchen)
Attachment #8886512 - Attachment description: Add new IPC messages to dump/reset coverage counters → Part 1: Add new IPC messages to dump/reset coverage counters
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
Attachment #8886515 - Flags: review?(jmaher) → review+
Attachment #8886512 - Flags: review?(kchen) → review+
Attachment #8886518 - Flags: review?(jmaher)
Attachment #8886519 - Flags: review?(jmaher)
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!
Attachment #8886518 - Flags: review?(jmaher) → review+
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)
Attachment #8886519 - Flags: review?(jmaher) → review-
(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?
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.
Fixed commit message, carrying r+.
Attachment #8886518 - Attachment is obsolete: true
Attachment #8887939 - Flags: review+
Added call for resetting code coverage to the mochitest. Since the tests are run in isolation, it's fine to do it.
Attachment #8886519 - Attachment is obsolete: true
Attachment #8887940 - Flags: review?(jmaher)
Attachment #8886517 - Attachment is obsolete: true
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!
Attachment #8887940 - Flags: review?(jmaher) → review+
Attachment #8886513 - Flags: review?(erahm)
Attachment #8887943 - Flags: review?(erahm)
Attachment #8887944 - Flags: review?(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.
Attachment #8886513 - Flags: review?(erahm) → review+
Attachment #8887943 - Flags: review?(erahm) → review+
Attachment #8887944 - Flags: review?(erahm) → review+
(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.
Carrying r+.
Attachment #8886512 - Attachment is obsolete: true
Attachment #8889151 - Flags: review+
Attachment #8886515 - Attachment is obsolete: true
This is a trivial patch, just moving the mochitest and xpcshell tests under separate directories, like we usually do.
Attachment #8889159 - Flags: review+
Pushed by mcastelluccio@mozilla.com:
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
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
(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!
Blocks: 1431753
You need to log in before you can comment on or make changes to this bug.