feat: detect illegally nested tests (#4525)

[mmorearty]

Throws if there is an it() call inside an it() call. Same for test().

PR Checklist

• Addresses an existing open issue: fixes 🚀 Feature: Throw on nested it call #4525
• That issue was marked as status: accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

Adds some state to keep track of whether we are currently inside a test. If we are, and we enter a new test, throws an error.

helpful failure messages

If a nested test is detected, the failure that is logged shows the name of both
the outer and inner test. For example:

Error: Nested test "my inner test" detected inside test "my outer test". Nested tests are not allowed.

new tests

I added a bunch of tests:

• Both BDD and TDD
• Three kinds of async tests: callback, promise, and async/await
• Test that the test names are included in the error message, as noted above

global variable

I could find no existing state that was visible and allowed me to determine
whether we were already inside a test. Therefore, this PR creates a new global
variable, global._mochaExecutionState. If there is a better, non-global place
to store this, that would be great.

old failing tests

I don’t know why, but npm test had some failing tests even before my changes.
They are unrelated to my proposed changes.

minor code duplication and code complexity

Before my change, in lib/interfaces/{tdd.js,bdd.js} the main test functions,
context.test and context.it, were super short and simple. I don’t love the fact
that my change adds a bit of complexity to both (and it’s the same code in both
-- but on the other hand, those two functions already contain identical code).

risk of breaking existing tests

I want to point out that even though this is a desirable feature, I think there
is significant risk of breaking people’s current tests. I became aware of this
nested-test issue when I worked at Asana, and over our very large codebase,
there were at least 20 tests that had this problem. It took a lot of work to
fix them all.

If Mocha just made this change, this breakage could cause people to avoid
upgrading to more recent versions.

Mitigation options to consider:

• Add an option to disable the nested-test checks. My suggestion would be that
  nested-test checks are ON by default, but can be turned off. This way, when
  someone upgrades Mocha, they may get errors, but they can still keep the new
  Mocha without having to stop and invest a lot of time in fixing their tests.
  Also, on by default means that if anyone uses Mocha in a new project, they
  will get the nested-test checks.
• Decide what Semver version bump is appropriate. If the new checks are on by
  default, then this could be considered a breaking change, which would mean a
  major version bump.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: mmorearty / name: Mike Morearty (a710267, 0692d3e, b5d8eea, 554fb1f, efb05c8)

[JoshuaKGoldberg]

Thanks for starting on this! I think there should be a way to avoid globals.

[mmorearty]

Absolutely -- I didn't want to use a global variable, but didn't understand the structure of Mocha well enough to avoid it. But I think I have it working now.

When the test runner is called, it is passed a suite. I now store a currentRunnable on the suite. (I could not find an existing variable that worked for what we're doing here.)

Also, I tweaked it so that it also detects tests nested inside of hooks. For example, this is now caught (although this is far less likely to occur than a test inside a test):

before('my hook', () => {
  it('should not have this test inside a hook', () => {});
});

Thanks for the feedback; please let me know if any changes are needed.

[JoshuaKGoldberg]

Looks great to me, nice! I fiddled with it a bunch and couldn't find

No blocking review comments from me. Just one small "it's fine either way" consideration. But, since the backing issue #4525 is a semver-major breaking change, we're going to have to wait to merge this until we're prepping a major version with breaking behavioral changes.

[mmorearty]

Hi @JoshuaKGoldberg just checking in to make sure you saw that last week I addressed the point you raised about duplicate code :)

[JoshuaKGoldberg]

Lovely!

No blockers from me other than just waiting for a major version. Thanks!