test: keep coverage reports after coverage-clean

[apapirovski]

This PR adds the coverage folder to .gitignore and removes it from make coverage-clean. I feel like this is a useful change for anyone working on tests. It's nice to be able to keep the coverage reports to reference while writing tests without keeping the rest of the coverage instrumentation (which makes tests slow) and also without having to manually rename the coverage dir (and then avoid adding it to commits).

Checklist

• commit message follows commit guidelines

Affected core subsystem(s)

test, build

[ssbrewster]

That reminds me I need to raise a PR for this but i will take out the conditional on removing the coverage/ dir because it's covered by this PR. I'm also not sure about spinning up another make process.

I was waiting for input on #15214 but that's been long enough now.

[TimothyGu]

Hmm, I'd rather have a separate target for "restore but doesn't actually clean". make clean removes the built binaries as well. So something like:

• make coverage-restore: keeps coverage reports, keeps downloaded tools, restore lib_/
• make coverage-clean: restore lib_/, deletes everything related to coverage (reports, tools, etc.)

@ssbrewster @apapirovski does that sound like something that would satisfy your use cases?

[ssbrewster]

@TimothyGu yes that would work for my use cases i just wonder whether we should run coverage-restore as part of the make coverage target itself so it restores the lib directory after the coverage is generated. Is there any reason for not restoring that? If this is an edge case and the user wants to inspect those files then they could specify that by passing an environment variable to the make command (similar to option 2 in #15214). And we keep the make coverage-clean as a separate target for when the user wants to clean everything up.

[apapirovski]

I'm personally just not sure what the upside is of deleting the coverage reports, especially if they're in .gitignore. The folder is tiny and its useful info.

There are legitimate reasons to run make clean and then build fresh, whereas I don't see the same for coverage.

(I'm fine with any other changes proposed around this behaviour but I would still like to be able to somehow clean & keep just coverage and have it be in .gitignore.)

[BridgeAR]

I personally do not have a strong opinion about either suggested way but I think it is a good thing to make sure we have the possibility of keeping the coverage.

@nodejs/tsc I think it would be good to weight in.

[apapirovski]

Could we get some movement on this by any chance? Thanks! If this change is not wanted, I'm happy to close but would like to resolve one way or the other.

[jasnell]

CI: https://ci.nodejs.org/job/node-test-pull-request/10695/

[thefourtheye]

I don't have a strong preference, but I am leaning towards the idea proposed by @TimothyGu.

[mcollina]

LGTM

[apapirovski]

@TimothyGu @thefourtheye would either of you like to make your objection official? Want to make sure since otherwise this can get merged.

[BridgeAR]

Seems like there are no strong objections and this can land.

If someone objects later on we could change it again anyway.

[mhdawson]

LGTM

[jasnell]

New CI: https://ci.nodejs.org/job/node-test-pull-request/11637/

[jasnell]

there's some red in the CI that should be looked at.

[apapirovski]

It's hard to imagine it's related given that it's this test: async-hooks/test-signalwrap on one specific environment that has been red all day.

[jasnell]

There's also this failure on freebsd... that has not failed in any other test I've seen today. Also likely unrelated but just want to confirm

not ok 264 parallel/test-cluster-send-handle-large-payload
  ---
  duration_ms: 0.562
  severity: fail
  stack: |-
    undefined:1
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    
    SyntaxError: Unexpected token a in JSON at position 0
        at JSON.parse (<anonymous>)
        at Pipe.channel.onread (internal/child_process.js:487:28)

[jasnell]

Ok, quick check through and there's really no way those could have been affected by this change. Will land as soon as CI finishes

[jasnell]

Landed in c6b7052

[MylesBorins]

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.