feat: detect illegally nested tests (#4525)

[mmorearty]

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

PR Checklist

• [x] Addresses an existing open issue: fixes #4525
• [x] That issue was marked as status: accepting prs
• [x] 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[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: mmorearty / name: Mike Morearty (a710267e4db9a84ccd3c12d872bb312e299e8cc2, 0692d3ec741cc1c4a01b0854235e606ac9599d68, b5d8eea22217976491ba0ad078d7c5f1113a6da3, 554fb1f62fd83e767a42fd1f14ed0bb498ceb583, efb05c8212672babc7693da9c7afe64364a8be0e)

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

[mmorearty]

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

[mark-wiemer]

Calling this out for #5357 :)