Fix async callback deadlock

[ahamid-fossa]

When mocha test function does not accept and invoke the done callback async style, then mocha-tape-deck can hang because the wrapper promise never resolves.

[ahamid-fossa]

(newer?) mocha will always detect callback style because this.fn.length == 1, but when used with a bare async method, the callback is passed down but then never invoked.

The fix is to detect whether downstream test fn accepts callback - if so pass wrapper, if not call done() ourselves on async method resolution.

[coderabbitai]

Walkthrough

The PR updates the package version to 0.1.2 and modifies the test function wrapper logic in MochaTapeDeck.register to detect whether a test function expects a done callback parameter by checking originalFn.length > 0. Based on this detection, the wrapper either passes the doneWrapper callback or invokes done() directly to resolve the Mocha done handling. Three new regression test cases were added to verify the wrapper's behavior with async functions, callback-style tests, and promise-returning functions.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix async callback deadlock' clearly and concisely summarizes the main change: resolving a deadlock issue with async test functions that don't use the done callback.
Description check	✅ Passed	The description directly addresses the problem being fixed: when mocha test functions don't accept/invoke the done callback, the wrapper promise hangs, and this change resolves that issue.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/index.ts (1)

158-175: ⚠️ Potential issue | 🔴 Critical

Failure path is swallowed; cleanup is never executed.

Line 174 does not call resetNock (it only creates a bound function), and rejection paths are not propagated through completion. This can still leave callback-mode runs hanging on failures.

🔧 Proposed fix

-        testExecutedPromise
+        const completionPromise = testExecutedPromise
           .then(() => {
             if (done) {
               if (originalFnExpectsDone) {
-                return donePromise
-                  .then((res) => {
-                    done(res);
-                  });
+                return donePromise;
               } else {
-                done();
+                return;
               }
             }
           })
           .then(() => this.fnSuffix())
           .then(this.resetNock.bind(this))
-          .catch(() => {
-            this.resetNock.bind(this);
+          .then((res) => {
+            if (done) {
+              done(res);
+            }
+          })
+          .catch((err) => {
+            this.resetNock();
+            if (done) {
+              done(err);
+              return;
+            }
+            throw err;
           });

-          // if we return with a done fn defined, we get the error Resolution method is overspecified.
+        // if we return with a done fn defined, we get the error Resolution method is overspecified.
         if (!done) {
-          return testExecutedPromise;
+          return completionPromise;
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/index.ts` around lines 158 - 175, The promise chain around
testExecutedPromise swallows failures and never actually calls resetNock because
resetNock.bind(this) is not invoked and errors are not re-thrown; update the
chain so resetNock() is called in all paths (use a finally-style then that
invokes this.resetNock()) and ensure the catch handler rethrows the error (or
returns a rejected promise) so caller gets the rejection; also make sure when
originalFnExpectsDone is true you return donePromise.then(res => done(res)) so
the chain waits for the callback, and call this.fnSuffix() only after those
steps complete.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/index.spec.ts`:
- Around line 160-161: The tests are mutating process.env.NO_CASSETTE_MOCKING
inline (e.g., setting origEnv = process.env.NO_CASSETTE_MOCKING;
process.env.NO_CASSETTE_MOCKING = '1') which can leak state if a callback never
runs; extract this setup/teardown into describe-level hooks by moving the
save/restore logic into beforeEach and afterEach for the affected describe
block(s), using a local variable name (e.g., origEnv) scoped to the hooks, and
remove the inline saves/restores from individual tests (affects the lines
referencing process.env.NO_CASSETTE_MOCKING around 160, 177, 186, 200, 209, 222
in the spec); ensure each afterEach restores process.env.NO_CASSETTE_MOCKING to
the saved origEnv (or deletes it if undefined).

---

Outside diff comments:
In `@src/index.ts`:
- Around line 158-175: The promise chain around testExecutedPromise swallows
failures and never actually calls resetNock because resetNock.bind(this) is not
invoked and errors are not re-thrown; update the chain so resetNock() is called
in all paths (use a finally-style then that invokes this.resetNock()) and ensure
the catch handler rethrows the error (or returns a rejected promise) so caller
gets the rejection; also make sure when originalFnExpectsDone is true you return
donePromise.then(res => done(res)) so the chain waits for the callback, and call
this.fnSuffix() only after those steps complete.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9e01fae6-2908-4436-91e4-591279a827d5

📥 Commits

Reviewing files that changed from the base of the PR and between 817eceb and ed8bf2f.

⛔ Files ignored due to path filters (2)

• package-lock.json is excluded by !**/package-lock.json
• yarn.lock is excluded by !**/yarn.lock, !**/*.lock

📒 Files selected for processing (3)

• package.json
• src/index.ts
• test/index.spec.ts

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Extract env-var setup/restore into suite hooks to avoid leakage.

process.env.NO_CASSETTE_MOCKING handling is repeated and callback-coupled. If a callback is not reached, env state can bleed into following tests. Prefer beforeEach/afterEach (or a helper) in this describe block.

Also applies to: 177-178, 186-187, 200-201, 209-210, 222-223

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/index.spec.ts` around lines 160 - 161, The tests are mutating
process.env.NO_CASSETTE_MOCKING inline (e.g., setting origEnv =
process.env.NO_CASSETTE_MOCKING; process.env.NO_CASSETTE_MOCKING = '1') which
can leak state if a callback never runs; extract this setup/teardown into
describe-level hooks by moving the save/restore logic into beforeEach and
afterEach for the affected describe block(s), using a local variable name (e.g.,
origEnv) scoped to the hooks, and remove the inline saves/restores from
individual tests (affects the lines referencing process.env.NO_CASSETTE_MOCKING
around 160, 177, 186, 200, 209, 222 in the spec); ensure each afterEach restores
process.env.NO_CASSETTE_MOCKING to the saved origEnv (or deletes it if
undefined).