Skip to content

fix(automate): name App Automate sessions per test instead of only at worker teardown (SDK-7270) #131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anish353 wants to merge 2 commits into
base: main
Choose a base branch
from
+44 −3
Open

fix(automate): name App Automate sessions per test instead of only at worker teardown (SDK-7270) #131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3da6065
fix(automate): name App Automate sessions per test instead of only at…
anish353 Aug 12, 2026
0986e13
chore(changeset): auto-generate from PR template (patch)
github-actions[bot] Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/pr-131.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@wdio/browserstack-service": patch
---

- Fixed App Automate and Automate session names staying on the static `sessionName` capability instead of the test title, for suites that reload the session between tests or whose run ends before the WebdriverIO `after` hook.

@anish353 anish353 Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] The reload clause is contradicted by this PR's own evidence

The note credits the fix to "suites that reload the session between tests or whose run ends before the WebdriverIO after hook." Row 1 of your own evidence table disproves the first clause: 9.33.1 (shipped code), after() reached → 3 renames, correct per-test titles — on a build you describe as the customer's exact reload-per-test shape.

The mechanism agrees: onReload refreshes KEY_FRAMEWORK_SESSION_ID (service.ts:809-812), so each reloaded session already got its own sessionMap entry and the old sweep already PUT a name for every one. Reload alone was never a failure mode. The real cause is solely the second clause — the worker never reaching after() (service.ts:584 is the only EXECUTE/POST trigger; the customer log shows EXECUTE trackEvent = 0).

This matters because it's the customer-facing release note: users who reload per test but do reach after() would read this as fixing something that was never broken for them.

Suggestion: drop the reload clause here and in the internal note. Or substantiate it — it would hold only if the Automate REST API refuses to rename a terminated session, which is worth confirming either way since it's a useful fact.

Reviewer: fallback independent reviewer

42 changes: 39 additions & 3 deletions packages/browserstack-service/src/cli/modules/automateModule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ interface TestResult {

interface SessionData {
lastTestName: string
appliedName?: string // last name successfully PUT for this session, for de-duping
testResults: Map<string, TestResult> // testName -> TestResult
}

Expand Down Expand Up @@ -95,6 +96,42 @@ export default class AutomateModule extends BaseModule {
}

TestFramework.setState(instace, TestFrameworkConstants.KEY_AUTOMATE_SESSION_NAME, name)

// SDK-7270: name the session NOW, while it is still the live session, instead of
// relying solely on the onAfterExecute sweep at worker teardown. Sessions are closed
// as soon as the suite reloads them (`browser.reloadSession()` per test), and a worker
// that never reaches `after()` — interrupted run, hard exit, crash — never fires
// onAfterExecute at all, leaving every session on the creation-time `sessionName`
// capability. Restores the pre-9.27 behaviour, where the rename was issued per test.
await this.flushSessionName(sessionId)

@anish353 anish353 Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Skipped tests also fire TEST/PRE, so a hook failure fans this out into hundreds of blocking PUTs

TEST/PRE isn't fired only by service.ts:498. skipReporter.ts:58 awaits framework.trackEvent(TEST, PRE, …) for every skipped test, and reportSuiteSkipped (:72-92) walks every remaining test and nested suite calling reportSkippedTest when a before all / beforeEach hook fails — all inside the awaited afterHook. Each of those now issues a blocking rename PUT, and because the titles are distinct the appliedName de-dupe collapses none of them.

Failure scenario: a failed before all in a 300-test suite adds ~300 sequential HTTPS PUTs to teardown — roughly a minute at typical latency — inside an awaited WDIO hook. Statically-skipped tests also transiently rename the live session to a test that never ran.

UNVERIFIED: whether this trips the WDIO afterHook timeout — I didn't confirm the configured value. If it's in the tens of seconds, this escalates to High, so worth a check on your side.

Suggestion: pass a marker in reportSkippedTest's TEST/PRE args (e.g. skipped: true) and skip the flush for it; or make the per-test PUT fire-and-forget and let the awaited sweep stay authoritative.

Reviewer: fallback independent reviewer

}

