coverage: NODE_V8_COVERAGE for Node's test suite, remaining blockers

[bcoe]

I'm in the process of trying to collect coverage for Node.js' own internal libraries, and am running into a few significant problems:

• line counts appear to be all over the place; also we don't collect coverage for the wrap itself on several internal modules. Here's output from assert.js as an example.

  • with coverage: make coverage work for Node.js #23941, we are now collecting coverage for the top level of internal modules.
  • it's still somewhat difficult to figure out the initial offset of files, this is being worked on in (lib: modify NativeModule to use compileFunction #23837, and module: use compileFunction over Module.wrap #21573).
  • coverage can't currently be collected or functions with return statements coverage isn't collected for closing braces in if statements, see: https://bugs.chromium.org/p/v8/issues/detail?id=8381 (@hashseed, @schuay).
    • coverage isn't collected for throw statement.

• the performance of merging 2800 reports is atrocious, I think this is partially because files like loader.js need to get merged over and over again, and also have many 1000s of blocks.

  • @demurgos' work has significantly improved merging performance.

• I'm seeing a few failures for tests that instrument code used by coverage itself; specifically tests that mock the events module.

would love some help getting unblocked from these two issues, very excited to move our own test suite towards built in coverage.

CC: @addaleax, @demurgos, @hashseed, @TimothyGu, @schuay

• Version: >v10.10.0
• Platform :all platforms
• Subsystem: test

[schuay]

For point 2 (merging coverage reports), one possibility would be to make V8's behavior here configurable and add a mode that skips all of the attempted reductions of reported coverage. Another alternative may be to disable these entirely.

Do you have more details about 1 and 3? Is the coverage reported by V8 incorrect?

[hashseed]

You could definitely play around with how V8's prepares the data, e.g. remove the calls to merging ranges.

[bcoe]

@hashseed @schuay I could get V8 up and running on my machine again and take a stab, how might we configure this setting. My concern would be screwing with the format that Chromium expects in inspector.

[hashseed]

Well we could make this a flag in either V8 or Devtools protocol so that it does affect Devtools' use case.

[bcoe]

@hashseed correct me if I'm wrong, but if we didn't collapse together ranges, you'd have every range in every processes' output, regardless of whether each block/function is executed. So, merging would become as simple as walking through the two files and adding them together in unison?

[hashseed]

Not sure I understand fully what you mean. We employ a bunch of passes to reduce the ranges we need to pass through the protocol. If these passes do not run, you would get ranges for every block and every function, but some would nest inside others.

[bcoe]

@hashseed merging two reports in Istanbul is very simple, because you know you'll have the same lines, blocks, and functions:

FileCoverage.prototype.merge = function (other) {
    var that = this;
    Object.keys(other.s).forEach(function (k) {
        that.data.s[k] += other.s[k];
    });
    Object.keys(other.f).forEach(function (k) {
        that.data.f[k] += other.f[k];
    });
    Object.keys(other.b).forEach(function (k) {
        var i,
            retArray = that.data.b[k],
            secondArray = other.b[k];
        if (!retArray) {
            that.data.b[k] = secondArray;
            return;
        }
        for (i = 0; i < retArray.length; i += 1) {
            retArray[i] += secondArray[i];
        }
    });
};

https://github.com/istanbuljs/istanbuljs/blob/master/packages/istanbul-lib-coverage/lib/file.js#L248

I think potentially we'd get to the same spot with the V8 reports if we didn't reduce ranges?

[hashseed]

Yes. If you don't reduce the ranges in here we should get ranges that you could overlay on top of each other from different runs.