qunitjs / qunit

🔮 An easy-to-use JavaScript unit testing framework.
https://qunitjs.com
MIT License
4.02k stars 784 forks source link

Prevent modules from leaking testEnvironment to parent modules #1710

Closed chriskrycho closed 1 year ago

chriskrycho commented 1 year ago

Today, modules' testEnvironment is leaked to any parent module. This makes it impossible for callers to rely on garbage collection of items attached to the module context, which in turn makes it impossible to test against memory leaks from the code under test. Accordingly, this PR makes two changes:

linux-foundation-easycla[bot] commented 1 year ago

CLA Not Signed

chriskrycho commented 1 year ago

This was a mistaken understanding of the intent here; sorry for the noise!

Krinkle commented 1 year ago

@chriskrycho No problem! Two related thoughts come to mind:

QUnit also supports the, sometimes perceived as more modern, way of sharing values between test cases and between different parent modules, through lexical scope rather than a "this" object.

The "nested" module scope approach (https://api.qunitjs.com/QUnit/module/) is very similar in terms inherence and shadowing, so it's not exactly attractive for being different or better. But, in my experience JS developers tend to be less surprised by how values are shared through lexical scopes, and e.g. less surprised by a module that has let foo; in the parent scope, and then beforeEach() setting foo = …, compared to only a beforeEach that assignsthis.foo = …; and having to guess whether and how that might be inherited or copied etc.

I believe for Ember, some properties are set automatically through automation, so that might be a reason to stick with the "this" object. No pressure of any kind. Both are supported and work perfectly fine.

There's a proposal on the roadmap to apply more inheritence to the "this" object during before() callbacks QUnit 3. If you have ny thoughts on that, I'd love to hear it, over at https://github.com/qunitjs/qunit/issues/1328.