Skip to content
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

Fix unit test memory leak #4325

Merged
merged 4 commits into from
Dec 8, 2021
Merged

Fix unit test memory leak #4325

merged 4 commits into from
Dec 8, 2021

Conversation

jimrandomh
Copy link
Collaborator

The out-of-memory errors in unit tests were caused by two separate issues each of which leaked lots of memory, adding up to just barely reach the threshold of where things would fail (by increasing slightly every time we added more tests, until it started breaking so we noticed it). The first is jestjs/jest#10550, which leaks about 50MB per test file (about 1GB total). The second was a double-initialization of jest-mongodb which leaks a subprocess and some metadata with each test file (about 200MB total).

There is no proper fix for the Jest module caching leak, but downgrading from nodejs 15+ to nodejs 14 is an effective mitigation which reduces the size of the leak by an order of magnitude. So I've changed the nodejs version in .github/workflows/unitTests.yaml. Peak memory usage is still O(n) in codebase size and in number of test files, which will eventually become a problem, but hopefully by then it will have been fixed upstream.

@jimrandomh jimrandomh requested a review from a team as a code owner December 4, 2021 01:24
@jimrandomh jimrandomh requested review from s-cheng and removed request for a team December 4, 2021 01:24
@jpaddison3
Copy link
Collaborator

I'm nervous about changing the node version only on tests. I'm worried about it causing a situation where we do something that's ok in our development environment, and then tests fail on CI only, don't fail locally, and a dev who wasn't familiar with this PR being incredibly confused.

What's the harm in downgrading to 14 across-the-board? It doesn't look like 15 has that much going for it over 14, and 14 is Long-Term-Stable.

@jpaddison3
Copy link
Collaborator

Otherwise very appreciative for the fix to our unit test sadness.

Copy link
Collaborator

@s-cheng s-cheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with JP, kind of nervous about the version change, but also I'm not sure how much that matters. Thanks for fixing this! :)

@jimrandomh
Copy link
Collaborator Author

I do agree that differences between prod and test are generally bad and to be minimized, so we might consider downleveling prod from 15 to 14. That's a lot riskier than downleveling test, though, so if we do it it should be a separate change with a bit of testing. And test being stuck at 14 should be temporary, until jestjs/jest#10550 is resolved.

@jimrandomh jimrandomh merged commit 11db8dd into master Dec 8, 2021
@radcapitalist
Copy link

I can confirm for my project that downgrading Node from 16.13 to 14.15 greatly mitigated the unit test leak. Our unit tests (81 test files) were able to complete with Node 14.15. With Node 16.13, we only get through 20 test files before the JavaScript heap is out of memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants