Bad application of source maps to coverage reports for ternary expressions

[skozin]

I don't know for sure where I should report this issue, so sorry if it doesn't belong here.

When I generate coverage report with nyc ava && nyc report --reporter=html, the resulting report shows incorrect highlighting of the source code. And, the most critically, overall coverage numbers for branches and functions look incorrect (much less than expected).

I have prepared the repo with the reproduction case.
Also, I've uploaded the report to this GH pages site.

Here is a screenshot that shows incorrect highlighting:

Annotations on the left suggest that _takeFromWaitingPublisher() function has been executed multiple times, and that's indeed the case as the test code is crafted to ensure this. Yet it is highlighted as not being covered.

The _take() function also has a similar problem, although its code is highlighted in yellow, which means "branch not covered". However, the highlighted code has clearly been executed multiple times.

Also, the highlighting sometimes starts and ends at positions that make no sense, e.g. between "t" and "h" letters inside this keyword.

[bcoe]

@skozin does the percentage of coverage seem correct, but the highlighting off? This is most likely a problem with how we remap from the sourcemap to the coverage files used by Istanbul; @novemberborn is a subject matter expert on this part of the codebase.

[skozin]

@bcoe unfortunately no,

And, the most critically, overall coverage numbers for branches and functions look incorrect (much less than expected).

[skozin]

It reports 75% Branches (33/44) and 66.67% Functions (4/6), although almost all functions have been executed, and almost all branches inside them were taken.

Specifically, I can see 12 functions, and 9 of them were executed, which means 75%:

Covered:

constructor: 1x
get _needsDrain: 6x
get _bufferSize: 8x
take: 24x
anon inside take: 24x
_take: 24x
_takeFromWaitingPublisher: 9x
_triggerWaiters: 6x
assert: 49x

Not covered:

anon at the end of _take
_emitDrain
nop

[skozin]

I've simplified the case a little. Now nyc reports 73.68% Branches (28/38) and 66.67% Functions (4/6). The function coverage I expect to see is 80% (8/10):

Covered:

constructor: 1x
take: 24x
anon inside take: 24x
_take: 24x
_takeFromWaitingPublisher: 9x
_triggerWaiters: 9x
_emitDrain: 1x
assert: 49x

Not covered:

anon at the end of _take
nop

Total:

80% (8/10)

[skozin]

Also, If I move from ES6 classes to plain functions, the functions coverage numbers start to match the expected ones: 80% (8/10). See the no-class branch.

So the issue with incorrect functions stats has something to do with how Babel transpiles ES2015 classes (any maybe other constructs).

Also note that the reported total number of functions differs when comparing master (6 functions incorrectly reported) with no-class (10 functions correctly reported), though these branches differ only in the presence of class construct around the functions.

However, highlighting remains wrong in both cases.

[novemberborn]

@skozin I'll try and have a look at this tomorrow.

[skozin]

@novemberborn Thanks a lot!

[novemberborn]

@skozin finally got round to this. First I compiled index.js directly ($(npm bin)/babel index.js --out-dir build) and switched the tests to use ./build/index.js. The coverage report looked good:

I then looked more closely at the locations in your original code where the coverage report goes wrong. These are ternary expressions. When I reformat them the report matches that of the Babel build output:

The conclusion then is that we're having trouble converting coverage reports for certain ternary expressions. I haven't investigated whether this is an issue with the coverage reports from Istanbul or our conversion logic.

I suppose you could reformat your code a little until this gets fixed properly. Thanks for finding this!

[novemberborn]

For future reference, using braces in the arrow function also breaks the source map application.

The repo with the original test case and a workaround is at https://github.com/novemberborn/nyc-babel-issue-repro.

[skozin]

@novemberborn, great, thanks for your help! I'll reformat my trenaries as a temporary solution.

And what do you think about incorrect Funcs coverage numbers I described in previous comments?

I've simplified the case a little. Now nyc reports 73.68% Branches (28/38) and 66.67% Functions (4/6). The function coverage I expect to see is 80% (8/10):

Covered:

constructor: 1x
take: 24x
anon inside take: 24x
_take: 24x
_takeFromWaitingPublisher: 9x
_triggerWaiters: 9x
_emitDrain: 1x
assert: 49x

