-
Notifications
You must be signed in to change notification settings - Fork 783
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
Ensure module and test callbacks are released for GC. #1279
Conversation
} ) | ||
.catch( error => { | ||
if ( error instanceof SyntaxError ) { | ||
console.log( "Must launch node 10 with `--expose-gc --track-retaining-path --allow-natives-syntax`" ); |
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.
node >= 9 with --expose-gc --allow-natives-syntax
|
||
// Reset `module.hooks` to ensure that anything referenced in these hooks | ||
// has been released to be garbage collected. | ||
module.hooks = {}; |
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.
something in the suiteEnd probably was using hooks?
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 has been a doozy digging into. I'm still somewhat uncertain, but it seems that the test-on-node
grunt task is leaking tests (not properly waiting for the QUnit.done
that is setup in tests/log.js
which does a setTimeout
inside the done callback).
I've pushed a small cleanup that prevents the failure, but there is definitely some work needed in this area to clean up the leakage between tests.
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 believe that as of this commit, when QUnit is tries to report the "No tests run" error, it is unable to do so now, because:
if (module.hooks[handler].length) {
^
TypeError: Cannot read property 'length' of undefined
at processHooks
I ran into this as part of reproducing #1405 locally.
This issue trolled us hard #1280 |
@trentmwillis - Thoughts? |
FWIW, the following roughly polyfills the changes to work on older QUnit versions: QUnit.testDone(function() {
// release the test callback
QUnit.config.current.callback = undefined;
});
QUnit.moduleDone(function() {
// release the module hooks
QUnit.config.current.module.hooks = {};
}); |
This polyfills the changes from qunitjs/qunit#1279 to older versions of QUnit. --- Prior to the changes in this PR, all module and test callbacks are retained (forever). This may not seem significant, but as folks use closure scope to store data across tests (which is very common). For example, prior to the changes in this PR the following would retain the local variable: ```js QUnit.module('top', function(hooks) { let largeThing; hooks.beforeEach(function() { largeThing = new LargeThing(); }); hooks.afterEach(function() { largeThing.destroy(); }); test('something that uses largeThing', function(assert) { // ...snip... largeThing.someMethod(); }); }); ```
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.
Looks good to me. Nice job on the tests for this as well!
FYI, I'll try to cut a patch release this week. |
@trentmwillis friendly reminder :) |
I just released 2.6.1. Sorry for the delay! |
Awesome, thanks! :) |
Prior to the changes in this PR, all module and test callbacks are retained (forever). This may not seem significant, but as folks use closure scope to store data across tests (which is very common).
For example, prior to the changes in this PR the following would retain the local variable:
In larger test suites, this really adds up...
Based on @krisselden's work in emberjs/ember.js#16609