Repository navigation
fix(cli): record a timed-out mocha test's failure when mocha reports it (SDK-7843) - #272
Conversation
…it (SDK-7843) wdio runs a test's afterTest inside the test's runnable, after the body. When a test hits mocha's timeout, mocha fails it while the body and its afterTest are still pending. With `bail`, or when it is the worker's last test, wdio then runs after() BEFORE that afterTest. On the CLI flow everything after() does then runs without the failure: - AutomateModule.onAfterExecute marks the session `passed`, because its results only come from afterTest. - The test's TestRunFinished is stranded, so Test Hub reaps the build as `timeout` about 60 min later. The classic flow never had this, because it reads the session status from after(result), which is mocha's own failure count. Since 9.39.3 the CLI also boots for App Automate accounts whose bin-session config has no `apis` (#261), so those accounts now hit it. One customer had 78 timed-out sessions marked passed in 6 days, against 0 the week before. Fix at the source: the service registers a finisher per mocha test in beforeTest (cli/earlyTestFinish.ts). The reporter's onTestFail, which mocha drives at the moment it fails the test, reports the failure through it with mocha's own result. after() awaits those finishes before its deferred-finish flush and EXECUTE/POST. The late afterTest then sees the test was already reported and stands down. A normal failure is still reported by afterTest, which runs before mocha's `fail`, so the reporter is a no-op for it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
…ogging an ERROR (SDK-7843) Console output from wdio's `before` hook reaches trackEvent(LOG, POST) before mocha's first hook, when no test or hook instance exists, so resolveInstance printed "resolveInstance: unable to resolve/create instance ... LOG POST" on every worker. Drop it at debug level, as the classic path does. Same change as #264. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
Automated SDK PR Review
Verdict: TestReporter is registered. Customers on the CLI flow with mocha and Test Reporting, Accessibility and Percy all off still get passed sessions on a mocha timeout. Add a fallback that doesn't depend on the reporter, or scope the release note. The 5 suggestions don't block the merge.
Summary: 0 critical · 1 warning · 5 suggestions across 9 files reviewed.
Intent: On the CLI flow, a mocha test that hits mocha's timeout (with bail, or as the worker's last test) had its session marked passed and got no TestRunFinished, because wdio runs afterTest inside the test runnable and after() ran before the late afterTest. The PR records the failure at the moment mocha reports it: beforeTest registers a per-test finisher (cli/earlyTestFinish.ts), the reporter's onTestFail runs it with mocha's own result, afterTest stands down for a test already reported, and after() awaits those finishes before the deferred-finish flush and EXECUTE/POST. The CLI finish moves unchanged into finishCliTest(). It also carries #264's change: a LOG event with no tracked instance is dropped at debug level instead of logging the resolveInstance ERROR. Surfaced by #261, which let the CLI boot for App Automate accounts that used to fall back to the classic flow.
Risk: Medium. It changes when a mocha test's finish is reported on the CLI flow, for Automate and App Automate session status and for Test Reporting. The classic flow and non-mocha frameworks are untouched.
0 critical · 1 warning · 5 suggestions | Files reviewed: 9
Rovo enrichment unavailable — review based on local SDK docs only.
The verdict hinges on one coverage gap. The event-order fix is correct against the @wdio/utils testFnWrapper, @wdio/mocha-framework and mocha Runner source, but it only engages when TestReporter is registered.
Not anchored inline:
- Cross-framework (#23): Jasmine on the CLI flow wraps
afterTestin the same testFnWrapper, so a spec timeout under Jasmine may have the sameafter()-before-afterTestordering. This is not verified and is out of scope here; Cucumber'safterScenarioruns outside the step, so it is unlikely to be affected. The classic flow is untouched:onTestFailreturns when the CLI isn't running, and everyservice.tschange is insideisRunning(). - v8 port #273: it intentionally diverges. There is no uuid restore, no bail cascade and no flush ordering, because those don't exist on v8. Its changeset accordingly claims only the session-status fix. It copies the same unreachable
try/catch(Suggestion 2) and has the same reporter-registration dependency (Warning 1); please check v8's reporter gate. - #5 (error boundaries): each reporter-started finish has a
.catch, so a rejection can't breakafter(). The wait is not time-bounded, which matches the existing unboundeddrainSkipReports/ flush /EXECUTE/POSTawaits inafter(). - Rovo enrichment unavailable — review based on local SDK docs only.
See inline comments below for full Problem and Suggested Fix detail on each finding.
Generated by Automated SDK PR review.
| * marks the session and closes the test with it. A normal failure is already reported by | ||
| * `afterTest` before mocha's `fail`, so this is a no-op for it. | ||
| */ | ||
| onTestFail(testStats: TestStats) { |
There was a problem hiding this comment.
⚠️ Warning — [CORRECTNESS] The fix depends on TestReporter, which is not registered when Test Reporting, Accessibility and Percy are all off
Problem
The only thing that reports a timed-out test before after() is TestReporter.onTestFail. The service adds that reporter only when shouldProcessEventForTesthub('') is true (service.ts:126-128). That needs LTS, or one of BROWSERSTACK_OBSERVABILITY, BROWSERSTACK_ACCESSIBILITY or BROWSERSTACK_PERCY set to 'true' (testHub/utils.ts:47-63).
The session-status side doesn't have that gate. The CLI boots for every supported framework that isn't multiremote (launcher.ts:357-361). AutomateModule is always loaded (cli/index.ts:162), and it marks the session in onAfterExecute from the TEST/POST results.
So an Automate or App Automate customer on the CLI flow with mocha and Test Reporting off hits the original bug unchanged. A timeout under bail, or on the last test, still gives an empty results map at EXECUTE/POST, so the session is marked passed. The same happens when Test Reporting is switched off at runtime after a failed build-start (handleErrorForObservability sets BROWSERSTACK_OBSERVABILITY='false').
The changeset and the customer-facing release note say the Automate / App Automate dashboard status is fixed, with no condition. All six verification runs had Test Reporting on, so none of them covers this configuration.
Suggested Fix
Option A (recommended): a reporter-independent fallback in after(). In beforeTest, capture the live mocha runnable (context.test) alongside the finisher. In after(), after awaitCliTestFinishesOnFailure() and before the flush, finish every finisher that is still registered and whose runnable has state === 'failed', using { passed: false, error: runnable.err }. mocha's Runner#fail sets test.state = 'failed' before it emits fail (lib/runner.js:440). This covers Test-Reporting-off workers. When the reporter is present it is a no-op, because it already claimed the finisher.
Option B: register the reporter whenever the CLI flow runs mocha, even when shouldProcessEventForTesthub('') is false. You would then have to confirm that every other TestReporter handler is gated by needToSendData / shouldProcessEventForTesthub, so that nothing new goes to Test Hub. That is a larger surface to audit.
With either option, add a test where the reporter is absent (testObservability: false, no a11y/percy env) and a timed-out test still reaches EXECUTE/POST as failed. If you defer this, scope the release note to "with Test Reporting enabled".
Confidence: 🟢 Objectively verifiable: the registration gate (service.ts:126, testHub/utils.ts:47-63) and the unconditional AutomateModule load (cli/index.ts:162) are both in the head commit.
There was a problem hiding this comment.
Valid, fixed in 7c9e8c0 (Option A). beforeTest now registers the live mocha runnable (test.ctx.test, read before any await) alongside the finisher. In after(), awaitCliTestFinishesOnFailure() first finishes every finisher still registered whose runnable has state === 'failed' (set by Runner#fail before it emits fail), with passed: false, mocha's duration, and Timeout of <ms>ms exceeded. when timedOut is set. It then awaits all finishes, before the flush and EXECUTE/POST. When the reporter is registered it already claimed the finisher, so this is a no-op. New test: no reporter, the runnable is failed after beforeTest, and after(1) still sends TEST/POST passed=false before the flush and EXECUTE/POST. Mutation-checked: dropping the fallback fails it. Same change in #273 (ab144e5), which had the same reporter gate (service.ts:95).
| * test timed out), in which case afterTest must not report it again. A test that was never | ||
| * registered still belongs to afterTest. | ||
| */ | ||
| export function claimCliTestFinish(identifier: string): boolean { |
There was a problem hiding this comment.
💡 Suggestion — [CORRECTNESS] Finishers are keyed per test, not per attempt; a mocha retry can hand the next attempt's finisher to the previous attempt's late afterTest
Problem
With this.retries(n), a timed-out attempt that will be retried makes mocha emit retry, not fail (lib/runner.js:813-819), so the reporter does nothing. mocha starts the next attempt straight away. Its beforeTest overwrites the finisher, and _cliTestUuids, under the same ${parent} - ${title} key.
When the first attempt's body finally resolves, its late afterTest calls claimCliTestFinish(id). That deletes the second attempt's finisher and returns true. finishCliTest then closes the second attempt's test_run (it restores the second attempt's uuid) with the first attempt's result.
If the last attempt then times out too, onTestFail finds no finisher, and the original bug comes back for that run. Most of this was already true before the PR, but this PR's guarantee doesn't hold under retries.
Key the hand-off per attempt: append the attempt to the id. On the service side that is the snapshot's _currentRetry (the {...context.test} copy carries it). On the reporter side it is testStats.retries, which WDIOReporter increments on test:retry.
There was a problem hiding this comment.
Valid, fixed in 7c9e8c0. The hand-off and the _cliTestUuids snapshot are now keyed per attempt with cliTestAttemptKey(id, attempt). The service takes the attempt from _currentRetry on the {...context.test} copy (the same copy afterTest gets as originalTest), and the reporter takes it from TestStats.retries, which WDIOReporter sets at test:start from its retry count and resets at test:end. Both are 0 for the first attempt, which keeps the plain identity. A retried attempt's late afterTest now claims and closes its own attempt, with its own uuid, and the next attempt stays claimable by onTestFail. Tests: unit (separate keys), reporter (retries: 1 uses the attempt key), service (attempt 0's late afterTest, then attempt 1 timing out → two TEST/POSTs, uuid-1 then uuid-2). Mutation-checked.
| // deferred into that stash) and before EXECUTE/POST, which marks the session status | ||
| // from the results recorded so far. | ||
| try { | ||
| await awaitCliTestFinishesOnFailure() |
There was a problem hiding this comment.
💡 Suggestion — [ANTI-PATTERN] try/catch around awaitCliTestFinishesOnFailure() cannot fire (#26)
Problem
finishCliTestOnFailure stores finisher(result).catch(...) in inFlight, so every promise in it already resolves, and Promise.all over them cannot reject. Nothing else in the helper can throw. The try/catch and its Exception awaiting failed-test finishes log are unreachable. They suggest a failure mode that doesn't exist (sdk-anti-patterns #26).
Drop the wrapper, or keep the single error boundary inside earlyTestFinish.ts, where it already is. #273 copies the same wrapper.
| } | ||
| } | ||
| await BrowserstackCLI.getInstance().getTestFramework()!.trackEvent(TestFrameworkState.LOG_REPORT, HookState.POST, { test, result: results }) | ||
| await BrowserstackCLI.getInstance().getTestFramework()!.trackEvent(TestFrameworkState.TEST, HookState.POST, { test, result: results, suiteTitle: this._suiteTitle }) |
There was a problem hiding this comment.
💡 Suggestion — [CORRECTNESS] The uuid is restored before LOG_REPORT's await, but TEST/POST reads it after; on the reporter path mocha keeps running in between
Problem
finishCliTest writes the timed-out test's uuid onto the single tracked instance and then awaits LOG_REPORT/POST. Only after that does TEST/POST read the uuid back from the instance.
When the finish starts from onTestFail, mocha doesn't wait for it. It goes straight on to the afterEach / after all hooks (wdio beforeHook/afterHook → CLI hook events) and, without bail, to the next test's INIT_TEST, which overwrites that slot. Any of these can land inside the LOG_REPORT await.
The old late-afterTest path had the same window, and the no-bail real run (mjhq2iqi…) came out right. Still, the new path starts the finish exactly when mocha moves on, which makes the overlap more likely. Re-apply the resolved uuid right before TEST/POST (or pass it explicitly) to close the window.
There was a problem hiding this comment.
Valid, fixed in 7c9e8c0. finishCliTest pins the resolved uuid before LOG_REPORT/POST and again right before TEST/POST. If the second pin replaced another test's uuid (the next test's INIT_TEST ran during the LOG_REPORT await), it hands the slot back to that uuid once TEST/POST returns, so the running test's later events don't pick up the old uuid. New test with a real slot: the finish starts on failure, the next test's beforeTest mints uuid-2 during LOG_REPORT, the TEST/POST still carries uuid-1, and the slot ends at uuid-2. Mutation-checked for both the re-pin and the hand-back. Not applicable to #273: v8 has no uuid snapshot/restore, so this PR doesn't change how v8 reads the uuid.
| // arrives before mocha's first hook, so there is no test or hook to attach it to yet and | ||
| // resolveInstance cannot create one for LOG. The classic path drops such a log silently; do the same instead of | ||
| // printing an ERROR on every worker (SDK-7843). | ||
| if (testFrameworkState === TestFrameworkState.LOG && !TestFramework.getTrackedInstance()) { |
There was a problem hiding this comment.
💡 Suggestion — [PROCESS] The pre-test LOG change duplicates open PR #264 (and #267 is still open)
Problem
This hunk, its test file and its release-note bullet match #264 (still OPEN against main). The change itself is sound: it only short-circuits LOG when getTrackedInstance() is null, and the TEST/hook paths still log the ERROR, which a test covers.
If both PRs merge, the changelog gets the resolveInstance bullet twice, and whichever merges second conflicts. Close #264 as folded into this PR, or drop the hunk here. #267 (the rejected late-finish approach, which this PR replaces) is also still OPEN; close it so it isn't merged by mistake. The v8 pair (#265 / #273) has the same overlap.
There was a problem hiding this comment.
Intentional: the change was folded in here (and into #273) on purpose, so one 9.x release fixes both symptoms the SDK-7843 customer reported. #264/#265 are superseded and #267 (the rejected late-finish approach) is replaced by this PR; they'll be closed so they can't be merged by mistake or duplicate the changelog bullet.
There was a problem hiding this comment.
Considered — an author comment can't ground its own finding (Rule A); packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts·WdioMochaTestFramework.trackTestEvent still needs a non-author reviewer's explanatory confirmation.
— SDK PR Review Agent
There was a problem hiding this comment.
Considered — an author comment can't ground its own finding (Rule A); packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts·WdioMochaTestFramework.trackTestEvent still needs a non-author reviewer's explanatory confirmation.
— SDK PR Review Agent
There was a problem hiding this comment.
| vi.restoreAllMocks() | ||
| }) | ||
|
|
||
| it('reports the failure, then marks the session, then ignores the late afterTest', async () => { |
There was a problem hiding this comment.
💡 Suggestion — [TESTING] No retry case, and the timeout+bail service test never exercises the bail cascade
Problem
The new tests cover reporter-first finishing, a normal failure staying with afterTest, an unregistered test, after() waiting for an in-flight finish, and a rejecting finish. The body also reports that two of them were mutation-checked.
Gaps remain:
- Bail cascade:
mochaOpts.bailis on, buttimedOutTest.ctx.testhas noparent, soreportBailSkippedTestsreturns early. The test never shows that the cascade (skipped tests'TestRunFinished) runs from the reporter path and finishes before the flush. - Retries: no case for mocha retries (see Suggestion 1).
- No reporter: no case for a worker without the reporter (see Warning 1).
Give ctx.test.parent a root suite with one unreached test, and assert that its skipped TEST/POST lands before flushPendingTestFinishEvent.
There was a problem hiding this comment.
Valid, added in 7c9e8c0. Bail cascade: ctx.test.parent is a suite under a root with one unreached test, and the test asserts the skipped TEST/POST lands after the failed one and before flushPendingTestFinishEvent. Retries: see the retry thread. No reporter: see the reporter-gate thread. Plus the uuid re-pin case. 23 SDK-7843 tests in all on main. The full suite has the same 78 failures as main (pre-existing), with 1328 passing. tsc and eslint are clean.
- Finish a test mocha already failed from after() when no reporter claimed it, using mocha's own runnable, so the fix holds with Test Reporting, Accessibility and Percy all off (the reporter is only registered when Test Hub events are on). - Key the finish hand-off and the uuid snapshot per retry attempt, so a retried attempt's late afterTest closes that attempt and not the next one. - Pin the test's uuid again right before TEST/POST, since mocha moves on while a finish started on failure awaits LOG_REPORT, then hand the slot back to the test now running. - Drop the try/catch around awaitCliTestFinishesOnFailure(); every finish catches its own error. - Tests: no reporter, retries, bail cascade from the reporter path, uuid re-pin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
- Finish a test mocha already failed from after() when no reporter claimed it, using mocha's own runnable, so the fix holds with Test Reporting, Accessibility and Percy all off (the reporter is only registered when Test Hub events are on). - Key the finish hand-off per retry attempt, so a retried attempt's late afterTest closes that attempt and not the next one. - Drop the try/catch around awaitCliTestFinishesOnFailure(); every finish catches its own error. - Tests: no reporter, retries. Same changes as #272. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
Automated SDK PR Review
Verdict: ✅ Good to go — W1 and S1–S3, S5 are resolved, and S4 is an accepted fold-in. Two non-blocking suggestions: (1) src/service.ts is 🟡: the no-reporter fallback finishes a mid-spec timed-out test on the currently tracked instance, which can mis-attribute Test Reporting data when TestHubModule is loaded without the reporter. (2) Refresh the stale verification counts in the PR body and add a real run with Test Reporting off. Close #264, #265 and #267 before merging.
Summary: 0 critical · 0 warnings · 2 suggestions across 9 files reviewed.
This is a re-review at 7c9e8c09. The no-reporter fallback (runnable.state === 'failed' → finish in after()) holds against the wdio v9.32.0 hookArgsFn ({ ...context.test } keeps ctx; mocha's Context#runnable sets ctx.test to the live Test) and mocha 10.8.2's Runner#fail (state = 'failed' before fail is emitted). It is a no-op when the reporter has already claimed the attempt. Retry keys match on both sides.
Prior findings status
| # | Prior finding | Status | Evidence |
|---|---|---|---|
| W1 | TestReporter |
RESOLVED | Option A: const runnable = test.ctx?.test (read before any await) → registerCliTestFinisher(key, fn, runnable); awaitCliTestFinishesOnFailure() finishes unclaimed entries with runnable?.state === 'failed' before the flush and EXECUTE/POST. New no-reporter test. |
| S1 | Per-test, not per-attempt keys | RESOLVED | cliTestAttemptKey(id, test._currentRetry) (service) and cliTestAttemptKey(id, testStats.retries ?? 0) (reporter) give the same number (@wdio/reporter test:start/test:retry/test:end). |
| S2 | Unreachable try/catch (#26) |
RESOLVED | Bare await awaitCliTestFinishesOnFailure(), with the comment "Every finish catches its own error, so this cannot reject." |
| S3 | uuid read after the LOG_REPORT await |
RESOLVED | Local resolvedUuid; pinUuid() before LOG_REPORT and before TEST/POST, then the slot is handed back. The residual is in Suggestion 1. |
| S4 | Overlap with #264 / #267 | DECLINED-ACCEPTABLE | Folded in on purpose. #264, #265 and #267 are still OPEN; close them before merging. |
| S5 | Test gaps | RESOLVED | Bail cascade with a real parent chain, retry (uuid-1 then uuid-2), no-reporter and uuid re-pin cases added. |
| — | #23 Jasmine note / #5 unbounded wait | Unchanged, acceptable | Out of scope; consistent with the existing after() awaits. |
| — | #273 divergence | Addressed | ab144e5 carries the fallback, the attempt keys and the cleanup. |
Not anchored inline:
- [PROCESS] PR description: the verification is stale and has no real run for the two new paths. The "Review follow-up" bullets are current, but the Verification section still says "Tests (11 new)" and "78 failed / 1319 passed". The follow-up reports 23 tests and 1328 passed. The fix section still says the fix relies on the reporter, which "already runs in every worker when Test Reporting is on". All six real sessions ran with Test Reporting on, and there is no real run with Test Reporting, Accessibility and Percy all off, or with
mochaOpts.retries. The release note now claims the fix without conditions. That is right by construction, because the fallback covers the no-reporter worker, but that path is unit-tested only. Add one App Automate or Automate run withtestObservability: false(expect the session to befailedwith the timeout reason), and update the stale counts. - Rovo enrichment unavailable — review based on local SDK docs only.
See inline comments below for full Problem and Suggested Fix detail on each finding.
Generated by Automated SDK PR review.
| pinUuid() | ||
| await BrowserstackCLI.getInstance().getTestFramework()!.trackEvent(TestFrameworkState.LOG_REPORT, HookState.POST, { test, result: results }) |
There was a problem hiding this comment.
💡 Suggestion — [CORRECTNESS] The no-reporter fallback finishes a mid-spec timed-out test on whichever test instance is tracked at after()
Problem
The fallback in awaitCliTestFinishesOnFailure() runs finishCliTest() from after(). finishCliTest() re-pins only the uuid; LOG_REPORT/POST and TEST/POST still act on TestFramework.getTrackedInstance().
For a bail or last-test timeout that is the timed-out test's own instance, because after each / after all reuse it (resolveInstance only mints a new instance at a BEFORE_* / INIT_TEST boundary). The new tests cover that shape. A mid-spec timeout without bail, whose body is still pending when the worker ends, is different. Take test A timing out, then B…Z running, with A's body still hung at after():
- The tracked instance is Z's.
LOG_REPORT/POSTrunsloadTestResult()on Z's instance and overwrites Z's result with A's failure. TEST/POSTstashes A's finish (uuid A) withargs.instance= Z's instance. Z's own finish is still inpendingTestFinishes, andsendTestFrameworkEvent()reads the data fresh from the instance at flush time. So the flush inafter()sends Z'sTestRunFinishedwith A's failure, and A's with Z's name and data.- The first
pinUuid()'s return value is dropped, so Z's instance keeps uuid A afterwards. That is harmless here, because the stash carries its uuid explicitly.
The session status is unaffected and correct, because AutomateModule.onAfterTest reads args.result, not the instance. The impact is limited to Test Reporting data, and only when TestHubModule is loaded while TestReporter is not, for example after the Test Reporting flag is turned off at runtime by handleErrorForObservability. Before this PR, A was never finished at all, so this is a trade of a missing event for a mis-attributed one in a narrow configuration. The re-pin added for S3 has the same limit: if an INIT_TEST ever minted a new instance inside the LOG_REPORT await, only the uuid would be restored. In practice that await is microtask-only today, so the reporter path does not hit this.
Suggested Fix
Capture the test's own TestFrameworkInstance in beforeTest, right after INIT_TEST (next to the uuid snapshot). In finishCliTest(), when the tracked instance is not that one, swap it in with TestFramework.setTrackedInstance(ctx, own) around LOG_REPORT/POST → TEST/POST, then restore the previous one. That replaces the uuid-only pin. If that is too invasive for a patch, narrow the fallback: call the finisher only when the tracked instance still carries this test's uuid, and otherwise record the result for the session status alone. Add one service test where a second test's beforeTest runs between the timed-out test and after(), with no reporter.
Confidence: 🟡 The data flow is traceable in wdioMochaTestFramework.resolveInstance / loadTestResult and testHubModule.sendTestFrameworkEvent, but whether TestHubModule loads without the reporter depends on the binary's testhub response, which can't be verified from the SDK alone.
There was a problem hiding this comment.
Valid. This is resolved by the rework in 26a5872 (refined in f51407e), which takes the first approach you suggested: report against the test's own instance.
- No more tracked-instance slot.
finishCliTest(), the uuid pin andearlyTestFinish.tsare gone.WdioMochaTestFrameworknow captures each attempt's ownTestFrameworkInstanceat TEST/PRE, inopenTestAttempt. - Every finish uses that instance. That covers afterTest, the reporter's
fail, and the no-reportersettleTestFinishes(). They all go throughresolveTestAttempt, andtrackTestEventthen usesattempt.instance, notgetTrackedInstance(). - Your Z/A scenario can't happen now.
loadTestResultand the deferredTestRunFinishedboth read A's own instance, so Z's instance and result are never touched.
Test: without a reporter, settle finishes a test mocha already failed, from its runnable in tests/cli/wdioMochaTestFramework.timedOutTest.test.ts. A second test starts after the timed-out one, so it holds the tracked slot at settle time. The finish still closes the timed-out test's own run, with its timeout error. Related: closes a late afterTest against its own test run while the next test holds the slot asserts that the running test keeps its own uuid.
Real sessions: the no-bail run dgttbcxh… shows 2 passed, 1 failed, 0 in progress. The bail-mid runs (okv7vi3w…, hmqkve23…) show 1 passed, 1 failed, 1 skipped, 0 in progress.
…ade (SDK-7843) - With no reporter, a timed-out test whose body finishes late gets an afterTest saying only whether the body threw. If mocha already failed that attempt, claimCliTestFinish now reports mocha's failure and afterTest stands down, instead of reporting it passed. - A finish started on failure runs after mocha moved on, so the suite's shared ctx.test points at a hook (which inherits the suite's retries) or the next test. Pass the runnable captured in beforeTest to hasRetryPending and the bail cascade, so the cascade is not skipped. - Hand the slot back after TEST/POST also when the first uuid pin displaced another test. - Wait for in-flight finishes in the uuid test instead of a fixed sleep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
With no reporter, a timed-out test whose body finishes late gets an afterTest saying only whether the body threw. If mocha already failed that attempt, claimCliTestFinish now reports mocha's failure and afterTest stands down, instead of reporting it passed. Same change as #272. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
RUN_TESTS |
|
[SDK Wdio Test] TRA build state: passed | Stability 100% — verdict: success. Passed: 52, Failed: 0, Aggregate: 52. TRA: https://observability.browserstack.com/builds/1hf56s9jthrtrmtvgwhv9vck6rfkke7qgmxidpmv |
…K-7843) Review: keep service.ts / reporter.ts to feeding the existing CLI events and leave product logic to the CLI layer, instead of a side-channel util. - reporter onTestFail sends LOG_REPORT/POST + TEST/POST through framework.trackEvent (as skipReporter does), with mocha's result. TestHubModule and AutomateModule already handle a failed TEST/POST; the only problem was that it arrived after after(). - WdioMochaTestFramework tracks each mocha test attempt from TEST/PRE: the finish is reported once, against the instance the attempt started on (replaces the service's _cliTestUuids pin), keeps mocha's failure when a late afterTest says passed, and runs the bail cascade (moved from service.ts, using the runnable captured at TEST/PRE). - settleTestFinishes() (TestFramework, no-op by default) waits for finishes wdio does not await and finishes tests mocha already failed when no reporter is registered; after() calls it before the flush and EXECUTE/POST. - beforeTest passes mocha's bail through its TEST/PRE call. - cli/earlyTestFinish.ts removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…K-7843) v8 port of the #272 review rework: service.ts / reporter.ts only feed the existing CLI events; product logic stays in the CLI layer. - reporter onTestFail sends LOG_REPORT/POST + TEST/POST through framework.trackEvent with mocha's result; AutomateModule already marks the session from a failed TEST/POST, it just arrived after after(). - WdioMochaTestFramework tracks each mocha test attempt from TEST/PRE and reports its finish once, against the instance it started on, keeping mocha's failure when a late afterTest says passed. - settleTestFinishes() (TestFramework, no-op by default); after() calls it before EXECUTE/POST. - cli/earlyTestFinish.ts removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
Automated SDK PR Review
Verdict: finishedAttempts is never cleared, so a second test with the same immediate-parent and title in a worker has all of its finishes dropped.
Summary: 1 critical · 0 warnings · 0 suggestions. Re-review at 26a58726 (route the timed-out test finish through trackEvent).
The refactor addresses the earlier findings on this PR:
- finishes now run on the attempt's own instance (
wdioMochaTestFramework.ts:146) instead of whichever instance is tracked; - attempts are keyed per retry (
attemptKey,:91-95); - the no-reporter path is settled in
after()before the flush.
The one new problem comes from the de-dupe set this refactor introduces. Neither base nor the previous head has it.
The v8 port #273 (ede6baaa) carries the same finishedAttempts set, so the same fix applies there. #264, #265 and #267 are still open; per the earlier reply they are superseded by this PR.
See the inline comment for the full Problem and Suggested Fix.
Generated by Automated SDK PR review.
| const key = WdioMochaTestFramework.attemptKey(args.test as Frameworks.Test) | ||
| const source = args.fromMochaFail ? 'fail' : 'afterTest' | ||
| const attempt = this.openAttempts.get(key) | ||
| if (this.finishedAttempts.has(key) || (attempt?.finishingFrom && attempt.finishingFrom !== source)) { |
There was a problem hiding this comment.
🔴 Critical — [CORRECTNESS] finishedAttempts is never cleared, so a same-named test later in the worker loses every finish
Problem
Every LOG_REPORT/POST and TEST/POST of a mocha test is de-duplicated through resolveTestAttempt. The key is WdioMochaTestFramework.attemptKey(test) (:91-95), which is getUniqueIdentifier(test, 'mocha') plus (retry n). For mocha, getUniqueIdentifier is ${test.parent} - ${test.title} (util.ts:1109-1119): the immediate parent's title, not the full path and not the file.
When TEST/POST is processed, the key is added to finishedAttempts (:315). Nothing ever removes it: openTestAttempt (:273-283) only sets openAttempts. So once a test has finished, any later test in the same worker that produces the same key is caught by the first clause here:
if (this.finishedAttempts.has(key) || …) { … return null }trackEvent then returns without running hooks (:140-143). Here is a concrete spec:
describe('login', () => { describe('valid', () => { it('works', …) }) })
describe('signup', () => { describe('valid', () => { it('works', …) }) })Both tests key to valid - works. The second test's TEST/PRE still opens an attempt, but its afterTest finish, the reporter's onTestFail finish and the settleTestFinishes() finish are all dropped.
As a result:
- the test never gets its TestRunFinished, so it stays In Progress on the dashboard until the idle reap;
- AutomateModule never records its result, so a failure in that test is missing from the session verdict.
Neither base nor 092e69b5 has this set, so it is a regression introduced by this commit.
Suggested Fix
Key the attempt on something unique within the worker, rather than only clearing the set. Clearing finishedAttempts on TEST/PRE alone would let a late fail for the first valid - works land on the second one.
Both callers already pass fullTitle and file:
- the hook path passes the wdio test object;
- the reporter path builds
{ title, parent, fullTitle, file, _currentRetry }atreporter.ts:172.
So key on those:
static attemptKey(test: Frameworks.Test): string {
const retry = (test as { _currentRetry?: number })._currentRetry
const identifier = test.fullTitle
? `${test.file ?? ''}::${test.fullTitle}`
: getUniqueIdentifier(test, 'mocha')
return retry ? `${identifier} (retry ${retry})` : identifier
}The afterTest↔reporter hand-off only works if both sides produce the identical string. Please confirm that the hook-side test.fullTitle and the reporter's TestStats.fullTitle agree for nested describes, and pin it with a test that drives both paths for one test.
Also add a test with two same-named tests under different outer describes, asserting both get a TEST/POST. Keep finishedAttempts.delete(key) in openTestAttempt as a backstop if you like, but it is not the fix on its own.
Confidence: 🟢 Objectively traced:
- key construction at
:91-95→util.ts:1119; - add at
:315, with no delete anywhere in the file; - drop at
:298→return null→trackEventreturns at:141; - base
d5eb5edehas nofinishedAttempts.
There was a problem hiding this comment.
Valid. Fixed in f51407e (and in #273, 359575a), but not with the suggested key.
Why not file::fullTitle: the two sides can't produce the same string.
- wdio's afterTest/beforeTest get
{ ...context.test, parent: context.test?.parent?.title }(@wdio/mocha-frameworkhookArgsFn). The spread copies only the runnable's own properties, and mocha'sfullTitle()is a prototype method, sotest.fullTitleisundefinedthere. - The reporter's
TestStatshasfullTitle(A.B.title), but its constructor sets nofile.
So the late afterTest and the reporter's fail would key differently, and the de-duplication this PR exists for would break.
There was also a second problem behind this one. Two same-key attempts can be open at once: a timed-out test's afterTest can arrive after the next same-named test's TEST/PRE. The by-key openAttempts entry then gets overwritten, so clearing the set alone wouldn't fix it.
The fix:
- Finished is tracked per attempt (
attempt.finished). There's no global key set any more. - afterTest resolves by the test's body.
fnis an own property, so it's on both the beforeTest copy and wdio's afterTest identity snapshot. With_currentRetryit identifies the attempt exactly, even when keys collide. - The reporter's
failhas only the title and parent. It resolves to the latest attempt with that key. That resolution now happens before any await intrackTestEvent, and mocha only starts the next test viasetImmediate, so this is always the test mocha just failed. - The resolved attempt is pinned to the event's
testobject (aWeakMap). That source's TEST/POST reuses it, so it can't land on a later same-named test.
Tests (in wdioMochaTestFramework.timedOutTest.test.ts, on both branches):
- Your case:
login > valid > worksandsignup > valid > worksboth get a TEST/POST. - A same-named test starts while the first test's afterTest is still pending. The first test's late afterTest arrives, then the second test fails. Both close against their own test runs, with the right verdicts.
- The reporter's TEST/POST stays on its test when a same-named test starts between its LOG_REPORT and TEST/POST.
Mutation checks: each test fails with its piece removed. Restoring the never-cleared key set fails 1 and 2, a key-only lookup fails 2, and dropping the pin fails 3.
Suites:
- main: same 78 failures as
mainbefore this change, 1327 passed.tscandeslintare clean. - v8: 1136/1136.
Real sessions with your spec, where the second works fails:
| Before | After | |
|---|---|---|
| WDIO 9 session | 27d948c4 passed |
43dde01e failed (its assertion) |
| WDIO 9 Test Reporting | 1 passed, 1 in progress | 1 passed, 1 failed |
| WDIO 8 session | 7e1d8f0e passed |
e2eb13f3 failed |
| WDIO 8 Test Reporting | 1 passed, 1 in progress | 1 passed, 1 failed |
The timeout+bail repro still behaves as before on both:
- WDIO 9: session
122b36d4failed; Test Reporting 1 passed, 1 failed, 1 skipped. - WDIO 8: session
c2241847failed; Test Reporting 1 passed, 1 failed.
Neither shows a resolveInstance error.
…name (SDK-7843)
A finished attempt's key went into `finishedAttempts` and was never cleared.
The key is `${parent} - ${title}` with only the immediate parent, so a later
test with the same key in a worker (e.g. login > valid > works and
signup > valid > works) had every finish dropped: it stayed In Progress in
Test Reporting and its failure never reached the session status. Two
same-key attempts open at once (a timed-out test's late afterTest) also
overwrote each other's entry.
Track finished per attempt. Resolve wdio's afterTest by the test's body
(`fn`, on both the beforeTest and afterTest copies of the mocha test) plus
retry, and the reporter's `fail`, which has only title and parent, by the
latest attempt with that key, resolved before any await so it is the test
mocha just failed. Pin the resolved attempt to the event's `test` object so
the same source's TEST/POST cannot land on a later same-named test.
`file::fullTitle` (review suggestion) cannot key both sides: wdio's hook
copy `{ ...context.test }` drops mocha's prototype `fullTitle()`, and the
reporter's TestStats has no `file`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
Automated SDK PR Review
Verdict: fail reports a failure, the bail cascade now runs inside a finish that wdio does not await. That breaks the "immediate is only for awaited callers" invariant that skipReporter.ts relies on.
Summary: 0 critical · 1 warning · 0 suggestions. Re-review at f51407ea.
Earlier findings: all resolved at this head.
- The never-cleared attempt-key set from the previous review is fixed:
attempt.finishedis now per attempt, afterTest resolves throughfn+_currentRetry, and the reporter's TEST/POST is pinned. The three new tests intimedOutTest.test.tscover it. - Your reason for not using the suggested
file::fullTitlekey holds. In webdriverio'shookArgsFn, the{...context.test}spread drops the prototype methodfullTitle(), andTestStatssets nofile.
Still open from before: #264, #265 and #267 are open. Close them before merging, as planned.
Edge case worth a thought (not a finding): a test body shared across a loop, it(name, sharedFn) with the same immediate parent and title, would share an attemptsByFn entry.
See the inline comment for the full Problem and Suggested Fix.
Generated by Automated SDK PR review.
| args.instance = instance | ||
| await this.runHooks(instance, testFrameworkState, hookState, args) | ||
| if (attempt && testFrameworkState === TestFrameworkState.TEST) { | ||
| await this.reportBailSkippedTests(attempt, args.result as Frameworks.TestResult) |
There was a problem hiding this comment.
⚠️ Warning — [CONCURRENCY] The bail cascade now runs inside the un-awaited fail finish, so its immediate skip reports can interleave with mocha's after-hooks
Problem
A finish that comes from the reporter's fail is detached on purpose. trackEvent adds it to pendingFinishes and returns (:133-141), and wdio never awaits it. That finish now ends with the bail cascade:
await this.runHooks(instance, testFrameworkState, hookState, args)
if (attempt && testFrameworkState === TestFrameworkState.TEST) {
await this.reportBailSkippedTests(attempt, args.result as Frameworks.TestResult)
}reportBailSkippedTests (:270-290) does not check the source; the comment at :267-268 says it runs on the fail path deliberately. It calls reportSuiteSkipped, which reports each dropped test with { immediate: true }.
skipReporter.ts justifies immediate with one invariant, at :130-135 and again at :160-163: every caller is awaited by wdio, "so nothing else can claim the tracked slot underneath them". That no longer holds for this path. Each immediate skip report starts with INIT_TEST/PRE, which repoints the tracked instance. mocha fires fail when the test fails, then still runs that test's afterEach and the suite's after hooks under bail. Those hook events (and logs sent during them) resolve through the tracked instance, so they can attach to a bail-skipped test's run instead of the failed test's.
The session verdict is not affected, because settleTestFinishes() awaits pendingFinishes before the session is marked. The exposure is misattributed hook runs and logs on the dashboard, in exactly the timed-out-test + bail flow this PR targets.
Suggested Fix
This is offered as an approach rather than a patch. The ordering depends on mocha's runtime, and I haven't run it.
- Option 1 (preferred): on the
failpath, don't cascade inside the detached finish. Record that the attempt owes a bail cascade, and run it fromsettleTestFinishes(), whichafter()awaits before the flush and EXECUTE/POST. Bail has already aborted the spec, so moving these skips to end of spec loses nothing. The afterTest path can keep cascading inline, because wdio awaits it. - Option 2: queue the fail-path cascade through the deferred (non-
immediate) branch ofreportSkippedTest. This needs an option passed throughreportSuiteSkipped, including its recursive call atskipReporter.ts:185.
Either way, update the invariant comments in skipReporter.ts (:130-135, :160-163), which still say every cascade caller is awaited. Add a test where the reporter's fail arrives with bail set and an afterEach hook event follows. Assert that the hook event lands on the failed test's instance, not on a skipped one.
Confidence: 🟢 for the structure: the detached finish at :133-141 → the cascade at :231-232 → reportSuiteSkipped(..., immediate), with no source gate at :270-290, against the invariant stated at skipReporter.ts:130-135. 🟡 for the runtime interleave: it depends on mocha running after-hooks after fail under bail, which I checked at mocha 10.8.2 because the lockfile does not pin mocha.
There was a problem hiding this comment.
Valid, thanks. Fixed in 2d7bba7 with Option 1.
- When the reporter's
failreports the failure, the finish no longer runs the bail cascade. It records the cascade inowedBailCascades, andsettleTestFinishes()runs it oncependingFinisheshas drained.after()awaits that before the flush and EXECUTE/POST, so the skips still land before the session status is marked. The afterTest path still cascades inline, because wdio awaits it. - The invariant comments in
skipReporter.ts(reportSkippedTestandreportSuiteSkipped) now nameafter()as the awaited caller for this case. - New test in
wdioMochaTestFramework.bailCascade.test.ts:failwithbailset, then anafterEachPRE. It asserts the tracked instance is still the failed test's, that nothing is skipped yet, and that A3 and B1 are reported aftersettleTestFinishes(). It fails with the cascade put back inline. The existing fail-path cascade test now callssettleTestFinishes()too. - Real run (timed-out test with
afterEach, bail, a sibling describe): the timed-out test's AFTER_EACH PRE/POST stayed on its own test run, and both skips were sent after the hook. Test Reportingyqinf1n3: failed, P1 F1 S2. Automate buildcf220be2: failed. - Suites: same 78 pre-existing failures, 1328 passed. tsc and eslint are clean.
v8 (#273) needs no change, because its CLI framework has no bail cascade.
On the shared-body edge case: two tests would need the same parent, the same title and the same fn, and the first would have to finish late while the second runs. I've left it as is.
…ttle (SDK-7843) wdio does not await the reporter's `fail`, and mocha still runs the failed test's after-hooks after it. Running the bail cascade inside that finish let its immediate skip reports claim the tracked slot while those hooks ran, so their events could attach to a skipped test. Record the cascade on that path and run it from settleTestFinishes(), which after() awaits before the flush and EXECUTE/POST. The afterTest path still cascades inline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
SDK PR Review — integrity violation
The review gate could not certify this run on 2d7bba79299d, so no review decision was submitted. This is not an approval and not a verdict on the code — the review must be re-run.
Gate reasons:
- G6 — 5 kb proposal(s) but no proposal PR recorded (kb-pr.txt) — run pr-kb-proposal.sh
— SDK PR Review Agent
|
🔴 Blocking findings — fix required Findings: 0 critical · 1 warning(s) · 2 suggestion(s) · 0 needing human review — 2 posted in the findings review; 1 already on this PR from an earlier round, not repeated.
Change map (generated deterministically from the diff)graph LR
subgraph nwdio_service["wdio-service"]
npackages_browserstack_service_tests_cli_wdioMochaTestFramework_test_ts["⚠ wdioMochaTestFramework.test.ts<br/>~629 lines"]
npackages_browserstack_service_src_cli_frameworks_wdioMochaTestFramework_ts["⚠ wdioMochaTestFramework.ts<br/>~307 lines"]
npackages_browserstack_service_tests_service_test_ts["⚠ service.test.ts<br/>~298 lines"]
npackages_browserstack_service_src_service_ts["⚠ service.ts<br/>~114 lines"]
npackages_browserstack_service_tests_reporter_test_ts["⚠ reporter.test.ts<br/>~104 lines"]
npackages_browserstack_service_tests_service_afterSkipOrdering_test_ts["⚠ service.afterSkipOrdering.test.ts<br/>~103 lines"]
npackages_browserstack_service_tests_cli_wdioMochaTestFramework_filePath_test_ts["⚠ wdioMochaTestFramework.filePath.test.ts<br/>~78 lines"]
npackages_browserstack_service_tests_cli_wdioMochaTestFramework_logKind_test_ts["⚠ wdioMochaTestFramework.logKind.test.ts<br/>~69 lines"]
npackages_browserstack_service_src_reporter_ts["⚠ reporter.ts<br/>~40 lines"]
npackages_browserstack_service_src_cli_skipReporter_ts["⚠ skipReporter.ts<br/>~23 lines"]
npackages_browserstack_service_src_cli_frameworks_testFramework_ts["⚠ testFramework.ts<br/>~17 lines"]
n_changeset_pr_272_md["⚠ pr-272.md<br/>~6 lines"]
end
↻ This verdict comment is the review anchor — it's updated in place on each run (the gate posts its status separately). — SDK PR Review Agent |
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
…7843) mocha retries a failed attempt by cloning it and emitting `retry`, not `fail`, so the attempt's runnable never gets state `failed`. A timed-out attempt that was retried was then neither finished by settleTestFinishes() nor kept failed when its late afterTest said passed. A later attempt of the same test having started means mocha failed this one; check that too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
Automated SDK PR Review — findings at bff8fc4b
Verdict: 🔴 Fix 2 issues (engine verdict). The verdict comment above shows
Summary: 0 critical · 2 warnings · 0 suggestions. This was an incremental re-review: only wdioMochaTestFramework.ts, timedOutTest.test.ts and service.timedOutTestFinish.test.ts changed since the last review and were reviewed. The other files were not re-reviewed in this run.
Both findings are inline below. I checked each one against the code at bff8fc4b.
Generated by Automated SDK PR review.
| if (body) { | ||
| const byRetry = this.attemptsByFn.get(body) ?? new Map<number, TestAttempt>() | ||
| byRetry.set((test as { _currentRetry?: number })._currentRetry ?? 0, attempt) | ||
| this.attemptsByFn.set(body, byRetry) |
There was a problem hiding this comment.
⚠️ Warning — [CORRECTNESS] attemptsByFn is keyed only on the test body, so tests that share one body overwrite each other's attempt
Problem
openTestAttempt stores each attempt under attemptsByFn.get(body).set(_currentRetry, attempt) (:315-319). The body is the test function object, and mocha lets several tests share one, e.g. for (const c of cases) it(c.name, sharedFn). Each it then overwrites the 0 entry of the same map.
Two things then go wrong:
- A timed-out test's late
afterTestresolves throughfindTestAttempt→attemptsByFnto the next test's attempt. It reports onto that test's run, sets itsfinishingFrom, and the real finish of the next test is then dropped as already reported. mochaFailed(:329-337) reads the same map, so a later same-body test at retry 0 doesn't count as a retry, and the key collision can't be told apart from a retry.
This is narrow (shared function objects plus a timeout), but it is the same per-test identity problem this PR fixed for colliding titles.
Suggested Fix
Approach, not verified: key on the attempt object rather than the shared function.
- Store the attempt on the runnable that mocha gives each test (for example a
WeakMap<MochaRunnable, TestAttempt>keyed bytest.ctx.test, captured at TEST/PRE). - Fall back to
fnonly when there is no runnable.
Confirm that wdio's afterTest test object still reaches the same runnable (ctx.test). Add a test with two its sharing one function, where the first times out.
Confidence: 🟢 the keying at :315-319 and the lookup at :333-336 are in the code. The trigger (shared test functions) is uncommon.
There was a problem hiding this comment.
Valid. Fixed in c8e8404 (main) and 683b532 (#273, v8), with a different key from the one suggested.
Why not key on the runnable (test.ctx.test): the test that wdio hands to afterTest has no reference to the runnable. testFrameworkFnWrapper (@wdio/utils) builds it as { ...context.test } via hookArgsFn, taken as identitySnapshot before the body runs. ctx is mocha's Context, which the whole suite shares. By the time a timed-out test's afterTest runs, mocha has moved on, and ctx.test points at the hook or the next test. So a runnable read at afterTest isn't this test's.
What changed: attemptsByFn is now keyed by body, then by attemptKey (parent title + title + retry), not by body + retry number.
it(c.name, sharedFn)loops get one entry per test, so a late afterTest finds its own attempt.mochaFailednow counts only a later retry of the same test (same identifier, higher_currentRetry). A different test that shares the body no longer looks like a retry.- A collision would now need the same body, the same parent title and the same title, which is two indistinguishable tests.
Test: closes a late afterTest by its own test when tests share one body in wdioMochaTestFramework.timedOutTest.test.ts (both branches). The first of two its sharing one function times out, and its late afterTest says passed. It asserts each finish lands on its own test run, with the timed-out one failed. It fails with the old body + retry key.
| // after-hooks: the cascade's skip reports would claim the tracked slot under them | ||
| this.owedBailCascades.push({ attempt, result: args.result as Frameworks.TestResult }) | ||
| } else { | ||
| await this.reportBailSkippedTests(attempt, args.result as Frameworks.TestResult) |
There was a problem hiding this comment.
⚠️ Warning — [CONCURRENCY] The afterTest path runs the bail cascade inline; for a timed-out test with no reporter this happens after mocha has moved on
Problem
trackTestEvent defers the cascade on the reporter's fail path (:234-237) because mocha still runs the failed test's after-hooks. The afterTest path still cascades inline (:239).
That is safe while wdio is awaiting afterTest inside the test. For a timed-out test with no reporter registered (Test Reporting, Accessibility and Percy all off), there is no fail path:
- The late afterTest claims the finish (
:384keeps mocha's failure). - It then runs the cascade inline after mocha has already timed the test out and, under bail, gone on to its after-hooks.
- Each skip report's INIT_TEST then repoints the tracked instance under those hooks.
That is the interleave the fromMochaFail branch was changed to avoid.
Suggested Fix
Approach, not verified: when this.mochaFailed(attempt) is true (mocha already failed or timed out the attempt), push to owedBailCascades on the afterTest path too, and let settleTestFinishes() run it. Cascade inline only for a test that afterTest finished normally.
Confidence: 🟡 the code paths are as described. Whether hook events land on a skipped test depends on mocha's runtime ordering, which hasn't been run.
There was a problem hiding this comment.
Valid. Fixed in c8e8404 as suggested. #273 (v8) has no bail cascade, so it needs no change.
- In
trackTestEvent, the cascade is now owed tosettleTestFinishes()when it comes from the reporter'sfailor whenthis.mochaFailed(attempt)holds at afterTest. In the second case mocha has already failed or timed out the attempt and moved on to its after-hooks. A normal failure still cascades inline: its afterTest runs inside the test, before mocha marks the runnable, sostateis still unset there. settleTestFinishes()now drainsowedBailCascadeswith awhile/shiftloop and then setssettled. A late afterTest that only arrives after settle has run still cascades inline. By then mocha has finished, so that cascade isn't dropped.- I updated the
reportBailSkippedTestsdoc and both invariant comments inskipReporter.ts.
Test: runs the cascade from settle for a timed-out test's late afterTest, with no reporter in wdioMochaTestFramework.bailCascade.test.ts. Under bail, the runnable is marked failed/timed out, then afterTest LOG_REPORT/TEST POST arrives with no fromMochaFail, then AFTER_EACH PRE. It asserts nothing is skipped yet and the tracked instance is still the failed test's. After settleTestFinishes() it asserts A3 and B1 are reported. It fails with the cascade inline.
Runs:
- main: same 78 pre-existing failures, 1332 passed (+2). tsc and eslint are clean.
- v8: all pass. tsc is clean.
- Real session (timeout +
afterEach+ sibling describe, bail, 10 s): Test Reporting6wnjojvhis failed, P1 F1 S2, nothing in progress. Automate buildae3bc833is failed.
…l cascade (SDK-7843) - Tests can share one body (`it(name, sharedFn)`), so attempts keyed by the body and retry overwrote each other: a timed-out test's late afterTest resolved to the next test's attempt. Key them by body, then attemptKey. - A timed-out test's afterTest arrives after mocha has moved on to its after-hooks, like the reporter's `fail`. When mocha already failed the attempt, owe its bail cascade to settleTestFinishes() too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
1 similar comment
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
| * @param {*} args | ||
| */ | ||
| async trackEvent(testFrameworkState: State, hookState: State, args: Record<string, unknown> = {}) { | ||
| if (args.fromMochaFail) { |
There was a problem hiding this comment.
Suggestion: reuse the skip-report chain instead of adding pendingFinishes
This PR adds a second way of tracking reporter work that wdio doesn't await. skipReporter.ts already has one for exactly this case (SDK-7493): reportChain, which after() awaits through drainSkipReports(). The PR's version is:
pendingFinishes(wdioMochaTestFramework.ts:103);- the
fromMochaFailbranch intrackEvent(:142-151); - the wait loop in
settleTestFinishes()(:419-421); - a second wait in
after()(service.ts:682).
Why one chain is better
The two aren't ordered against each other. drainSkipReports() (service.ts:674) only waits for reportChain. So a queued onTestSkip report can run its INIT_TEST/TEST/PRE while the timed-out test's TEST/POST is still in flight. That puts two tests on the shared tracked-instance slot at once, which is what SDK-7493 removed. The fail goes to the attempt's own instance, so it may well be safe, but with one chain the question doesn't come up.
Suggested shape
skipReporter.ts: export a general way to queue work onreportChain, as theimmediatepath already does.reporter.tsonTestFail: queue its pair on the chain:await enqueueReport(async () => { await framework.trackEvent(TestFrameworkState.LOG_REPORT, HookState.POST, { test, result, fromMochaFail: true }) await framework.trackEvent(TestFrameworkState.TEST, HookState.POST, { test, result, fromMochaFail: true }) })
WdioMochaTestFramework.trackEvent: drop the branch,pendingFinishesand its wait loop.trackEventbecomesawait this.trackTestEvent(...).fromMochaFailstays only as the source flag forresolveTestAttempt.settleTestFinishes(): keep it for the no-reporter fallback and the owed bail cascades. It still runs afterdrainSkipReports(), so the order holds: the fail, then its cascade, then the flush andEXECUTE/POST.
One thing to keep: finding the attempt synchronously
Today resolveTestAttempt runs before mocha's setImmediate starts the next test. Once queued, the call starts in a promise callback instead. That still runs before setImmediate when the chain is idle, and I don't see anything that would keep it busy when mocha emits fail.
If you'd rather not rely on that timing, add a small synchronous step that onTestFail calls before queueing. For example, pinFailedAttempt(test) would do the findTestAttempt and resolvedFinishes.set part straight away.
Not suggesting
- Reusing the deferred
queuedSkipspath: it would hold a timed-out test's finish until the worker ends, even when no bail is set and later tests keep running. - Reusing
reportSkippedTest(): it skips any test already marked as started, and it hard-codes a skipped result.
There was a problem hiding this comment.
Agreed, thanks. Done in abee9a0 (main) and 788bc81 (#273, v8), in the shape you suggested, including the synchronous pin.
skipReporter.ts: newenqueueReport(work, what)is now the one way ontoreportChain. Theimmediateskip path uses it too.reporter.tsonTestFail: callsframework.pinFailedAttempt(test)synchronously in mocha'sfail. It then queues the LOG_REPORT/POST + TEST/POST pair withenqueueReport.pinFailedAttempt: a no-op on the baseTestFramework. InWdioMochaTestFrameworkit does thefindTestAttempt+resolvedFinishes.set, so the attempt is fixed before mocha'ssetImmediatestarts the next test, whatever is already on the chain.WdioMochaTestFramework.trackEvent: thefromMochaFailbranch,pendingFinishesand its wait loop are gone, andtrackTestEventis folded back intotrackEvent.fromMochaFailonly remains as the source flag forresolveTestAttempt.settleTestFinishes(): keeps the no-reporter fallback and the owed bail cascades. It runs afterdrainSkipReports(), so the order is the fail, then queued skips, then cascades, then the flush and EXECUTE/POST.- v8:
after()never awaitedreportChain. I addeddrainSkipReports()there (it returns the chain), andafter()now awaits it before settling.
Tests (reporter.onTestFail.test.ts):
- The pin happens before the queued LOG_REPORT/TEST/POST, and both carry the pinned test object.
- A timed-out test's
failfollowed by anonTestSkipreport: the skip's INIT_TEST/PRE starts only after the fail's TEST/POST has ended. - Both fail against the old direct
trackEventcalls. The timed-out-test helpers now pin and queue the same way, thendrainSkipReports()+settleTestFinishes(), asafter()does.
Runs:
- main: same 78 pre-existing failures, 1334 passed. tsc and eslint are clean.
- v8: 1141/1141, tsc clean.
- Real sessions, Test Reporting / Automate:
| Branch | Spec | Test Reporting | Automate |
|---|---|---|---|
| main | timeout + afterEach + bail |
1hf6bwqb failed, P1 F1 S2 |
20aaec01 failed |
| main | duplicate names | ucoe63jo failed, P1 F1 |
d57508c2 failed |
| v8 | timeout + bail | peme68m9 failed, P1 F1 |
b83b9be0 failed |
| v8 | duplicate names | tuxmsptv failed, P1 F1 |
48e2ee3e failed |
…port chain (SDK-7843) Reuse skipReporter's reportChain, which after() already drains, instead of a separate pendingFinishes set. The reporter pins the failed test to its attempt synchronously in mocha's `fail` (pinFailedAttempt), then queues LOG_REPORT and TEST/POST through enqueueReport. A queued skip report can no longer run while a timed-out test's finish is in flight. settleTestFinishes() keeps the no-reporter fallback and the owed bail cascades, and runs after drainSkipReports(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
…SDK-7843) Move the wdioMochaTestFramework, reporter and service tests this branch added (and the files they sat beside) into tests/cli/wdioMochaTestFramework.test.ts, tests/reporter.test.ts and tests/service.test.ts. No test is added or removed. In service.test.ts the moved after() cases use the real after() captured before the `after` suite replaces it, real timers for the cases that time a finish, and restore only their own spies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
|
RUN_TESTS |
|
[SDK Wdio Test] TRA build state: passed | Stability 100% — verdict: success. Passed: 52, Failed: 0, Aggregate: 52. TRA: https://observability.browserstack.com/builds/3vlacy4ppxmljwy4gyphhp937aa2sycp6xwxfgtd |
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
SDK PR Review — findings at 182e6edf
Verdict: 🔴 Fix 1 blocking issue
0 critical · 1 warning(s) · 2 suggestion(s) · 0 needing human review
2 finding(s) inline; 1 already posted on this PR, not repeated. The signed verdict comment carries the merge-gate state.
Prior findings
RESOLVED 11 · UNRESOLVED 1 · NOT-A-FINDING 1
- Suggested fix rejected by the author on finishedAttempts is never cleared, so a same-named test later in the worker loses every finish: "wdio's afterTest/beforeTest get { ...context.test } ... mocha's fullTitle() is a prototype method, so test.fullTitle is undefined there. The reporter's TestStats has fullTitle but its constructor sets no file."
- Suggested fix rejected by the author on attemptsByFn is keyed only on the test body, so tests that share one body overwrite each other's att: "the test that wdio hands to afterTest has no reference to the runnable ... ctx is mocha's Context, which the whole suite shares. By the time a timed-out test's afterTest runs, mocha has moved on, and ctx.test points at the hook or the next test."
— SDK PR Review Agent
| const identifier = getUniqueIdentifier(attempt.test, 'mocha') | ||
| const retry = WdioMochaTestFramework.currentRetry(attempt.test) | ||
| return [...(body && this.attemptsByFn.get(body)?.values() || [])].some((other) => | ||
| getUniqueIdentifier(other.test, 'mocha') === identifier && WdioMochaTestFramework.currentRetry(other.test) > retry) |
There was a problem hiding this comment.
⚠️ Warning — [CORRECTNESS] A retried timed-out attempt's inferred failure can overwrite its passing retry in the session status
Problem
mochaFailed() treats an attempt as failed when a later attempt of the same test has started (wdioMochaTestFramework.ts:342). For a mocha retry that is not mocha's verdict. mocha retries a failed or timed-out attempt without failing it: it clones the test, emits retry and never calls Runner#fail (mochajs/mocha@v10.8.2 lib/runner.js:813-823). So the reporter's onTestFail never fires, and mocha's failure count, which the classic flow uses via after(result), stays 0 when the retry passes.
Two new paths then report the first attempt as failed after the retry has already reported its pass:
settleTestFinishes()(:406-412): the first attempt's body never returned, so it is still inopenAttemptswith nofinishingFrom, andafter()reports it failed.resolveTestAttempt()(:390-392): the first attempt's body returned late withpassed, after the retry finished, and its result is rewritten to a failure.
AutomateModule.onAfterTest keys a mocha result by the session-name string (automateModule.ts:251-254, resultKey = test.fullName ? … : name; wdio's mocha test has no fullName). Both attempts share that key, so the later write wins and onAfterExecute marks the session failed for a test that passed on retry. At base neither write happened: the hung attempt sent no TEST/POST, and a late passed afterTest was not rewritten.
The test without a reporter, settle finishes a retried attempt whose body never returned (tests/cli/wdioMochaTestFramework.test.ts:532-543) asserts exactly this order: flaky <uuid1> passed=true, then flaky <uuid0> passed=false. Reporting the first attempt's own test run as failed in Test Reporting is reasonable. The problem is that the same TEST/POST also reaches the session tally. The v8 port (#273 at c299adf5) has the same rule (wdioMochaTestFramework.ts:221-229, used at :276 and :294).
Suggested Fix
Approach, not verified: keep closing the earlier attempt's test run as failed, but keep it out of the session verdict when the failure is inferred only from a later attempt having started (runnable.state !== 'failed').
- In
resolveTestAttemptandsettleTestFinishes, mark that case on the event args, for exampleargs.supersededByRetry = true. - In
AutomateModule.onAfterTest, skiptestResults.set(and the instance status write) when that flag is set, so the latest attempt's result stands. - Add a test that drives both attempts through AutomateModule (or asserts the args the module observes) and checks the session ends
passed.
Apply the same change in fix(cli): record a timed-out mocha test's failure when mocha reports it (SDK-7843) [v8] #273.
This fix is unverified against AutomateModule.onAfterTest and the binary's handling of a flagged TEST/POST; confirm it before applying.
Impact / likelihood: high — the Automate / App Automate session is marked failed although mocha, and the classic flow's after(result), count the test as passed · low — needs mocha retries, a first attempt that times out, a passing retry, and the first attempt's body still pending (or resolving after the retry finished)
Confidence: 🟢 — mocha lib/runner.js:813-823 (v10.8.2) retries without failing; wdioMochaTestFramework.ts:342, :391, :408-412 report the retried attempt as failed after its retry; automateModule.ts:251-254 keys mocha results by name, last write wins
— SDK PR Review Agent
| * body and wdio's afterTest are still pending, and moves on. These drive the real framework in | ||
| * that order and record what the product modules (TestHubModule, AutomateModule) would observe. | ||
| */ | ||
| describe('WdioMochaTestFramework — a test finish is reported once, against its own test run', () => { |
There was a problem hiding this comment.
💡 Suggestion — [MAINTAINABILITY] The PR body's test inventory names test files and counts that are not in the PR
Problem
The PR body lists tests/cli/wdioMochaTestFramework.timedOutTest.test.ts (7), tests/cli/wdioMochaTestFramework.bailCascade.test.ts (9), tests/reporter.onTestFail.test.ts (4), tests/service.timedOutTestFinish.test.ts (2) and "New test: tests/cli/wdioMochaTestFramework.preTestLog.test.ts (3 cases)". None of these files exists at HEAD; for example pr-fetch-ref returns not found at 182e6edf…: …/wdioMochaTestFramework.timedOutTest.test.ts and the same for …preTestLog.test.ts. The cases were consolidated into this file (13 cases in this describe, 11 in the bail describe, 3 in the pre-test LOG describe), tests/reporter.test.ts (6 cases) and tests/service.test.ts. The filePath and logKind test files were also folded into this file.
Suggested Fix
Update the Tests section of the PR body to name tests/cli/wdioMochaTestFramework.test.ts, tests/reporter.test.ts and tests/service.test.ts, with their current describe blocks and counts.
Impact / likelihood: low — reviewers and the release record point at test files and counts that do not exist · high — every reader of the PR description
Confidence: 🟢 — the PR body's Tests list names five files that are not at HEAD
— SDK PR Review Agent
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
1 similar comment
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
4 participants
What is this about?
On the CLI flow, a mocha test that hits mocha's timeout is reported as if it hadn't failed:
passed.TestRunFinished, so Test Hub reaps the build astimeoutabout 60 minutes later.A customer hit this (SDK-7843, group 2109715, iOS + Android App Automate,
mochawithbail, 300 s / 600 s timeouts). I counted their sessions that ran past the mocha timeout and were then retried, which means the attempt failed:Example: build #753, session
3915be364994ad61431860dd123e258df0c0d666("bottom nav", 352 s). The terminal showsTimeout of 300000ms exceeded, yet the dashboard shows passed. Their own SDK log hasSession status updated … status: 'passed'139 ms before the test's failure reached the SDK.Why it happens
wdio runs a test's
afterTesthook inside the test's runnable, after the body. When the test times out, mocha fails it, and emitsfailto reporters, while the body and itsafterTestare still pending. Withbail, or when it's the worker's last test, wdio then runsafter()before thatafterTest. Measured on a real session with stock 9.39.2:onTestFail(with the timeout error)after(result=1): mocha's own failure count is 1AutomateModule.onAfterExecutemarks the session passed. Its results come only fromafterTest(TEST/POST), and there is none yet; an empty map means passed.afterTestfinally fires, too late for both the session status and theTestRunFinishedflushWhy the classic flow never had this: it sets the session status from
after(result), which is mocha's own count. The CLI flow dropsresult.Why it surfaced now: #261 (4eb12b3) made
updateURLSForGRRtolerate a bin-session config withoutapis. Up to 9.39.2 that config crashed the CLI bootstrap for these App Automate accounts, so they silently ran the classic flow. Since 9.39.3 the CLI boots and they hit this path.The fix: send the failure through the existing CLI events when mocha reports it
service.tsandreporter.tsonly feed the existingtrackEventcalls; the product modules are unchanged.TestHubModule(Test Reporting) andAutomateModule(session status) already handle a failedTEST/POSTcorrectly. The only problem was that it arrived afterafter().reporter.tsonTestFail: on the CLI flow with mocha, it sendsLOG_REPORT/POST+TEST/POSTthroughframework.trackEvent, with mocha's own result (passed: false, the timeout error, its duration). This is the same patterncli/skipReporteralready uses from the reporter.WdioMochaTestFramework(CLI layer) tracks each mocha test attempt fromTEST/PRE:failorafterTest); the late duplicate is dropped._cliTestUuidspin.afterTestsayspassedbut mocha already failed the test, the failure is reported.service.ts. It runs as part of the failed finish, so it lands before the flush, and it uses the runnable captured atTEST/PRE.settleTestFinishes()(a no-op on the baseTestFramework) waits for finishes wdio doesn't await. With no reporter registered, it also finishes tests mocha already failed, from mocha's runnable.service.ts:beforeTestaddsbailto its existingTEST/PREcall.after()callssettleTestFinishes()before the deferred-finish flush andEXECUTE/POST._cliTestUuidsand the bail cascade are removed, soservice.tsis 87 lines shorter than onmain.Unchanged:
afterTestbefore mocha emitsfail, so the reporter's events are dropped as already reported.onTestFailreturns immediately.Also in this PR — the
resolveInstanceERROR (same change as #264): console output from wdio'sbeforehook (the customer's[SelfHealer] Installed …) reachestrackEvent(LOG, POST)before mocha's first hook, when no test or hook instance exists.resolveInstancethen printedresolveInstance: unable to resolve/create instance for TestFrameworkState.LOG HookState.POSTon every worker.WdioMochaTestFramework.trackEventnow drops a LOG with no tracked instance at debug level, as the classic flow does. New test:tests/cli/wdioMochaTestFramework.preTestLog.test.ts(3 cases). Verified on real sessions under #264: stock 9.39.2 build2owxolsli9…prints the ERROR, patched buildh5a3ibnc2o…is clean.Review follow-ups:
afterTest, bail cascade from the captured runnable.service.ts/reporter.tsinto the CLI framework. The events go through the existingtrackEventcalls, andcli/earlyTestFinish.tsis gone. Behaviour is the same as above; it was re-verified on real sessions (below).Verification
Real sessions, with
mochaOpts.timeoutexceeded by awaitFor…and Test Reporting on. Every build was read back from/ext/v1/builds/<uuid>/testRuns, and every session from the Automate / App Automate session API:c7bsborr…bailca3982e2…passed ❌passed❌ (timed-out test never closed)jygwt2vd…baild534ff4b…failed, reason = mocha timeoutqyccjc2i…bail815df7b4…failedmjhq2iqi…4f261091…failedissawcvh…bailc7627749…passed ❌wlmwmp17…bail17e6fe50…failed, reason = mocha timeoutRe-verified after the 26a5872 rework (same repro; every build read back from
/ext/v1):rqss9pq5…bailb48b64fb…failed, reason = mocha timeoutdgttbcxh…b86c33b1…failedokv7vi3w…bail4b0af703…failedagg8icwu…bail9fe3304a…failed, reason = mocha timeoutIn every run the late
afterTestlogswas already reported, dropping … from afterTest. No run prints theresolveInstanceERROR.Tests:
tests/cli/wdioMochaTestFramework.timedOutTest.test.ts(7, real framework):fail, lateafterTestdropped;afterTest, thefailafter it dropped;afterTestcloses its own test run while the next test holds the slot;passed;faildropped.tests/cli/wdioMochaTestFramework.bailCascade.test.ts(9): the SDK-7063 cascade cases moved fromservice.test.ts, plus the cascade from the captured runnable when mocha has moved on to a hook.tests/reporter.onTestFail.test.ts(4): the events and identity the reporter sends.tests/service.timedOutTestFinish.test.ts(2):after()settles before the flush andEXECUTE/POST;afterTestno longer touches the tracked slot.service.test.ts:beforeTestpasses mocha'sbail(not wdio's) to the framework.after(), the retry key or the cascade each fails a test.Full suite: 78 failed / 1324 passed with this PR vs 78 / 1305 on
main: the same pre-existing failures, compared by name, mostly network-dependentservice.test.tscases.tsc -p tsconfig.prod.json --noEmitandeslintare clean.Follow-ups:
onAfterExecute-from-afterTestlogic: port in fix(cli): record a timed-out mocha test's failure when mocha reports it (SDK-7843) [v8] #273 (real-session verified on WDIO 8).after()) treated the symptom rather than the cause.Related Jira task/s
SDK-7843
Release (mandatory for every PR — required for the
ready-for-reviewlabel)Version bump: (required — tick exactly one)
Release notes type: (optional)
Release notes (customer-facing): (optional but encouraged)
resolveInstance: unable to resolve/create instance ... LOG POSTerror printed when a wdiobeforehook writes to the console.Release notes (internal): (required — engineer-facing; what actually changed / why)
onTestFail(CLI, mocha) sendsLOG_REPORT/POST+TEST/POSTviaframework.trackEventwith mocha's result.WdioMochaTestFrameworktracks attempts fromTEST/PRE: it reports each finish once, against the attempt's own instance (replacing_cliTestUuids), keeps mocha's failure over a latepassed, and runs the bail cascade (moved fromservice.ts).settleTestFinishes()runs fromafter()before the flush andEXECUTE/POST.TestHubModule/AutomateModuleare unchanged.afterTestinside the test's runnable, so for a mocha timeout underbail(or on a worker's last test)after()runs first.AutomateModule.onAfterExecutethen marks the session from results that onlyafterTestsupplies (empty →passed), and the test'sTestRunFinishedis stranded (build reaped astimeout). The classic flow usedafter(result)and was correct.apisnow boot the CLI instead of silently falling back to the classic flow.WdioMochaTestFramework.trackEventdrops a LOG event with no tracked instance at debug level instead of logging the resolveInstance ERROR.Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.
🤖 Generated with Claude Code