Node: Coverage reporting is broken

[BridgeAR]

The coverage report from the last couple of days is broken (see https://coverage.nodejs.org). I guess it's related to the V8 update. It could also be a @nodejs/build issue but I guess the update is more likely.

// cc @bcoe @nodejs/v8-update

[richardlau]

Can't get to my irc logs at the moment, but I did post some initial investigation in #build. It looks like when it started to break that native addons were being compiled at the same time as the main node binary and this was causing an error which caused the makefile to bail out before running all the tests. No idea what cause it to start happening, but the first build where it started was the one containing the update to V8 7.1.

[targos]

If that's what you say, https://github.com/nodejs/node/pull/23982 would probably fix it.

[richardlau]

hmm http://logs.nodejs.org/node-build/index seems to have stopped on 2018-12-01. I'll copy the logs from home when I get there for reference. I have no idea why the regular CI's aren't running into the problem, but I believe (again from irc) that @addaleax and @Trott were seeing it locally.

[Trott]

If that's what you say, #23982 would probably fix it.

I guess the question is if @bnoordhuis's comment is blocking or not.

[Trott]

hmm http://logs.nodejs.org/node-build/index seems to have stopped on 2018-12-01. I'll copy the logs from home when I get there for reference. I have no idea why the regular CI's aren't running into the problem, but I believe (again from irc) that @addaleax and @Trott were seeing it locally.

I was not seeing it locally. I noticed it broken at coverage.nodejs.org. Haven't tested locally, but I can...

Here's my screenshot of #node-build IRC channel:

[bnoordhuis]

I guess the question is if @bnoordhuis's comment is blocking or not.

It is. Wrong is wrong.

http://logs.nodejs.org/node-build/index seems to have stopped on 2018-12-01

I restarted slurp just now.

[BridgeAR]

Good to know that the V8 update broke the addon to compile. I am running into that issue locally at the moment and I could not figure out what was wrong.

[addaleax]

If that's what you say, #23982 would probably fix it.

I don’t think it would do that, because that’s macOS-only thing and the problem that Rich and I were talking about on IRC is giving me significant problems on Linux.

I haven’t debugged this, but it looks like somehow, the Makefile is broken in the following ways:

1. make always triggers a re-compilation of the node binary. This should not happen if make was successfully run before and no changes have been made since.
2. Building test addons does not wait for the node binary to be finished, but still uses it. This leads to a race condition, where building an addon can fail because the binary is currently being linked; 1. makes this problem worse, because now running make -j4 && make -j4 test is not a viable way to work around the problem anymore.

[targos]

If that's what you say, #23982 would probably fix it.

I don’t think it would do that, because that’s macOS-only thing and the problem that Rich and I were talking about on IRC is giving me significant problems on Linux.

It's not a macOS-only thing. That PR fixes the problem for me on Linux. The change actually excludes macOS because it introduces another issue on that platform.

Also, the Makefile was already broken in the same way before the V8 7.1 upgrade. It was just a lot less likely to see it happen because there were less GYP actions executed to build V8.

[BridgeAR]

Does anyone have an idea how to solve this?

[ZYSzys]

So all the recent informations on https://coverage.nodejs.org/ are misleading right ?

[targos]

Maybe that's a candidate for the new pinned issues feature?

[bcoe]

👋 hello folks; unfortunately was a bit slammed this week with work and didn't get a chance to dig into this. I'm working out of a hotel all next week, and expect I'll have cycles to dig into this if no one beats me to it. _honestly..._

This might be a good time to switch to using NODE_V8_COVERAGE... my major remaining concern was that return statements result in missing lines of coverage, but a for this is close to landing:

https://chromium-review.googlesource.com/c/v8/v8/+/1339119

Which brings us to https://github.com/nodejs/node/pull/23837 being the last major blocker for what I feel is _ideal_ coverage support ... but, I think we could work around this and get things better than the state they've been in this past week.

What do folks think (CC: @schuay, and @hashseed who would be excited to see us moving over to built in coverage).

[mhdawson]

I've been keeping coverage.node.js going so now that I'm back I'll take a look once I've caught up a bit.

[mhdawson]

@bcoe can you make this meeting to discuss when/how it makes sense to switch over to NODE_V8_COVERAGE https://github.com/nodejs/build/issues/1635?

[bcoe]

@mhdawson I'll make an effort to be there. For what it's worth, I have been trying to reproduce the coverage issue on my OSX laptop, and have not been able to -- I swear it did it the first time I ran tests, and now it's working great:

-n Javascript coverage %: 
95.9%
-n C++ coverage %: 
84.9%

heisenbug?

[rvagg]

I'm not really grokking all of this, but I'd love if folks in here could have a think about how we might be able to perform a simple test that coverage works without doing full coverage, then we could possibly integrate that into CI, especially if there's a quick way of doing it. Opening an issue in nodejs/build with suggestions would be helpful.
Since we've had a few occasions where coverage has broken, maybe having it fail CI before commits land would be a good thing to do.

[targos]

I'd like to remind folks that broken coverage is "just" a symptom. I'd very much like to see this fixed at the source (gyp) instead of a workaround that would only make coverage work again.

[mhdawson]

I agree that we need to fix the "problem", but I will see if I can apply the patch temporarily as part of the job so that we don't have invalid coverage data in the mean time.

[mhdawson]

Coverage job is now "hacked" to apply the patch from #23982 and the last run was successful with what look like correct numbers (still waiting for it to replicate to coverage.nodejs.org). This will break easily but at least we will have the coverage data while the actual problem is fixed.

[Trott]

This will break easily but at least we will have the coverage data while the actual problem is fixed.

Is there a separate issue where the "actual problem" is being tracked or would that be this issue here?

[ZYSzys]

The coverage looks great recently.

BTW, do we have any approach to hit these lines which are only available on macOS ?

[Trott]

The coverage looks great recently.

@ZYSzys There was a workaround fix applied by @mhdawson but (IIUC) it's a workaround and not a proper fix. So this issue remains open. (Someone correct me if I'm wrong!)

BTW, do we have any approach to hit these lines which are only available on macOS ?

Please don't use this issue for other things. Open a separate issue (or ask in IRC). Discussions on issues like these tend to go loooong and things get easily lost.

[mhdawson]

I think we should rename this issue to reflect that actual problem. Coverage is working (well was broken for some days but due to a different issue but with the work around is running ok). It might get more attention with a proper title.

[refack]

IIUC this issue can be closed. The make issue has a workaround implemented, and https://coverage.nodejs.org/ is back to showing correct numbers.

Underlying issue is https://github.com/nodejs/node/issues/22006

[mhdawson]

@BridgeAR can you close if you agree?

[BridgeAR]

@mhdawson I am fine with that. I guess the other issue should be pinned in that case.