Skip to content

fix(mocha): fail fast when test hook import chain rejects#5624

Merged
DavertMik merged 1 commit into
advisor/001-promise-core-characterizationfrom
advisor/003-asyncwrapper-import-hardening
Jun 13, 2026
Merged

fix(mocha): fail fast when test hook import chain rejects#5624
DavertMik merged 1 commit into
advisor/001-promise-core-characterizationfrom
advisor/003-asyncwrapper-import-hardening

Conversation

@DavertMik

@DavertMik DavertMik commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

What

In 4.x the per-test mocha hooks setup() and teardown() in lib/mocha/asyncWrapper.js load ./test.js via a dynamic import() whose promise has no .catch(). If that import rejects — or the .then body throws (enhanceMochaTest on an undefined test, a listener throwing) — done() is never called and the mocha hook hangs forever: the silent-hang failure mode the 4.0 release is most concerned with. The sibling suiteSetup()/suiteTeardown() already handle this correctly.

This mirrors the suiteSetup shape exactly — appends .catch(err => doneFn(err)) to both import chains. It converts a potential silent hang into a reported error; it does not by itself fix the known acceptance failures.

Why it's safe

  • No recorder.errHandler is added in setup/teardown — the single module-level errFn slot is owned by the per-test test()/injected() handlers; adding one here would clobber it.
  • makeDoneCallableOnce already guards against a double done() call, so the catch cannot double-fire.
  • The dynamic import stays dynamic (a static import of ./test.js from asyncWrapper.js is an ESM cycle — lib/mocha/test.js imports test back from here).
  • grep -c catch lib/mocha/asyncWrapper.js goes 12 → 14 (exactly +2).

Tests

3 new regression tests in test/unit/mocha/asyncWrapper_test.js:

  • setup() / teardown() with a throwing then-body (a ctx.currentTest getter that throws) call done with the error within 1s instead of hanging.
  • setup() happy path calls done with no error.

Reverting the Step-1 lib edit makes both error-path tests hang/fail (verified locally). With the fix they pass.

Verification

  • npx mocha test/unit/mocha/asyncWrapper_test.js --exit → 12 passing
  • npm run test:unit → 750 passed, 0 failed, 11 skipped
  • npm run test:runner → 273 passed, 0 failed, 2 skipped
  • npm run lint → clean

Dependency

⚠️ Stacked on #5622 (plan 001 — its characterization suite is this change's regression gate). This PR is based on the advisor/001-promise-core-characterization branch; GitHub will retarget it to 4.x once #5622 merges. The first commit here is #5622's; review only the fix(mocha): … commit.

Note: #5622's commit in this branch also carries a one-line robustness tweak to a characterization assertion (onFailed.calledOnce → onFailed.called) that prevents a full-suite ordering flake — that tweak belongs to #5622 and was folded into it.

🤖 Generated with Claude Code
@claude
fix(mocha): fail fast when test hook import chain rejects
ac59a40 
In 4.x the per-test setup()/teardown() hooks load ./test.js via a dynamic
import() with no .catch(). If that import rejects, or the .then body throws
(e.g. enhanceMochaTest on an undefined test, a listener throwing), done() is
never called and the mocha hook hangs forever — the silent-hang failure mode
the 4.0 release is most concerned with.

This mirrors the existing suiteSetup()/suiteTeardown() shape exactly: append a
.catch(err => doneFn(err)) to both import chains. No recorder.errHandler is
added (the per-test handler owns the single errFn slot). makeDoneCallableOnce
already guards against a double done() call.

Adds 3 regression tests: setup() and teardown() with a throwing then-body call
done with the error within 1s instead of hanging, plus a happy-path check.
Reverting the fix makes the two error-path tests hang (verified).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@DavertMik DavertMik merged commit 0705154 into advisor/001-promise-core-characterization Jun 13, 2026
11 checks passed
@DavertMik DavertMik deleted the advisor/003-asyncwrapper-import-hardening branch June 13, 2026 14:29
@DavertMik DavertMik mentioned this pull request Jun 14, 2026
Merged
DavertMik added a commit that referenced this pull request Jun 14, 2026
@DavertMik @claude
fix(core): re-land asyncWrapper + promise-core error-path fixes onto …
eba4a62 
…4.x (#5624, #5633) (#5637)

* fix(mocha): fail fast when test hook import chain rejects

In 4.x the per-test setup()/teardown() hooks load ./test.js via a dynamic
import() with no .catch(). If that import rejects, or the .then body throws
(e.g. enhanceMochaTest on an undefined test, a listener throwing), done() is
never called and the mocha hook hangs forever — the silent-hang failure mode
the 4.0 release is most concerned with.

This mirrors the existing suiteSetup()/suiteTeardown() shape exactly: append a
.catch(err => doneFn(err)) to both import chains. No recorder.errHandler is
added (the per-test handler owns the single errFn slot). makeDoneCallableOnce
already guards against a double done() call.

Adds 3 regression tests: setup() and teardown() with a throwing then-body call
done with the error within 1s instead of hanging, plus a happy-path check.
Reverting the fix makes the two error-path tests hang (verified).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
(cherry picked from commit ac59a40)

* fix(core): restore sessions and propagate errors on session/within/retryTo error paths

Fixes four of the latent error-path divergences characterized in #5622, by
making the error paths symmetric with the success paths. The characterization
assertions are updated to the corrected behavior in the same commit.

- within() async error (lib/effects.js): the catch now calls finishHelpers()
  (so helpers' _withinEnd runs on error, not just success) and returns
  recorder.promise() (so the error propagates through within() instead of being
  detached onto a trailing task that a caller awaiting within() never sees).

- session() async error (lib/session.js): schedules a recorder task that runs
  recorder.session.restore so the recorder session is restored on error (it
  was only restored on success, leaking the session id). The existing
  restoreVars/listener cleanup is kept as-is — switching to finalize()'s real
  restoreVars() closes the browser context under BROWSER_RESTART=session.

- session() sync error (lib/session.js): the finally recorder.catch now calls
  recorder.session.restore before re-throwing (the only place that runs on a
  rejected chain).

- retryTo() (lib/effects.js): a thrown callback no longer reject()s the outer
  promise prematurely — it routes through recorder.throw so the retry logic
  owns the outcome (retry, or reject once maxTries is exhausted). A callback
  that throws then succeeds on a later attempt now resolves instead of
  rejecting. tries now starts at 1 on the first attempt (was 2); the retry
  count is preserved (tries < maxTries).

Verified: unit 748/0, runner 273/0 (incl. all retryFailedStep/rerun tests),
acceptance within/session/els green under both BROWSER_RESTART=browser and
=session.

Not changed here (would be unsafe or out of scope, see PR):
recorder.retries clearing (retryFailedStep depends on it), the stopped-recorder
no-op contract, and nested cross-level restore ordering.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
(cherry picked from commit dca7626)

---------

Co-authored-by: DavertMik <davert@testomat.io>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant

@DavertMik