fix: add global leaks detection on watch-run #4059
Conversation
|
|
|
ref: #1948 |
outsideris
added
type: bug
|
|
||
| before(function() {}); | ||
|
|
||
| after(function() {}); |
There was a problem hiding this comment.
because Runner.prototype.checkGlobals() is invoked every test-end-event and hook-end-event. (https://github.com/mochajs/mocha/blob/master/lib/runner.js#L136-L141)
There was a problem hiding this comment.
That makes sense, but isn't intuitive from the test. Request: how about a quick comment explaining why these are here?
|
👋 coming back to this @anpaul0615, are you still interested in working on this PR? As of #5027 there are again maintainers who can review it now. No worries if you no longer have time - but if you do that'd be great! (I do see that your last message was asking for a naming suggestion - unless directed otherwise we'll try to review it once we're ramped up enough!) |
|
|
||
| before(function() {}); | ||
|
|
||
| after(function() {}); |
There was a problem hiding this comment.
That makes sense, but isn't intuitive from the test. Request: how about a quick comment explaining why these are here?
| const run = () => { | ||
| try { | ||
| beforeRun(); | ||
| resetMocha(mocha); | ||
|
|
||
| if (!initialGlobals) { | ||
| initialGlobals = Object.keys(global); |
There was a problem hiding this comment.
[Style] Now that Mocha doesn't need to support super old versions of IE & the like, let's switch to a Set:
| initialGlobals = Object.keys(global); | |
| initialGlobals = new Set(Object.keys(global)); |
That way the .indexOf(...) === -1 check later can become a .has. A little more idiomatic IMO.
JoshuaKGoldberg
changed the title
JoshuaKGoldberg
added
the
status: in triage
JoshuaKGoldberg
removed
the
status: in triage
There was a problem hiding this comment.
@Uzlopak @voxpelli and I talked about this approach: we think it's promising and gets the job done partially, but there are blocking edge cases around side effects. For example, if the first time a test is run, the test imports cached polyfills that add globals, deleting the global after the test would mean subsequent tests would fail. delete global[k] isn't something that Mocha can reasonably do.
Instead, we think a more workable approach would be to have the runner check for global leaks against the initial state of globals before all tests. I.e. keep the initialGlobals idea, but not the delete global[k] idea. Doing so would mean that necessary globals that can only be introduced exactly one won't be deleted on --watch. (does that make sense?)
One drawback of this proposed approach is that if a leak exists in a test, then is fixed, it'll still be reported on re-runs. Mocha's messaging when --check-leaks and --watch are enabled will have to note that if the leak is fixed then the user will have to restart Mocha.
So, two requests, please:
- fix the behavior so that globals are never deleted, and instead runner checks leaks against the same initial globals every time
- add a unit test that adding a global variable only in the first run is still available in subsequent runs
See also #4954 / #4954 (comment). Part of why we're looking at deprecating the --check-leaks feature long-term is that it isn't comprehensive enough to get many common modern use cases. The only reliable way for a test framework to achieve true encapsulation(-ish) and leak detection is with VMs the way Jest & co do. For Mocha, you'd have to build a plugin and/or use linting and/or / type checking and/or "use strict"; and/or proper ESM.
JoshuaKGoldberg
added
the
status: waiting for author
|
👋 ping @anpaul0615, is this still something you have time for? |
|
It's been over a month since last ping, so closing this out as stale. If you again have time for this & the issue hasn't yet been fixed, feel free to reopen @anpaul0615. If anybody else sees this PR is closed and wants to tackle it, great! Go ahead and send your own PR. If it includes substantial code from this one, please include a co-author attribution. Cheers! 🤎 |
Description of the Change
Fixes #1948.