Not covered:

anon at the end of _take
nop

Total:

80% (8/10)

Also, If I move from ES6 classes to plain functions, the functions coverage numbers start to match the expected ones: 80% (8/10). See the no-class branch.

So the issue with incorrect functions stats has something to do with how Babel transpiles ES2015 classes (any maybe other constructs).

Also note that the reported total number of functions differs when comparing master (6 functions incorrectly reported) with no-class (10 functions correctly reported), though these branches differ only in the presence of class construct around the functions.

Should I open another issue for this?

[novemberborn]

@skozin if I take your no-class branch and apply the ternary workarounds I see the same uncovered lines as in the report for the built file (without source maps). I imagine any other mismatch is because of the ternary issue.

[skozin]

@novemberborn, I guess there is some misunderstanding here.

See, I run coverage on no-class-workaround branch, which is the same as workaround, but with all functions declared in a plain object instead of a class body (see the diff):

------------------------|----------|----------|----------|----------|----------------|
File                    |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
------------------------|----------|----------|----------|----------|----------------|
 nyc-babel-issue-repro/ |    89.29 |    73.68 |       80 |    89.29 |                |
  index.js              |    89.29 |    73.68 |       80 |    89.29 |... 3,44,93,113 |
------------------------|----------|----------|----------|----------|----------------|
All files               |    89.29 |    73.68 |       80 |    89.29 |                |
------------------------|----------|----------|----------|----------|----------------|

HTML report shows 80% Functions [8/10]. These are the correct numbers.

Now I run the coverage on workaround branch, that differs from no-class-workaround only in that all functions are declared inside the class body.

------------------------|----------|----------|----------|----------|----------------|
File                    |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
------------------------|----------|----------|----------|----------|----------------|
 nyc-babel-issue-repro/ |    89.29 |    73.68 |    66.67 |    89.29 |                |
  index.js              |    89.29 |    73.68 |    66.67 |    89.29 |... 3,44,93,113 |
------------------------|----------|----------|----------|----------|----------------|
All files               |    89.29 |    73.68 |    66.67 |    89.29 |                |
------------------------|----------|----------|----------|----------|----------------|

HTML report shows 66.67% Functions [4/6]. The functions total now is 6, which is obviously not correct, as there are total 10 functions in index.js (8 class methods and 2 anonymous functions inside these methods). Also, the coverage, in comparison with no-class-workaround branch, has decreased from 80% (8 funcs of 10) to 66.67% (4 funcs of 6).

I guess this is not what one would expect. Note that highlighting in HTML report is correct in both branches (as they both have the workaround applied), as well as line execution counts.

[skozin]

I've updated the branches to match more closely. Actually, now it's enough to see the diff, which now also contains coverage numbers: skozin/nyc-babel-issue-repro@workaround...no-class-workaround.

[skozin]

I'm not actually sure these incorrect functions numbers are caused by nyc, maybe this is an issue/feature in istanbul.

[novemberborn]

@skozin yea I'm starting to think applying the source maps to Istanbul's coverage report is wrong. We need raw execution info, map that back to the original source, and then determine coverage.

Will keep your cases in mind when I/someone gets round to improving the behavior here.

[novemberborn]

@skozin apparently this may be solved in future Istanbul versions, though I don't know how compatible those will be with nyc. See gotwarlost/istanbul#582 (comment)

[skozin]

@novemberborn, thanks for considering my cases, and for the link. Hope it will be compatible)

[skozin]

@novemberborn I tried to forcefully install [email protected], the latest of the alpha releases, instead of 0.4.2 required by nyc. It worked, with all reports successfully generated, so it appears to be compatible. Unfortunately, both issues still persist (incorrect highlighting and incorrect functions coverage). Maybe some future version will address this.

I guess here is the only related commit in istanbul-api (which is the main code of Istanbul, extracted into separate library in 1.0): istanbuljs-archived-repos/istanbul-api@6c44390, Initial source map implementation.

And here is the source map support lib, which usage is introduced in that commit: https://github.com/istanbuljs/istanbul-lib-source-maps.

[bcoe]

@skozin closing in favor of #266