/**
* PUT the session's current name if it has not already been applied.
* De-duped via `appliedName` so the onAfterExecute sweep does not re-send it.
*/
private async flushSessionName(sessionId: string): Promise<void> {
const testContextOptions = this.config.testContextOptions as TestContextOptions
if (testContextOptions.skipSessionName) {

@anish353 anish353 Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Unguarded testContextOptions deref (pre-existing pattern, reproduced here)

testContextOptions is genuinely reachable as undefined: unConfigureModules() (cli/index.ts:427) re-calls configure() with no 4th arg so config defaults to {}; setConfig swallows a parse failure leaving {}; and the binary returns a degenerate config on auth failure. The as TestContextOptions cast hides all of that from the compiler.

No new exposure — :66 already derefs before this line is reachable from onBeforeTest, and the onAfterExecute path derefs inside a try/catch in both old and new code. So this isn't a regression; it just reproduces the pattern where ?. would cost nothing.

Note this guard is also unreachable from onBeforeTest specifically (:66 has already returned by then) — it's live and correct only on the onAfterExecute path.

Reviewer: fallback independent reviewer

return
}

if (!sessionId) {
return
}

const sessionData = this.sessionMap.get(sessionId)
if (!sessionData || !sessionData.lastTestName || sessionData.appliedName === sessionData.lastTestName) {
return
}

const name = sessionData.lastTestName
await this.markSessionName(sessionId, name, {

@anish353 anish353 Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Awaited network round trip now on the per-test hot path, with no timeout

Under Mocha with defaults name embeds the test title (:80-85), so lastTestName changes every test and the de-dupe never suppresses. A conventional suite — one session, N tests, no reload — goes from 1 PUT at teardown to N PUTs, each awaited before the test body runs. _fetch (fetchWrapper.ts:19-35) sets no timeout, no retry, and no abort signal, so a slow or hanging API stalls every test start. At ~150–300 ms that's roughly 1–2.5 min added per worker on a 500-test suite.

This also makes the PR body's "no extra API calls in the steady state" inaccurate for the non-reload case — it holds only when consecutive titles repeat.

Mitigating, and worth saying: this is parity with the legacy non-CLI path — service.ts:503 awaits _setSessionName per test, de-duped via _fullTitle at :1006. So it's restored prior behavior, not a new design.

Suggestion: bound it with AbortSignal.timeout(~5000), or make the onBeforeTest flush fire-and-forget and keep the teardown sweep authoritative.

Reviewer: fallback independent reviewer

user: this.config.userName as string,
key: this.config.accessKey as string
})
sessionData.appliedName = name

@anish353 anish353 Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] This records attempted, not applied — so a failed PUT disarms the final sweep

markSessionName never checks response.ok (:274-276 — await fetch(...) → await response.json() → debug-log, no status check) and swallows everything in its catch (:277-279). A 401/404/429/5xx is indistinguishable from success. This line then sets appliedName unconditionally, so the teardown sweep at :220-221 no-ops. The field comment at :28 says "last name successfully PUT" — something the code can't actually know.

Failure scenario: the reload-per-test shape this PR targets puts one test per session. Test A's onBeforeTest PUT hits a 429 or a socket blip → appliedName = "A" → sweep suppressed → that session permanently keeps its creation-time sessionName capability. That's the exact bug this PR fixes, reintroduced under transient failure — while the new "Final sweep" comment advertises a guarantee it no longer provides. It also compounds with the per-test PUT volume: more requests make rate-limiting more likely.

Suggestion: have markSessionName return response.ok and set appliedName only on 2xx. Alternatively, make the teardown sweep unconditional and de-dupe only the per-test calls — that costs one extra PUT per session, which is exactly what main does today, so it's a safe fallback.

Reviewer: fallback independent reviewer

this.sessionMap.set(sessionId, sessionData)
}

async onAfterTest(args: Record<string, unknown>) {
Expand Down Expand Up @@ -180,9 +217,8 @@ export default class AutomateModule extends BaseModule {
}
}

if (!testContextOptions.skipSessionName) {
await this.markSessionName(sessionId, sessionData.lastTestName, { user: userName, key: accessKey })
}
// Final sweep — a no-op for sessions already named per-test in onBeforeTest.
await this.flushSessionName(sessionId)

if (!testContextOptions.skipSessionStatus) {
await this.markSessionStatus(sessionId, sessionStatus, failureReason, { user: userName, key: accessKey })
Expand Down
Loading