Rewrite JS coverage lcov output based on what the preprocessor did

RESOLVED FIXED in Firefox 56

Status

Testing
Code Coverage
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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)

Updated

2 years ago
Assignee: nobody → cmanchester
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
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?
(Assignee)

Comment 4

2 years ago
(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.
(Assignee)

Comment 6

2 years ago
Created attachment 8712864 [details]
Bug 1224691 - Add a script to re-write lcov files, replacing chrome urls with sourcefile locations.

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/
(Assignee)

Comment 7

2 years ago
Created attachment 8712865 [details]
Bug 1224691 - Parse lcov files and rewrite them based on preprocessor info.

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/
(Assignee)

Comment 8

2 years ago
Created attachment 8712866 [details]
MozReview Request: Bug 1224691 - Parse lcov files and rewrite them based on preprocessor info.

Review commit: https://reviewboard.mozilla.org/r/32687/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32687/
(Assignee)

Comment 9

2 years ago
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.

Updated

11 months ago
Blocks: 1365591
(Assignee)

Comment 12

11 months ago
Right, I'll get these rebased if necessary and posted for review soon.
Assignee: gmierz2 → cmanchester
Flags: needinfo?(cmanchester)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8712866 - Attachment is obsolete: true
(Assignee)

Comment 15

11 months ago
(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 16

11 months ago
mozreview-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/#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 18

11 months ago
mozreview-review
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+
(Assignee)

Comment 19

10 months ago
Mike, per comment 17 can you give these a once over when you are able? Thanks
Flags: needinfo?(mshal)

Comment 20

10 months ago
mozreview-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

::: 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 21

10 months ago
mozreview-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+
(Assignee)

Comment 22

10 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

10 months ago
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
Last Resolved: 10 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

10 months ago
Flags: needinfo?(mshal)
Depends on: 1373789
Depends on: 1374348
Depends on: 1379955
Depends on: 1381972
Depends on: 1386997
You need to log in before you can comment on or make changes to this bug.