-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[v25] --detectLeaks option always reports a leak when using jsdom environment #9507
Comments
Thanks! We probably mess with some global or some such we don't cleanup... |
This very minimal test point leaks describe('MemoryLeakTest', function () {
it('should should do nothing', function () {
expect(true).toBeTruthy()
})
}) run with |
I debugged and this issue from |
hi @SimenB , ref jsdom/jsdom#2825 |
You can use https://www.npmjs.com/package/jest-environment-jsdom-sixteen to get v16 in your tests. Jest itself well upgrade the default version in Jest 26 |
@SimenB in Jest 26, |
Jest 26 is months away, and we'll upgrade, not downgrade. So no need for a PR yet 🙂 If you want the old version of jsdom back, you can install |
A monkey-patch as modified by commit 75a921e maintains a reference to the most recent JSDOMParse5Adapter (and all its options) through the push/pop closures. Because the monkey-patches are never cleaned up, there is always a reference to the most recent JSDOMParse5Adapter and its related objects. This fixes jestjs/jest#9507 This may fix jsdom#2757
I was able to reproduce this using the above code snippet, and have tracked the issue down to commit jsdom/jsdom@75a921e released in jsdom 14. It is possible to work around the issue by adding the following code snippet to the async teardown() {
// ...
const OpenElementStack = require('parse5/lib/parser/open-element-stack')
if (OpenElementStack.prototype.pop.toString().includes('OriginalPop')) {
delete OpenElementStack.prototype.pop;
}
if (OpenElementStack.prototype.push.toString().includes('OriginalPush')) {
delete OpenElementStack.prototype.push;
}
}
} edit: I should've read a little closer, I didn't notice that lamhieu-vk already found the issue and put up a PR. I hope the workaround steps are at least helpful for someone. |
@terite |
The monkey-patch for maintaining the open element stack, as modified by commit 75a921e, maintained a reference to the most recent JSDOMParse5Adapter (and all its options) through the push/pop closures. This means there was always a reference from the rooted OpenElementStack.prototype.{push,pop} functions to the most recent JSDOMParse5Adapter and its related objects. This commit instead uses the this.treeAdapter pointer, which already exists in parse5. Doing so also allows us to move the monkeypatch back into the top level, instead of establishing it on construction of each tree adapter. Closes #2831. Closes #2825. Fixes jestjs/jest#9507.
The monkey-patch for maintaining the open element stack, as modified by commit 75a921e, maintained a reference to the most recent JSDOMParse5Adapter through the push/pop closures. This means there was always a reference from the rooted OpenElementStack.prototype.{push,pop} functions to the most recent JSDOMParse5Adapter and its related objects. Since one of the related objects was an element, which has a pointer to a document and window, essentially the whole JSDOM would be retained. This commit instead uses the this.treeAdapter pointer, which already exists in parse5. Doing so also allows us to move the monkeypatch back into the top level, instead of establishing it on construction of each tree adapter. Closes #2831. Closes #2825. Helps with the root cause of jestjs/jest#9507.
This has been fixed upstream in JSDOM now. Unfortunately, since we still support Node 8, we cannot upgrade to a fixed version. @terite do you think it makes sense to add your hack to @jeysal @thymikee thoughts on ^? Alternatively, people can install |
Maybe there's a slight chance that jsdom could release a patch to v15? |
I asked about release a patch to v15 but it is not supported! jsdom/jsdom#2825 (comment) |
hmm... if we upgrade |
Is there a way to use |
@kirillgroshkov sorry, missed your q. It's |
This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
💥 Regression Report
Starting in v25, the --detectLeaks option will always report a leak when using env=jsdom.
Last working version
Worked up to version: v24.9.0
Stopped working in version: v25.0.0
To Reproduce
https://github.com/dylanwulf/jest-25-detect-leak
npm run test
: run test with jsdom environmentnpm run test:sixteen
: run test with jest-environment-jsdom-sixteennpm run test:node
: run test with node environmentWhen using jsdom or jest-environment-jsdom-sixteen it reports a memory leak even though the only thing in the test is a console log. When using the node environment, it does not report a leak.
Expected behavior
Should not report a memory leak for a simple test that clearly does not leak memory.
Link to repl or repo (highly encouraged)
https://github.com/dylanwulf/jest-25-detect-leak
Run
npx envinfo --preset jest
Paste the results here:
The text was updated successfully, but these errors were encountered: