block and function level coverage are incompatible and should not be merged

[bcoe]

When an attempt is made to merge block-level granularity (isBlockCoverage = true) and function level granularity (isBlockCoverage = false), the two types of coverage are incompatible resulting in coverage that claims to have hit unreachable lines of code:

☝️ this is the Node.js test suite run on OSX, therefore code after isWindows should not be hit, the reason it's reported that the lines are hit 3 times, is that Node.js outputs a report with function level granularity that indicates that realpathSync is executed ...

this is probably due to the fact that fs.js is used during the Node.js bootstrapping process, before the inspector has block-level coverage enabled.

Solution

block level coverage should take precedence over function level coverage as soon as it's observed.

[demurgos]

As mentioned in #3, I'd like to keep this issue open a bit longer.

The old implementation did not use isBlockCoverage because I did not have any examples to check the effect of this parameter. As such, this issue is already very helpful.
Because of this, the current implementation produced invalid counts.

The fix in #3 ensures that function covs with mixed isBlockCoverage values are not merged. It does so by giving priority to isBlockCoverage: true and discarding isBlockCoverage: false, it results in less hits but all the hits are valid now.

I'd prefer a solution that preserves all function coverage information. I see two solutions:

• Add a new (non-V8) field to FunctionCov to track function-level counts. The new interface would be:

  interface FunctionCov {
    functionName: string;
    ranges: RangeCov[];
    isBlockCoverage: boolean;
    count: number;
  }
  

  This count field would be the sum of the counts of the root RangeCov of the function. ranges remains computed as in fix: handle attempts to merge function into block coverage #3 (priority to isBlockCoverage: true). When merging a function with 2 non-block hits and 3 block hits, it would produce something like {count: 5, ranges: [{start: 0, end: 100, count: 3}], isBlockCoverage: true, ...}.

• Use isBlockCoverage as part of the key to match function coverages. This means that to be merged, function coverages must have the same root range AND isBlockCoverage value. In case of mixed values, there will be two FunctionCov for the same function: one with isBlockCoverage: false and one with isBlockCoverage: true.

These two solutions allow to keep all the information. It would allow consumers to display better information. For example, Istanbul's reporters allow to distinguish between function counts and statement counts.

The first solution is easier to consume: there is only one FunctionCov per function, but it adds a non-V8 field. Still, it provides a graceful fallback if this field is ignored: we get the current behavior (less counts, but nothing breaks).
The second solution remains compatible with the V8 schema, but it may cause issues in consumers that do not expect a FunctionCov to be defined twice for the same function.

I prefer the first solution, but maybe strict compliance with the V8 schema is needed.

What is your opinion @bcoe?

[demurgos]

When an attempt is made to merge block-level granularity (isBlockCoverage = true) and function level granularity (isBlockCoverage = false), the two types of coverage are incompatible resulting in coverage that claims to have hit unreachable lines of code

Are these function coverages with different granularity part of the coverage of the same Node process or not? If they are both part of the same process coverage, then the second solution (to keep both and merge them separately) is better as it matches what happens already in V8.

[bcoe]

re: What is your opinion @bcoe?

@demurgos I don't have a strong opinion about implementation. I think the case in which we'd want to use function coverage and non-function coverage in conjunction is a pretty rare edge-case. You're either probably running your entire test-suite with block coverage enabled (detailed) or running it with only function coverage.

I'm not why running the Node test suite was outputting both types of coverage; this was potentially a race condition in which V8 did not have detailed coverage enabled fast enough.

[demurgos]

I was not able to produce mixed isBlockCoverage values for the same function in the same ProcessCov. The mixed values come from separate ProcessCov files. Because of this, none of the solutions corresponds to "native" V8 behavior and I'll go with the first solution to avoid loosing data (extra count field on FunctionCov objects).

If there's a way to get mixed isBlockCoverage values then I'll switch to the second solution, but for the moment I'll leave it aside: this model would cause more problems downstream and is more surprising (it looks like the FunctionCov values are duplicated).

[demurgos]

This also solves issues in complex cases that don't even involve mixed isBlockCoverage. For examples, there's this test case currently:

merges a similar sliding chain: a+b+c+d
0   1   2   3   4   5   6   7
[10------------------------)
[1-------------)
+
[10------------------------)
    [1-------------)
+
[10------------------------)
        [1-------------)
+
[10------------------------)
            [1-------------)
=
[31------------------------)
    [22----------------)
        [13--------)
            [4-)

Overall, the function is called 40 times, but no statement is called more than 31 times. By attaching the total count we can get better precision.

[JoHuang]

Hi, I found that mixed isBlockCoverage causes information with isBlockCoverage==false lost.
Why does node v8 generate 2 different granularity of results?