Closed Bug 1224691 Opened 6 years ago Closed 4 years ago

Rewrite JS coverage lcov output based on what the preprocessor did

Categories

(Testing :: Code Coverage, defect)

defect
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Depends on 2 open bugs, Blocks 3 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

We need to adjust based on differences in line info from #if and #include. The preprocessor leaves "//@line <number> "<file>"" comments in both cases we can use to correct line numbers. Dealing with #included files will be a more complicated, because we need to "split" lcov records, which could be tricky for summary info.

jcranmer has a working prototype for rewriting line numbers within a file in https://bugzilla.mozilla.org/show_bug.cgi?id=1214885#c3
Assignee: nobody → cmanchester
I'm going to upload what I have so far for this (used on top of bug 1214885 to produce http://people.mozilla.org/~cmanchester/jscov/ -- coverage on a linux debug build for the tests in browser/base/content/test/newtab).

Not entirely sure who to ask for review on this -- I'll find someone once bug 1214885 gets closer to landing.
Mozreview associated it with the other bug because it's on top of those patches, but https://bugzilla.mozilla.org/show_bug.cgi?id=1214885#c14 is the relevant commit.
I was looking at some of the reports, and I am quite surprised by some values, such as:
 http://people.mozilla.org/~cmanchester/jscov/browser/base/content/test/newtab/browser_newtab_bug752841.js.gcov.html#615

The "functions" view reports functions which should apparently be part of another file.  Could that be an issue with the aggregator, or is this an issue in the JS LCov implementation?  Unless this is an artifact of the test suite?
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> I was looking at some of the reports, and I am quite surprised by some
> values, such as:
>  http://people.mozilla.org/~cmanchester/jscov/browser/base/content/test/
> newtab/browser_newtab_bug752841.js.gcov.html#615
> 
> The "functions" view reports functions which should apparently be part of
> another file.  Could that be an issue with the aggregator, or is this an
> issue in the JS LCov implementation?  Unless this is an artifact of the test
> suite?

Those results look merged with those for browser/base/content/test/newtab/head.js. Looking at the original .info file, the record in question starts with:

SF:chrome://mochitests/content/browser/browser/base/content/test/newtab/head.js
SF:chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_bug752841.js
FN:128,this.test
FN:131,this.test/<
FN:133,this.test/</<    
FN:141,setup
FN:142,setup/<
FN:143,setup/</<
FN:144,cleanupAndFinish
FN:145,setup/</</cleanupAndFinish/<
FN:161,setup/promiseReady<
FN:163,setup/promiseReady</<
FN:1,top-level
FN:45,top-level
FN:237,getGrid
FN:246,getCell

The parser assumes one SF per record and attributes everything to the second file. When I run it again the results look better for the test file record, but the next record starts with:

SF:chrome://mochitests/content/browser/browser/base/content/test/newtab/head.js
SF:chrome://mochitests/content/browser/browser/base/content/test/newtab/head.js
FN:1,top-level
FN:45,top-level
FN:78,top-level
FN:82,top-level
FN:141,setup
FN:142,setup/<

(two SF: entries again).
Ok, I will look into this double SF issue (Bug 1227735) with no end_of_record in between.
The "remoteenabled", and "remoterequired" flags are used by some chrome
registered by test harnesses. In order to parse those manifests with mozpack,
the two flags are added as permitted.

Review commit: https://reviewboard.mozilla.org/r/32683/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32683/
This script uses the info generated by the UrlMap backend to replace chrome urls
found in lcov files with source-paths. There are several other cases a script
might not correspond to a source url, such as "data:" uris, which are handled
by omitting those sections in the produced lcov files. The set of files included by a JS source (if it is preprocessed) is also calculated, but not consumed
at this time. A more extensive lcov rewriting mechanism will be added in the
future to re-construct source information in produced coverage reports.

Review commit: https://reviewboard.mozilla.org/r/32685/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32685/
Sam, the commit posted in comment 7 should be useful for finding source files from resource/chrome urls.
:chmanchester

Are you still working on this?  If so, take it back.  Otherwise Greg will look at it.

