Leak detection (--check-leaks) is running before the hooks, preventing cleanup

Offirmo commented 1 year ago

Prerequisites

[x] Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
[x] Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
[x] 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
[x] Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node_modules/.bin/mocha --version(Local) and mocha --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

describe('test', function() {
    function _clean_global_scope() {
        delete globalThis._foo
    }
    before(_clean_global_scope)
    afterEach(_clean_global_scope)

    it('should set the global variable', () => {
        globalThis._foo = 33
        expect(globalThis._foo).to.equal(33)

        // _clean_global_scope() // if we uncomment this line, the test pass
    })
})

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

The output of mocha --version and node_modules/.bin/mocha --version: 10.1.0
The output of node --version: 18.12.1

Additional 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!

JoshuaKGoldberg commented 8 months ago

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

Offirmo commented 7 months ago

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)

JoshuaKGoldberg commented 7 months ago

I don't get any test failures when I try this locally on a basic Mocha project (the new one in https://github.com/mochajs/mocha-examples/pull/73). Neither when the line is commented nor when it's un-commented. So there's something missing. 😄

copy/paste

It's not just copy/paste being asked for, it's also sleuthing together the specifics of the surrounding context.

I'm guessing the expect comes from Chai, maybe it's another package library?
You're not on the latest Mocha (10.2.0), what other packages are out of date?
How are you running Mocha? Browser? CLI? Programmatically with the API?
What other packages are being used with Mocha? Or heck, what's the package manager?

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.

Offirmo commented 7 months ago

Hi @JoshuaKGoldberg thanks for the reminder.

Please find a minimal repro using the latest everything and no expect:

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 afterEach() to have executed.

Thanks for picking my 1 year old issue!

JoshuaKGoldberg commented 7 months ago

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 afterEach hooks... and potentially even after after hooks as well.

But I'm not extremely familiar with how folks in the wild generally use --check-leaks. Maybe there are some folks who are relying on this? For example, they might both be relying on --check-leaks and also running code that coincidentally wipes global variables. If that's a thing, then --check-leaks might need to be configurable as to when leak detection runs.

cc @mochajs/maintenance-crew for input. Marking this as status: in discussion for now. If nobody can back up my anxieties for a while then I'd expect we can treat this as a straightforward bug fix.

Offirmo commented 7 months ago

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:

indeed if we fix this it will be a breaking change and we should, at least temporarily, add an option to switch back to the old behaviour (and ask users who need this option for feedback)
I don't think it would affect many people:
- global variables are rightly shunned in the dev world, they're very rarely used and linters should catch them. my use case was very special (a globalThis polyfill which is not even needed anymore since all browsers and node support it now)
- the check-leaks option is not active by default, so not many people must be using it
- I reported this bug 1y ago and it attracted no activity

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 check-leaks by default? Also fail-zero? IMO all "good, safe" options should be enabled by default and disabled on-demand. What do you think?

JoshuaKGoldberg commented 7 months ago

P.S. different subject, but since we're at it, what's preventing us from enabling check-leaks by default? Also fail-zero? IMO all "good, safe" options should be enabled by default and disabled on-demand. What do you think?

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.

JoshuaKGoldberg commented 7 months ago

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.

JoshuaKGoldberg commented 7 months ago

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: beforeEach, it, afterEach, etc.

describe("test", function () {
  beforeEach(() => {
    globalThis._foo = 33;
  });

  it("should set the global variable", () => {
    delete globalThis._foo;
  });
});

  1) test
       "before each" hook in "test":
     Error: global leak(s) detected: '_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...

...switching the option to an enum like "after-hooks" (current) / "after-tests" (proposed)
...to an external framework, a la jest-leak-detector

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 https://github.com/mochajs/mocha/issues/791 with a mention of edge cases and performance implicications.

voxpelli commented 7 months ago

It was made to run on tests instead of hooks in version 0.0.1-alpha1: https://github.com/mochajs/mocha/commit/c3aaca1766e17f3fdd011b4514c23e3dd498644f

Then was re-added for hooks as well in version 0.0.7: https://github.com/mochajs/mocha/commit/75ebe53718755f3ff3fd019dd0f0d91807d1c5b8

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 globalThis, but eg. doesn't check for anything like JSON.foo = 123 or eg. memory leaks.

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 filterLeaks() has if they are still needed for ones use cases (though it eg. has special cases for ie6,7,8 and old school opera): https://github.com/mochajs/mocha/blob/8317f902a11a5837d00581e7926b145d20f59b61/lib/runner.js#L1186-L1217

Offirmo commented 6 months ago

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 ✅

JoshuaKGoldberg commented 6 months ago

Following up: we'll figure out a long-term plan for --check-leaks on the core team and propose it publicly. We'll set a goal that folks can always have the behavior they rely on now. I.e. any deprecations will be accompanied by at least docs on how to achieve the old behavior.

mochajs / mocha