Chris Manchester (limited bugmail, email directly) Assignee
	Description • 9 years ago

I'd like to experiment with using the coverage API added to the debugger in bug 1189360 to provide a summary of coverage for an individual browser-chrome test. Initially I'd like to use this to generate a periodic database of coverage for each test that automation would be able to use to prioritize tests for certain changesets (if a file is changed, and we see that the file is well covered by a handful of tests, we can run those tests first).

This might also be useful if we want to look at tests that have very similar coverage profiles, or files that are poorly covered by the entire set of tests.

Another feature we're interested in is finding the difference in coverage between runs of tests, so whatever serialization we come up with here will need to support that.

Initially this should be something that can be run locally without a lot of effort, automation will come later.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 1 • 9 years ago

Bug 1229598 - Add a mode to browser-chrome tests to summarize per-test code coverage.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 2 • 9 years ago

Comment 1 is aimed at providing output that will allow us to determine what line coverage is unique to a test file. It's working fairly well (I can look at the output and determine that among all the tests in browser/base/content/test/newtab, NewTabUtils.undoAll is only called by browser_newtab_undo.js). The coverage counts for test files themselves are all too low, I think due to one of the open bugs on flushing coverage.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 3 • 9 years ago

I've updated this to report all lines covered for each test. Uniqueness is interesting, but it only makes sense in a certain scope of which tests ran. If we record the set of all lines covered by a test we can calculate uniqueness after the fact.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 4 • 9 years ago

Comment on attachment 8698282 [details]
MozReview Request: Bug 1229598 - Add a mode to browser-chrome tests to summarize per-test code coverage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27907/diff/1-2/

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 5 • 9 years ago

I got this running on try here (some chunks are orange due to leaks, which is ok for now): https://treeherder.mozilla.org/#/jobs?repo=try&revision=005292c492e9

An example of the uploaded format is: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/d9deb2552a777b6cdb8c05c27dbdbd22f5cb788c324382099db2f0752ac935b68d8f3261d193db489f629d69f819ec085e536c2459a118d6ac8af6769a385a5c

{
  <test url>: {
    <source file 1>: [ <covered lines>... ],
    <source file 2>: [ <covered lines>... ]
    ...
  }
}

	Kyle Lahnakoski [:ekyle]
	Comment 6 • 9 years ago

I hope the format can be changed.  A format with a finite number of property names is much easier to query, and index.  For example:

> [
> {
>   "test_url": <test url>
>   "source_file": <source file 1>
>   "lines": [ <covered lines> ... ]
> },
> {
>   "test_url": <test url>
>   "source_file": <source file 2>
>   "covered": [ <covered lines> ... ]
> },
> ...
> ]

 * May the <covered lines> be integers rather than text?
 * Can I assume the coverage artifacts will all be named r".*_cov_\d+\.json" ?
 * Can we record the code lines **not** covered?

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 7 • 9 years ago

(In reply to Kyle Lahnakoski [:ekyle] from comment #6)
> I hope the format can be changed.  A format with a finite number of property
> names is much easier to query, and index.  For example:
> 
> > [
> > {
> >   "test_url": <test url>
> >   "source_file": <source file 1>
> >   "lines": [ <covered lines> ... ]
> > },
> > {
> >   "test_url": <test url>
> >   "source_file": <source file 2>
> >   "covered": [ <covered lines> ... ]
> > },
> > ...
> > ]
> 
>  * May the <covered lines> be integers rather than text?

Sure, they come out of the debugger as text but I can convert them.

>  * Can I assume the coverage artifacts will all be named r".*_cov_\d+\.json"
> ?

I need to change this again... it will be jscov_<uuid>.json or jscov_<timestamp>.json

>  * Can we record the code lines **not** covered?

This might be tricky, but a post-processing step could get close.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 8 • 9 years ago

Comment on attachment 8698282 [details]
MozReview Request: Bug 1229598 - Add a mode to browser-chrome tests to summarize per-test code coverage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27907/diff/2-3/

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 9 • 9 years ago

Comment on attachment 8698282 [details]
MozReview Request: Bug 1229598 - Add a mode to browser-chrome tests to summarize per-test code coverage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27907/diff/3-4/

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 10 • 9 years ago

Comment on attachment 8698282 [details]
MozReview Request: Bug 1229598 - Add a mode to browser-chrome tests to summarize per-test code coverage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27907/diff/3-4/

	Andrew Halberstadt [:ahal]
	Comment 11 • 9 years ago

Comment on attachment 8698282 [details]
MozReview Request: Bug 1229598 - Add a mode to browser-chrome tests to summarize per-test code coverage.

https://reviewboard.mozilla.org/r/27907/#review25721

Looks good. Just a few comments.

::: testing/mochitest/browser-test.js:255
(Diff revision 4)
> +                                          null);

nit: unnecessary null

::: testing/modules/CoverageUtils.jsm:15
(Diff revision 4)
> +const {TextEncoder, OS} = Cu.import("resource://gre/modules/osfile.jsm", null);

No need to pass in null, though if you ever want this to work on B2G, you need to pass in a scope. Ditto for import below. Scopes are preferred over using the return value anyway.

::: testing/modules/CoverageUtils.jsm:103
(Diff revision 4)
> +  dump("Collecting coverage for: " + testName + "\n");

Do these dumps get printed for every test? Feels like it could be unnecessarily verbose. If it's per suite, nevermind.

	Pulsebot
	Comment 12 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/42939c23673e

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/42939c23673e