fix: add global leaks detection on watch-run

[anpaul0615]

Description of the Change

• apply global variable reset process when rerunning test in watch mode
• add some test (that detect global leaks and rerun that test through touch that test file, and then detect global leak again)

Fixes #1948.

[jsf-clabot]

All committers have signed the CLA.

[coveralls]

Coverage increased (+0.1%) to 92.798% when pulling 3c610ca on anpaul0615:issue/1948 into 2de7c18 on mochajs:master.

[outsideris]

ref: #1948

[outsideris]

hooks needed here?

[anpaul0615]

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)

[JoshuaKGoldberg]

That makes sense, but isn't intuitive from the test. Request: how about a quick comment explaining why these are here?

[JoshuaKGoldberg]

👋 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!)

[JoshuaKGoldberg]

Looks great! 🙌

Leaving open and approved for an additional look by @mochajs/maintenance-crew. But only a couple of small nitpicks from me.

[JoshuaKGoldberg]

That makes sense, but isn't intuitive from the test. Request: how about a quick comment explaining why these are here?

[JoshuaKGoldberg]

[Style] Now that Mocha doesn't need to support super old versions of IE & the like, let's switch to a Set:

Suggested change

	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]

Thanks again for working on this - requesting some design changes. Please let us know if we've missed something! 🙂

[JoshuaKGoldberg]

@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.