Coverage reports wrong hit counter

[BridgeAR]

I just looked at a coverage report and found the line hit counter to be wrong for all else cases. It seems to be an off by one error. The closing brackets from loops also seem to be counted wrong. I found a few more issues as well.

1. An example for the first issue: https://coverage.nodejs.org/coverage-46af4c1d01b90ee6/lib/internal/util/comparisons.js.html#L522 and https://coverage.nodejs.org/coverage-46af4c1d01b90ee6/lib/internal/util/comparisons.js.html#L524. Line 522 should have the count from line 524 and line 524 should be 14 as line 525.

2. Loops https://coverage.nodejs.org/coverage-46af4c1d01b90ee6/lib/internal/util/comparisons.js.html#L544 and https://coverage.nodejs.org/coverage-46af4c1d01b90ee6/lib/internal/util/comparisons.js.html#L549:

Here I would expect the hit counter to be 3186x instead of 6709x (the loop was reached 3186 times). It is also a kind of off by one error, just a different one.

3. Something is also weird in this example: https://coverage.nodejs.org/coverage-46af4c1d01b90ee6/lib/internal/util/comparisons.js.html#L130. If the hit counter is indeed 46964 (which it should not be) than the next line should be zero but it is also counted as being hit 46964 times.

4. Closing brackets in general do not seem to be counted properly: https://coverage.nodejs.org/coverage-46af4c1d01b90ee6/lib/internal/util/comparisons.js.html#L153 here the counter is 10 instead of 166 even though the if has been reached 166 times.

5. Some numbers seem to increase magically, at least I did not find a logical correlation to them: https://coverage.nodejs.org/coverage-46af4c1d01b90ee6/lib/internal/util/comparisons.js.html#L134 reports 9134x which is logical due to the former 56098 - 46964. But the line afterwards reports 10263x?! How is it possible to increase the number of line hits in an if statement?
   The same with line 160 - 161.

6. I am not sure about this one but I would expect function declarations to report the times evaluated instead of the times they are called. https://coverage.nodejs.org/coverage-46af4c1d01b90ee6/lib/internal/util/comparisons.js.html#L30. In that case the counter for line 30 should be 693x.

@Trott brought up another issue which is at least not ideal:

The line if (true && bar) would not complain that there's no branch coverage for the first condition to be falsy (which is off course impossible in my example).
Showing branch coverage like this would likely allow to identify more dead code.

// @bcoe @hashseed

[Trott]

@Trott brought up another issue which is at least not ideal:

The line if (true && bar) would not complain that there's no branch coverage for the first condition to be falsy (which is off course impossible in my example).
Showing branch coverage like this would likely allow to identify more dead code.

Code in file foo.js:

function testIt () {
 const thing = true && false;
 return true && false;
}

if (testIt())
  console.log('foo');

console.log('bar');

What I run from the command line:

 c8 node ./foo.js

Result I get:

bar
----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |      100 |       60 |      100 |      100 |                   |
 foo.js   |      100 |       60 |      100 |      100 |               3,7 |
----------|----------|----------|----------|----------|-------------------|

Result I expect:

I expect line 2 (the assignment to thing) to also be reported as being uncovered.

[BridgeAR]

I found more issues:

• Lines after a break/return in a switch case are reported as not covered (e.g. newlines or comments).
  See e.g., https://coverage.nodejs.org/coverage-a046ae5ceddcaaf6/lib/_http_incoming.js.html#L204 and https://coverage.nodejs.org/coverage-a046ae5ceddcaaf6/lib/readline.js.html#L896

• The closing bracket from switch cases are not covered: https://coverage.nodejs.org/coverage-a046ae5ceddcaaf6/lib/readline.js.html#L792 (Probably due to the same reason as above since code that has no break or return report the closing bracket as covered: https://coverage.nodejs.org/coverage-a046ae5ceddcaaf6/lib/readline.js.html#L1000 but again with the wrong hit counter)

• Comments are reported as branch coverage https://coverage.nodejs.org/coverage-a046ae5ceddcaaf6/lib/readline.js.html#L695 (I expect only the opening bracket to be reported instead of the comment as well. This might be tricky and does not seem critical)

• Missing branch coverage is not always properly highlighted https://coverage.nodejs.org/coverage-a046ae5ceddcaaf6/lib/readline.js.html#L730. Here the whole line should be highlighted instead of only the && in the line above.

• (Not) using brackets on if statements changes the report between statement coverage and branch coverage https://coverage.nodejs.org/coverage-a046ae5ceddcaaf6/lib/readline.js.html#L558. Here without the brackets the whole statement is reported as missing branch coverage. The if statement above (https://coverage.nodejs.org/coverage-a046ae5ceddcaaf6/lib/readline.js.html#L556) reports the statement as missing state coverage and only the opening bracket as missing branch coverage. I guess is not critical but it seems a bit confusing.

[bcoe]

@BridgeAR I think there's a chance some of these edge-cases go away when we're able to completely upgrade the V8 version. I suggest that we run coverage with https://github.com/nodejs/node-v8 and remove from this enumerated list of issues and problems that go away on the latest V8.

With regards to issues related to return, I believe this will likely be a similar class of issue as addressed here https://chromium-review.googlesource.com/c/v8/v8/+/1339119 -- CC: @schuay, is there a chance we missed some edge cases around switch?

