fix(core): re-land asyncWrapper + promise-core error-path fixes onto 4.x (#5624, #5633)#5637
Merged
Merged
Conversation
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)
…tryTo 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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this PR exists
#5624 and #5633 show as merged, but their changes never reached
4.x. Both were stacked on theadvisor/001-promise-core-characterizationbranch (the base of #5622). #5622 was squash-merged to4.xfirst; #5624 and #5633 then merged into theadvisor/001branch afterward. So their commits live onadvisor/001and are not on4.x.Verified on current
4.x(20607b70):asyncWrapper.jshas no.catchhardening, andsession.js/effects.jshave none of the error-path fixes.This PR re-lands both deltas directly on
4.x(cherry-picked; no stacking this time).What's included
1.
fix(mocha)— fail fast when test hook import chain rejects (was #5624)setup()/teardown()inlib/mocha/asyncWrapper.jsnow.catch(err => doneFn(err))on the dynamicimport('./test.js')chain (mirrorssuiteSetup/suiteTeardown), so a rejected import reports an error instead of hanging the hook forever.2.
fix(core)— restore sessions & propagate errors on session/within/retryTo error paths (was #5633)within()async error now runs_withinEnd(finishHelpers()) andreturn recorder.promise()(error propagates, not detached).session()async/sync error now restores the recorder session (fixes the session-id leak) — surgical: only addsrecorder.session.restore, keeping the originalrestoreVarscall soBROWSER_RESTART=sessiondoesn't close the browser context (this was the regression that failed Playwright CI on fix(core): restore sessions and propagate errors on session/within/retryTo error paths #5633 and was fixed before merge).retryTo(): a thrown callback routes throughrecorder.throw(no prematurereject) so throw-then-succeed resolves and reject happens only aftermaxTries; first attempt istries === 1(was 2), retry count preserved.session_composition_test.jsupdated to the fixed behavior.Verification (cherry-picked onto current 4.x)
npx mochatarget unit files → 44 passingBROWSER_RESTART=sessionacceptance (session_test.js) → 9 passed, 2 skipped, 0browser context has been closedBoth changes were already CI-green on #5624/#5633 (incl. the full
playwright.ymlsuite across browsers + restart modes); CI will re-run here against current4.x.After merge
The
advisor/001,advisor/003,advisor/006branches can be deleted — their content is now fully represented on4.x(via #5622 + this PR).🤖 Generated with Claude Code