-
Notifications
You must be signed in to change notification settings - Fork 117
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
Conversation
enable filtering out events from "All Posts" page
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. |
Otherwise very appreciative for the fix to our unit test sadness. |
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 agree with JP, kind of nervous about the version change, but also I'm not sure how much that matters. Thanks for fixing this! :)
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. |
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. |
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.