Node: missing block coverage for functions

[bcoe]

• Version: v12.1.0
• Platform: Fedora 28 (Twenty Eight)
• Subsystem: test

This took me a while to figure out, _and is potentially in the V8 engine._

In Node 12, block coverage seems to be dropped for some functions, on some platforms.

Here's an example:

• this coverage output is generated on Fedora with [email protected], by running the test suite for this library.
• this coverage output is generated from an identical run on [email protected].

The main difference between the two test runs is that coverage for generateChangelogEntry is missing in the output from Fedora (this is a serious issue, because from a test reporter point of view, generateChangelogEntry will appear as covered).

• tracking issue on V8: https://bugs.chromium.org/p/v8/issues/detail?id=9212
• tracking issue on c8: https://github.com/bcoe/c8/issues/90

Additional Information

• ~this problem seemed to also occur on 11.14.0 on Fedora on @coreyfarrell's computer, but not 11.15.0~; @coreyfarrell was unable to reproduce on >11.10.0 with further testing.

CC: @joyeecheung @hashseed @schuay, was wondering if you might have any thoughts on this one ... I could imagine it possibly being an issue in V8, or potentially a race condition during the bootstrapping of coverage in Node.js.

[joyeecheung]

or potentially a race condition during the bootstrapping of coverage in Node.js.

With the current inspector protocol implementation in Node.js everything is synchronous, so there is not much room for race conditions that I can think of..

[siddhi92]

Is there solution

[schuay]

Replied on the V8 tracking issue.

[bcoe]

@nodejs/build @nodejs/testing, had a conversation with @schuay this morning, this issue is very odd;

He is able to reproduce the issue on his Linux desktop with the distributed Node.js versions, he is unable to reproduce the issue if he builds Node.js from source.

So far, we have not seen the issue crop up on OSX.

[rvagg]

There's going to be a big difference between the compiler, libc and libstdc++ that we use to build binaries for Linux and what's available on Fedora 28 for native compile. BUILDING.md now has details for Node 12 compiler config we use.
My suggestion would be to try and replicate that compile environment, maybe with the use of Docker.

[coreyfarrell]

All testing I did was with copied of node.js installed by nvm (https://nodejs.org/download/release/v12.1.0/node-v12.1.0-linux-x64.tar.xz for example). Sorry if that wasn't clear.

[bcoe]

@coreyfarrell was clear on that fact 👍

@schuay was testing with both the shipped version (which is what you get from nvm) and was testing by compiling from source, to try to further dig into the issue -- the frustrating thing is so far it seems compiler dependent (like @rvagg says).

[rvagg]

If you are having trouble coming up with an environment to replicate the compiler setup we use and that's an important issue here I can probably put together a simple Dockerfile for you to use to get that. Let me know if that's key here and I'll see what I can come up with when I have time.

[schuay]

To clarify, my locally-built version was not identical to the official release, so there's plenty of opportunities for behavioral differences to come in without starting to look at compilers and libc (e.g.: different Node commit, build-system changes since I built with GN).

I'm a bit swamped currently but will try to set aside some time to investigate further.

Edit: Perhaps someone can check if this still reproduces on ToT Node, and bisect if not.

[bcoe]

@joyeecheung @nodejs/v8, @schuay has landed a couple patches that should address the underlying cause of this bug (we isolated the bug to generator functions, which had some known issues in the block coverage implementation). Here are the patches in question:

https://github.com/v8/v8/commit/3002ff44ee0cd15fbfca9636f4b4bf0599bbe8a0
https://github.com/v8/v8/commit/8c33e289b5699958ae929f4f0691544656a33b22

Unfortunately, between the version of V8 pulled into Node 12.x.x and @schuay's patch, some fairly major changes have landed related to private class functions, and the patches above do not land cleanly (they create significantly more conflicts than I've seen backporting other PRs).

Any thoughts? are your planning to cherry-pick your work around private class functions @joyeecheung, landing this would hopefully help @schuay's patches apply more cleanly.

[targos]

We are working on the update to V8 7.5 and hope to be able to backport it to v12.x. Would it help to wait for it?

[bcoe]

@targos would [email protected] encompass, @schuay and @joyeecheung's work, if so waiting for 7.5 might solve this issue.

[joyeecheung]

@bcoe From a glance at the listed patches I don't seem to be able to find lines that I have touched before? Though if there are a lot of conflicts I think the better solution is usually to just wait for a complete update if it's to urgent to have the fix.

[bcoe]

@joyeecheung @schuay @rvagg, I just tested with the nightly build:

node-v13.0.0-nightly20190819ec16fdae54-linux-x64

And the issue of dropped functions from coverage continues to persist. I believe that the version of Node.js referenced above pulls in the work that Jakob did to attempt to address this issue.

I'm a bit perplexed:

• the issue exists on 12.0.0, so it seems to have cropped up as soon as the first releases of 12.x were cut.
• it does not crop up on v11.15.0 on linux-x64.
• it does not crop up on v12.8.1 on OSX (seems to be linux specific).

I think this is a fairly serious issue because it results in folks thinking that they've written tests for functions that might be completely unexercised.

[schuay]

That's unfortunate :[ If I recall correctly, I wasn't able to reproduce this at all back then. A reliable d8 (or debug node build) repro would be very helpful.

[bcoe]

@schuay I have created a demonstration of the bug here:

https://github.com/bcoe/node-27566-bug

As far as I can tell the root cause is the number of functions in a class, if a class has 14 or more functions on my machine, we drop all coverage information for the functions in that class, the class has under 14 functions we collect block coverage information for it.

[bcoe]

looks like we have a reproduction 🎉 and @schuay just landed a patch that he thinks should address this issue:

https://bugs.chromium.org/p/v8/issues/detail?id=9212

I'll make an effort to back port a V8 patch (it's been a while since I've done this, so might end up needing to ask for some help).

[schuay]

I verified https://crbug.com/v8/9212#c22 fixes the issue when applied to current Node master. Thanks for the repro, wouldn't have found this without it.

[rvagg]

Top quality work on finding, debugging and fixing @bcoe & @schuay! Have some 🍰 for a job well done that probably won't receive the appreciation it deserves.

[bcoe]

@rvagg @schuay I'll test with the next nightly version of Node.js and close this issue assuming I also confirm the fix. Thanks for digging into this so fast @schuay, once you had a reproduction.

[bcoe]

I have confirmed this fix with the nightly.