Repository navigation
fix(cli): record a timed-out mocha test's failure when mocha reports it (SDK-7843) - #272
AakashHotchandani wants to merge 9 commits into
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.
| 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
|
See the SDK PR Review Agent's report from your local run. Process integrityThe review gate could NOT certify this run — the verdict is held at
Change map (generated deterministically from the diff)graph LR
subgraph nwdio_service["wdio-service"]
npackages_browserstack_service_src_cli_frameworks_wdioMochaTestFramework_ts["⚠ wdioMochaTestFramework.ts<br/>~286 lines"]
npackages_browserstack_service_tests_cli_wdioMochaTestFramework_timedOutTest_test_ts["wdioMochaTestFramework.timedOutTest.test.ts<br/>~205 lines"]
npackages_browserstack_service_tests_cli_wdioMochaTestFramework_bailCascade_test_ts["wdioMochaTestFramework.bailCascade.test.ts<br/>~191 lines"]
npackages_browserstack_service_tests_service_test_ts["⚠ service.test.ts<br/>~145 lines"]
npackages_browserstack_service_src_service_ts["⚠ service.ts<br/>~113 lines"]
npackages_browserstack_service_tests_service_timedOutTestFinish_test_ts["⚠ service.timedOutTestFinish.test.ts<br/>~81 lines"]
npackages_browserstack_service_tests_reporter_onTestFail_test_ts["⚠ reporter.onTestFail.test.ts<br/>~75 lines"]
npackages_browserstack_service_tests_cli_wdioMochaTestFramework_preTestLog_test_ts["⚠ wdioMochaTestFramework.preTestLog.test.ts<br/>~56 lines"]
npackages_browserstack_service_src_reporter_ts["⚠ reporter.ts<br/>~33 lines"]
npackages_browserstack_service_src_cli_frameworks_testFramework_ts["testFramework.ts<br/>~7 lines"]
n_changeset_pr_272_md["pr-272.md<br/>~6 lines"]
npackages_browserstack_service_src_cli_skipReporter_ts["⚠ skipReporter.ts<br/>~6 lines"]
npackages_browserstack_service_tests_service_afterSkipOrdering_test_ts["⚠ service.afterSkipOrdering.test.ts<br/>~1 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. |
1 participant
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