feat: added a new CLI arg --merge-async to asynchronously and incrementally merge process coverage files to avoid OOM due to heap exhaustion

[bizob2828]

This PR changes the merging strategy of process coverage files to be async and incremental. Our team runs a lot of tests against a versioned matrix of dependencies in subprocesses. This in turns creates a lot of temp v8 coverage files. We were finding we were running out of heap on GitHub actions as you can see in this job

Fixes #338

Checklist

• npm test, tests passing
• npm run test:snap (to update the snapshot)
• tests and/or benchmarks are included
• documentation is changed or added

[AriPerkkio]

Any chance this _loadReports could be kept here, and its API as is? This is something Vitest uses internally. https://github.com/vitest-dev/vitest/blob/708b10fe227c04b4219a1e5d4b539def009729b4/packages/coverage-c8/src/provider.ts#L94-L95

I've been meaning to open a PR here and propose this method as part of public API.

[bizob2828]

Hmm ok I can keep loadReports but this PR fundamentally changes this in that it does not load all reports at once because of the OOM issues.

[bizob2828]

I think maybe I can provide this async merging as a CLI arg and keep existing functionality but that means for libraries that overrode "private" methods you'd have another one to override. @bcoe what are your thoughts?

[AriPerkkio]

Another way to prevent these OOMs would be to make _loadReports a bit more efficient:

1. Load a single V8 report from file system and convert it to JSON
2. Filter its result based on testExclude.shouldInstrument(result[].url) and filter out all unrelated coverage reports
3. Return these filtered ScriptCoverage[] from _loadReport - no other changes required

The V8 reports collected using process.env.NODE_V8_COVERAGE contain so much noise as they include all node_modules and NodeJS internal node:<module-name> modules. When all V8 reports are loaded into memory at once, there is a lot unnecessary memory occupied by these irrelevant coverage reports.

[bizob2828]

hmm. I could look into that. I realized I requested a review from you but I really would like @bcoe to take a look at this. Is this project still alive? there's quite a lot of PRs in the queue

[bcoe]

@bizob2828 one thought, what if we just had both options, with a flag like --merge-async. We could then keep the API the same, but add the new option for your use case.

[bizob2828]

@bizob2828 one thought, what if we just had both options, with a flag like --merge-async. We could then keep the API the same, but add the new option for your use case.

Yea I was thinking of doing this. It also looks like I have to fix 10 and 12 because fs doesn't have promise based methods. I can work on all of this.

[bizob2828]

@bcoe do you mind taking another look? I updated to default to same behavior but added a new --merge-async flag. Also removed some legacy tests that no longer worked which I think should be ok because Node <10 is no longer supported.

[bcoe]

This LGTM, but tests are failing (you may need to retake the snapshot... if the snapshot is actually different, I'm at a loss).

[bizob2828]

This LGTM, but tests are failing (you may need to retake the snapshot... if the snapshot is actually different, I'm at a loss).

@bcoe thanks for reviewing/approving. I see what's up. These tests atomic via chance because they were only run once. Now that they are run twice, merging sync and async it caused issues. I fixed it by providing --clean=true to the one test to delete all v8 coverage files. I made the fixes, can you take one last look please 🙏🏻

[bcoe]

@bizob2828 looks like we're most of the way there, just the Node 10 tests failing (perhaps just didn't run the snapshot for 10).

aside, we should really drop Node 10 support, no good reason to still support it.

[bizob2828]

@bcoe weird, I did re-run and update snapshots and it works locally. I'll dig more, sorry for the back and forth. Looks like it's a timestamp difference which I don't get how that'd ever worked before

[bizob2828]

@bcoe I think I fixed. The report wasn't correct for the same reason another test was failing. Now that the tests are being run more than once we need to clean up beforehand

[bcoe]

@bizob2828 thanks for this great contribution, please let me know if it helps you avoid OOM errors.

[bizob2828]

@bcoe I take it you're shipping this shortly?

[bizob2828]

@bizob2828 looks like we're most of the way there, just the Node 10 tests failing (perhaps just didn't run the snapshot for 10).

aside, we should really drop Node 10 support, no good reason to still support it.

I don't know how much work you want to invest in c8, but I'd be more than happy to do some of this cleanup since it's all still fresh in my head. lmk, I was thinking dropping 10/12/14 as they are out of LTS and cleaning up some of these tests.

[bcoe]

I don't know how much work you want to invest in c8, but I'd be more than happy to do some of this cleanup since it's all still fresh in my head. lmk

@bizob2828 I would be grateful for any contributions. I would be more than happy to drop Node 10 at this point, in a major version, and it would allow the second set of snapshot tests to be removed -- which be a very nice cleanup.

[bizob2828]

@bcoe here's a PR to drop 10.