Thank you!
Assignee: cmanchester → gmierz2
Flags: needinfo?(cmanchester)
Chris said he could rebase the patches, if we were OK with the approach he took. I think the approach is fine.
Another possibility (only for the rewriting of paths, not for the preprocessor-related stuff) would be to do it at runtime in the JSVM code that handles code coverage, by using nsIResProtocolHandler.
Blocks: 1365591
Right, I'll get these rebased if necessary and posted for review soon.
Assignee: gmierz2 → cmanchester
Flags: needinfo?(cmanchester)
Attachment #8712866 - Attachment is obsolete: true
(In reply to Marco Castelluccio [:marco] from comment #11)
> Chris said he could rebase the patches, if we were OK with the approach he
> took. I think the approach is fine.
> Another possibility (only for the rewriting of paths, not for the
> preprocessor-related stuff) would be to do it at runtime in the JSVM code
> that handles code coverage, by using nsIResProtocolHandler.

Handling this in nsIResProtocolHandler may be worth pursuing. As I'm thinking through how this might work, the patches I have here pretty much assume the build and test are happening on the same machine, I don't know how this is going to work in automation exactly.
Comment on attachment 8712864 [details]
Bug 1224691 - Add a script to re-write lcov files, replacing chrome urls with sourcefile locations.

https://reviewboard.mozilla.org/r/32683/#review150404
Attachment #8712864 - Flags: review?(mcastelluccio) → review+
I'm going to review the other patch tomorrow.

I'm doing a high level review, but someone more comfortable with mozbuild should review too.
Comment on attachment 8712865 [details]
Bug 1224691 - Parse lcov files and rewrite them based on preprocessor info.

https://reviewboard.mozilla.org/r/32685/#review150804
Attachment #8712865 - Flags: review?(mcastelluccio) → review+
Mike, per comment 17 can you give these a once over when you are able? Thanks
Flags: needinfo?(mshal)
Comment on attachment 8712864 [details]
Bug 1224691 - Add a script to re-write lcov files, replacing chrome urls with sourcefile locations.

https://reviewboard.mozilla.org/r/32683/#review153498

::: python/mozbuild/mozbuild/codecoverage/lcov_rewriter.py:1
(Diff revision 2)
> +# This Source Code Form is subject to the terms of the Mozilla Public

Where is lcov_rewriter.py actually used? The only thing I see so far is the test for it. Is it something a dev will run manually on lcov files?
Attachment #8712864 - Flags: review+
Comment on attachment 8712865 [details]
Bug 1224691 - Parse lcov files and rewrite them based on preprocessor info.

https://reviewboard.mozilla.org/r/32685/#review153482

::: python/mozbuild/mozbuild/codecoverage/lcov_rewriter.py:43
(Diff revision 2)
> +        self.branches = {}
> +        self.lines = {}
> +
> +    def __iadd__(self, other):
> +
> +        # These shouldn't differ.

Is this an assumption that you need to verify?

::: python/mozbuild/mozbuild/codecoverage/lcov_rewriter.py:52
(Diff revision 2)
> +        self.functions.update(other.functions)
> +
> +        for name, count in other.function_exec_counts.iteritems():
> +            new_count = count
> +            if name in self.function_exec_counts:
> +                new_count = int(count) + int(self.function_exec_counts[name])

Do you expect the counts to ever not be integers? It seems this block could just be self.function_exec_counts[name] = count + self.function_exec_counts.get(name, 0)
Attachment #8712865 - Flags: review+
Comment on attachment 8712864 [details]
Bug 1224691 - Add a script to re-write lcov files, replacing chrome urls with sourcefile locations.

https://reviewboard.mozilla.org/r/32683/#review153498

> Where is lcov_rewriter.py actually used? The only thing I see so far is the test for it. Is it something a dev will run manually on lcov files?

Right, this can be used locally, or more likely in an automation job yet to be determined.
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa2287646d68
Add a script to re-write lcov files, replacing chrome urls with sourcefile locations. r=marco,mshal
https://hg.mozilla.org/integration/autoland/rev/5882727a7d16
Parse lcov files and rewrite them based on preprocessor info. r=marco,mshal
https://hg.mozilla.org/mozilla-central/rev/aa2287646d68
https://hg.mozilla.org/mozilla-central/rev/5882727a7d16
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Flags: needinfo?(mshal)
Depends on: 1373789
Depends on: 1374348
Depends on: 1379955
Depends on: 1381972
Depends on: 1386997
Depends on: 1522304
You need to log in before you can comment on or make changes to this bug.