It would be helpful (even if you keep this open as a catch-all issue) if we could have individual reproductions and bug reports either in V8, within the code-coverage-block.js tests, or if the problem can be attributed to a remapping issue here, in the library that remaps from V8 format coverage to Istanbul format coverage.

☝️ I expect some of these tweaks to move coverage towards perfection will be in V8, and some will be in the library that attempts to translate from V8 to a format we can run reports on.

[hashseed]

Yeah it would be nice to isolate some repros first.

[Trott]

Yeah it would be nice to isolate some repros first.

I put some short repros in bcoe/c8#61 and bcoe/c8#62.

[Trott]

Chromium bug where the fix will fix some or all of these: https://bugs.chromium.org/p/v8/issues/detail?id=8691
I have no idea if we should expect a fix soon or not so much. In the meantime, in order to get more accurate coverage reports at coverage.nodejs.org, there are a few options:

• Not sure if adding istanbul comments as described at https://github.com/gotwarlost/istanbul/blob/master/ignoring-code-for-coverage.md will help.

• A lot of cases look like this:

if (someCondition) {
  throw someError;
}

If that gets changed to this, then the coverage will be correct:

if (someCondition)
  throw someError;

Both of those options seem churn-y and hack-y and likely to meet with resistance. (I'm not sure I like either of them myself.)

Another possibility:

• Find a way to automatically insert comments or strip braces in the CI job that generates coverage reports. (Seems hack-y and likely to be bug-prone to me.)

Which brings us at last to:

• Do nothing and just wait for the fix in V8 (at which point we can float a patch until we upgrade V8).

Any other options I'm not thinking of?

[bcoe]

@Trott I was going to start looking at what it would look like to support:

/* istanbul ignore next */

I was thinking, as a stop gap, we might be able to use similar logic to skip the statement after a throw no promises though, I might find myself going down a rabbit hole where I'm re-implementing the AST (this might be true of ignore functionality too).

[schuay]

Chromium bug where the fix will fix some or all of these: https://bugs.chromium.org/p/v8/issues/detail?id=8691. I have no idea if we should expect a fix soon or not so much.

That V8 issue is a bit confusing because it describes two different things, both related to exceptions. For the code you describe above (where the closing '}' after a throw statement appears uncovered), the fix should be very similar to what we already have for other jump statements at the end of a block. @bcoe do you have time to look into a fix in V8?

[bcoe]

@schuay I would happily look into a fix in V8, I would love to keep diving into that codebase until I'm feeling comfortable in it. Let's take the conversation to chat, and I'll update this thread once we have something in motion -- I was actually confused too, I didn't realize there was an easier fix for the closing } (was reading the thread as one solution for throws in general).

[bcoe]

https://chromium-review.googlesource.com/c/v8/v8/+/1480632 I have a pull open that I believe addresses some of the throw issues.

[jaydenseric]

I'm building a CLI code coverage reporter from scratch, using the NODE_V8_COVERAGE env var, targeting Node.js v10+.

In testing it's come to light Node.js v10 - 11 has a lot of false positives around closing braces. Will whatever the Node.js/V8 fixes were in v12+ ever be backported for v10 -11? If not, I'll have to figure out the best way to skip code coverage for unreliable Node.js versions, which is not ideal.

[jaydenseric]

I've noticed that Node.js v12 (but not v10, or v13) needs semicolons in some situations to realize certain functions exist and are not covered.

Example:

- () => {}
+ () => {};

[BridgeAR]

@bcoe are you still looking into this? The hit counters still seem off. I briefly checked https://coverage.nodejs.org/coverage-4bec6d13f9e9068f/lib/internal/util/comparisons.js.html and I found for example these:

Here the if is reached 115 times. If the next line is reached the same amount of time, the line afterwards should be hit zero times. But it says it's hit 49 times.

This is pretty weird as well: the if is reached less frequently than the actual content. That's impossible, since it's not a loop or something like that.

Here the switch case is reached 78 times. As soon as a case is hit, it returns, so the following hit counters of the cases should be lower by the number of former returns. But they are always 78 times, which can't be.

---

There are more issues but I guess it's best to address them one after the other and I can check from time to time

[BridgeAR]

Finally blocks seem to get a branch not covered even though they will always be triggered:

https://coverage.nodejs.org/coverage-4bec6d13f9e9068f/lib/internal/util/inspector.js.html

[bcoe]

@BridgeAR with regards to finally blocks: https://chromium-review.googlesource.com/c/v8/v8/+/1962265

[bcoe]

@BridgeAR I don't believe there's a tracking issue for the execution counts in the v8 bug tracker it might be worth opening one.

[bcoe]

@BridgeAR I believe the issue with finally statements is now fixed, see:

https://coverage.nodejs.org/coverage-cc0748f509140b7b/lib/internal/util/inspector.js.html

Does that bring the outstanding issues down to the hit counters?

[jasnell]

Ping @BridgeAR @bcoe ... is this fixed?

[bcoe]

👋 it might be good to close this, and open detailed issues here, and I will make an effort to attach those issues to upstream v8 issues when appropriate.

[jasnell]

Ok, will go ahead and close then. Thanks @bcoe!