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
Leak detection (--check-leaks) is running before the hooks, preventing cleanup #4954
Comments
@Offirmo could you please post a standalone reproduction? As in, something one of us can clone locally and run a couple quick commands to see the issue? It's hard to triage this with just a code block + plaintext description. |
Hi @JoshuaKGoldberg thanks for responding. I posted a standalone test above that trivially produce the issue. Is it inconvenient to copy/paste? Do I really need to setup a full repo? (I can if that help) |
I don't get any test failures when I try this locally on a basic Mocha project (the new one in mochajs/mocha-examples#73). Neither when the line is commented nor when it's un-commented. So there's something missing. 😄
It's not just copy/paste being asked for, it's also sleuthing together the specifics of the surrounding context.
Having a single unified reproduction that a maintainer can quickly get working on my computer bypasses all that questioning. It makes sure we can get exactly our issue, quickly, without guesswork. https://antfu.me/posts/why-reproductions-are-required is a good blog post that goes into this more. |
Hi @JoshuaKGoldberg thanks for the reminder. Please find a minimal repro using the latest everything and no https://github.com/Offirmo/bug-report--mocha-202401 To re-iterate the issue, I suspect mocha's "leak checker" is running too soon, it should wait for the Thanks for picking my 1 year old issue! |
Ok swell thanks - confirmed that reproduction works for me! And it's very straightforward, thanks for that. 😄 I can definitely see why this behavior isn't optimal for cases like this. It feels to me like the right fix would be to have leak detection done at the very least after But I'm not extremely familiar with how folks in the wild generally use cc @mochajs/maintenance-crew for input. Marking this as |
Hi @JoshuaKGoldberg , thanks for taking the time to look at this bug and thanks as well for your care in protecting backward compatibility 👍 My 2 cts on your questions:
Hence IMO it shouldn't affect many users and this should be a straightforward bug fix. It's just my personal 2cts of course, I acknowledge the world is vast :) Have a nice week-end. P.S. different subject, but since we're at it, what's preventing us from enabling |
Heh, I think just inertia. For a while Mocha wasn't actively maintained. And now per #5027 we're trying not to make too many splashy changes. I'd welcome an issue each for those two suggestions - maybe in some number of months we'd feel confident enough to look at changes like that. But I suspect it'd break enough people that it'll be a while, if ever. |
Oh! Just found #2206 - which is a similar request. Since this issue has the great reproduction (❤️) I'll close that one out as a duplicate of this one. |
At first this seemed like a straightforward bug. But @voxpelli and I took a deeper look and found that leak detection has for many years run after every phase of tests: describe("test", function () {
beforeEach(() => {
globalThis._foo = 33;
});
it("should set the global variable", () => {
delete globalThis._foo;
});
});
So if the behavior were to be resolved to work as the issue expected, it would be a big switch for the leaks. It'd have to be made more configurable, such as...
It's not clear to us whether it'd be worth the work to keep this around in the enum form. It was switched from opt-out to opt-in in #791 with a mention of edge cases and performance implicications. |
It was made to run on tests instead of hooks in version 0.0.1-alpha1: c3aaca1 Then was re-added for hooks as well in version 0.0.7: 75ebe53 So essentially: This has always been detected the way it is tested right now and I think changing that now 10+ years later is to make it into something that it has never been. The naming is also very unfortunate – it doesn't check for leaks, it checks for pollution of global namespaces, something that was a real struggle back in the jQuery days (probably why it was detected by default then) but which in today's world of modules is much less of an issue, especially considering that strict mode prohibits implicit assignment to global scope. I think the use case for this option is nowadays probably better solved through linting like: https://eslint.org/docs/latest/rules/no-implicit-globals Current implementation has special browser specific heuristics to be able to diff top level keys on My vote would be: Deprecate in next major release, remove in release afterwards and suggest that people do such tests themselves by eg. doing: describe("test", function () {
let globalThisKeys;
beforeEach(() => {
globalThisKeys = Object.keys(globalThis);
});
afterEach(() => {
if (Object.keys(globalThis).some(key => globalThisKeys.includes(key) === false)) {
throw new Error('Leaked global');
}
});
it("should set the global variable", () => {
globalThis._foo = 123;
});
}); Possibly adding in some of the same heuristics as the Mocha-internal Lines 1186 to 1217 in 8317f90
|
Too bad! Even imperfect, this feature was very useful! However I agree that the current implementation is too awkward and worth deprecating and removing. That could indeed be re-implemented properly as a plugin ✅ |
Following up: we'll figure out a long-term plan for |
Prerequisites
faq
labelnode_modules/.bin/mocha --version
(Local) andmocha --version
(Global). We recommend that you not install Mocha globally.Description
I'm unit testing a code which deals with global variables (intended).
Mocha leaks detection worked well and detected that I was leaking my unit test global variables.
I then proceeded to use hooks in attempt to clean the global scope. However it seems mocha's leak detection is running BEFORE the hooks, making it impossible to clean the global scope in hooks. I believe that's not what we want.
Steps to Reproduce
Expected behavior: [What you expect to happen]
No error.
Actual behavior: [What actually happens]
Error: global leak(s) detected: '_foo'
Reproduces how often: [What percentage of the time does it reproduce?]
100%
Versions
mocha --version
andnode_modules/.bin/mocha --version
: 10.1.0node --version
: 18.12.1Additional Information
If I manually call
_clean_global_scope()
at the end of the unit test, the leak is not reported. However the point of the hooks is to avoid this duplication.Also thanks for this trusty tool that I'm using for so many years!
The text was updated successfully, but these errors were encountered: