-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: added a new CLI arg --merge-async
to asynchronously and incrementally merge process coverage files to avoid OOM due to heap exhaustion
#469
Conversation
…process coverage files to avoid OOM due to heap exhaustion.
lib/report.js
Outdated
/** | ||
* Returns the list of V8 process coverages generated by Node. | ||
* | ||
* @return {ProcessCov[]} Process coverages generated by Node. | ||
* @private | ||
*/ | ||
_loadReports () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to prevent these OOMs would be to make _loadReports
a bit more efficient:
- Load a single V8 report from file system and convert it to JSON
- Filter its
result
based ontestExclude.shouldInstrument(result[].url)
and filter out all unrelated coverage reports - 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@bizob2828 one thought, what if we just had both options, with a flag like |
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. |
…mentally merge process coverage files to avoid OOM due to heap exhaustion
--merge-async
to asynchronously and incrementally merge process coverage files to avoid OOM due to heap exhaustion
eb08f1b
to
8d9aeb2
Compare
@bcoe do you mind taking another look? I updated to default to same behavior but added a new |
This was breaking the lib/report.js since it now relies on either `fs/promises` or `fs.promises` depending on the version of Node.js.
8d9aeb2
to
028120e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but tests are failing (you may need to retake the snapshot... if the snapshot is actually different, I'm at a loss).
…ning across different test iterations within the same process
@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 |
@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. |
@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 |
110f767
to
fc8bdb0
Compare
@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 |
fc8bdb0
to
6264919
Compare
running tests more than once now.
6264919
to
18e037a
Compare
@bizob2828 thanks for this great contribution, please let me know if it helps you avoid OOM errors. |
@bcoe I take it you're shipping this shortly? |
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. |
@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. |
…mentally merge process coverage files to avoid OOM due to heap exhaustion (bcoe#469)
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 passingnpm run test:snap
(to update the snapshot)