feat(v8): platformise CucumberJS on the CLI/binary flow - #207
AdityaHirapara wants to merge 19 commits into
Conversation
Add 'cucumber' to CLIUtils.CLISupportedFrameworks, give setupTestFramework an explicit 'webdriverio-cucumber' branch (previously no else arm, so testFramework stayed null and every event was dropped without an error), and widen the field to the TestFramework base type every consumer already uses. Membership alone is not safe to ship. The legacy InsightsHandler -> Listener -> api/v1/batch transport is gated only on TESTOPS_BUILD_COMPLETED and BROWSERSTACK_TESTHUB_JWT, both of which the CLI flow sets itself, and none of the cucumber lifecycle hooks were CLI-guarded. Opening the gate alone would keep the legacy path POSTing TestRunStarted/TestRunFinished/CBTSessionCreated under the binary-issued JWT alongside the tracker, reporting every scenario twice under two uuids with no error. The guards for the emitting hooks therefore land here, not in a follow-up. beforeStep/afterStep are deliberately left unguarded: they emit nothing and only build the step list afterScenario reads back via hasTestStepFailures(), a read that is not CLI-gated and feeds the process exit code.
…rk class WdioCucumberTestFramework goes from an inert stub to the cucumber event tracker. It extends the base TestFramework, not the mocha one: WDIO never calls beforeTest/afterTest for cucumber and its hook invocations carry no title, so mocha's INIT_TEST/TEST/hook boundaries have no source here. A scenario raises TEST/PRE at beforeScenario and TEST/POST at afterScenario. That is forced rather than chosen — the binary's WDIO language index dispatches only on TEST and ^(BEFORE_|AFTER_), and silently ignores anything else, so a scenario-specific state would have produced a green build with no tests. Raising TEST is also what fills automateModule's sessionMap, which is why the session is now named and marked. Hooks are classified from a state machine over the class's own cucumber bookkeeping, never from a title. util.ts getHookType() is left alone: it matches Mocha's quoted titles and widening it would change mocha and jasmine too, so service.beforeHook/afterHook discriminate on the framework instead. Before this, every cucumber hook boundary threw inside an awaited WDIO hook and the hook was lost with a logged stack trace in a green build. The step-depth counter is deliberately not reset per scenario, matching the legacy handler: one missed afterStep classifies every later AFTER_EACH as unreported for the rest of the run, and reproducing that is parity. bdd_meta_info.feature.path is sent ABSOLUTE. The binary re-bases it against the project path for file_name/location and the git root for vc_filepath; pre-relativising on this side makes both fields depend on the binary's own cwd (SDK-7233). A failed BeforeAll abandons the whole feature, so every scenario it never reached (Rule-nested included) is reported skipped. Those rows are built as detached instances and sent straight to TestHub — routing them through the observers would rename the session, stop accessibility and run a Percy teardown per row. A hook finish with no recorded start drops both the finish and the cascade, as the legacy path does. accessibilityModule now owns the per-scenario Web A11y scan on this flow. It observes the TEST states raised above, and leaving the legacy handler live alongside it ran sendTestStopEvent twice per scenario against the same test run uuid — while that handler is only half-initialised here, since service.before() never calls its before(). preferScenarioName was a silent no-op on this flow: the only _updateJob carrying the name is gated off while the binary is up, and the session had already been named after the feature. It is written explicitly now, after the EXECUTE/POST tracker call rather than racing it. Unreachable for mocha and jasmine, whose scenario array is never populated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…on and session verdict Hook-scoped logs now carry their hook's state on the log record, so the binary keys them to hook_run_uuid instead of stamping a hook uuid into test_run_uuid. automateModule gains a build-level hook observer, so a failed BeforeAll/AfterAll fails the session as legacy's after() did; keys per-scenario results on the scenario rather than the feature name; and treats a skipped scenario as legacy's _failureStatuses does. All cucumber-gated — mocha's path is unchanged. accessibilityModule's scan decision passes the world through, restoring the tag-aware filter cucumber has on legacy.
…alse setSessionName suppresses the session NAME, not its registration. Skipping registration left onAfterExecute with nothing to status-mark, so a failing run under setSessionName: false reported no status at all where legacy marked it — its after() status block gates on setSessionStatus alone. Applies to every framework on the CLI flow, wdio_mocha included: a mocha session that is unmarked today becomes status-marked. The name stays suppressed, since onAfterExecute's naming call is guarded on both the flag and a non-empty name.
…observer failures An Automate-only CLI run (every TestHub product off) never registered its driver: service.before() raised AutomationFrameworkState.CREATE/POST only inside the shouldProcessEventForTesthub block, so webdriverIOModule never recorded the session id or capabilities. automateModule's sessionMap stayed empty and the Automate session was left unmarked, where the legacy flow marked it. Session tracking does not depend on TestHub, so the raise is now gated on the CLI being up and nothing else. On cucumber the same gap also failed every scenario: testHubModule's session event dereferenced the missing session id, re-threw, and — because the event was neither awaited nor caught — surfaced as an unhandled rejection inside the user's own cucumber Before hook, skipping every step. The event is now awaited, and eventDispatcher gives each observer its own boundary so one module's failure neither aborts the observers registered after it nor escapes into the framework hook that raised the state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…call beforeFeature awaited _setSessionName before reaching onFeatureStart, so any rejection of that session-update call aborted the rest of the hook and the CLI framework never saw the feature. Observability then lost the feature path for that feature (root file_path empty). Reordering is safe because the two statements are independent: onFeatureStart only assigns local bookkeeping from its own arguments and raises no wire event, and _setSessionName sets nothing it reads. Cucumber-only hook, so mocha and jasmine are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sessionNameFormat is a function, so JSON.stringify drops it and it never reaches the binary. automateModule, which is written to apply it, therefore fell back to the raw suite title and PUT that over the correctly formatted name the SDK had already written. Carry a sessionNameFormatProvided boolean across the boundary instead, and have automateModule defer its name write when a formatter exists, leaving the SDK — which holds the live function — as the naming authority. beforeSuite now also names mocha on the CLI flow in that case, where previously nothing applied the format at all. Nothing changes when sessionNameFormat is unset. The guard sits at the single write site, so lastTestName, the cucumber result key and percy's session-name read are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Supersedes the previous approach, which made the legacy-side _setSessionName the namer whenever sessionNameFormat was set. That split session naming across two code paths on the same flow and reached into the legacy path to do CLI work. automateModule runs in the SDK process, so it never needed the formatter to survive the gRPC/JSON round trip -- it only needed to stop reading the round-tripped copy, where function-valued keys are silently dropped. It now takes the live service options by injection and resolves sessionNameFormat from them, so every naming decision stays in one place. Injected rather than imported: cli/index.ts constructs this module, so importing it back closes an ESM cycle (it surfaced as "Class extends value is not a constructor" in wdioMochaTestFramework). Adds two tests. The first shapes testContextOptions as the binary really returns it -- with no sessionNameFormat key -- and asserts the formatted name still reaches the session; it fails against the previous code, so it pins the behaviour rather than merely passing alongside it. The second asserts the suite title is used when no formatter is configured, guarding the claim that nothing changes for the unset case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited), Workspace UI (inherited) Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
…platformisation-v8 # Conflicts: # packages/browserstack-service/src/cli/modules/testHubModule.ts
service.ts decided this on the CLI path and wrote the name itself, which split session naming across two code paths on the same flow -- the module named the session at TEST/PRE from the feature title, and service.ts renamed it afterwards from its own scenario list. automateModule now counts non-skipped cucumber scenarios and applies the rename at EXECUTE/POST, the first point where "exactly one ran" is knowable. service.ts carries preferScenarioName on the cucumber TEST/POST event and keeps the rename only for the legacy path. This matches the shape the v9 line already uses. The scenario is tracked before the skipSessionStatus return: setSessionStatus false opts out of the status, not of the rename. Also completes the TestFramework test mock with getState. isCucumberInstance was previously reached only through a short-circuit, so the missing mock method never surfaced; it is called unconditionally now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sation-v8' into SDK-7606/wdio-cucumber-platformisation-v8
Per-File Confidence
Change map (generated deterministically from the diff)graph LR
subgraph nwdio_browserstack_service["wdio-browserstack-service"]
npackages_browserstack_service_src_cli_frameworks_wdioCucumberTestFramework_ts["wdioCucumberTestFramework.ts<br/>~663 lines"]
npackages_browserstack_service_tests_cli_frameworks_wdioCucumberTestFramework_test_ts["wdioCucumberTestFramework.test.ts<br/>~275 lines"]
npackages_browserstack_service_src_service_ts["service.ts<br/>~231 lines"]
npackages_browserstack_service_tests_service_test_ts["service.test.ts<br/>~227 lines"]
npackages_browserstack_service_src_cli_modules_automateModule_ts["automateModule.ts<br/>~158 lines"]
npackages_browserstack_service_tests_cli_modules_automateModule_test_ts["automateModule.test.ts<br/>~158 lines"]
npackages_browserstack_service_src_cli_eventDispatcher_ts["⚠ eventDispatcher.ts<br/>~13 lines"]
npackages_browserstack_service_src_cli_cliUtils_ts["cliUtils.ts<br/>~8 lines"]
npackages_browserstack_service_src_cli_frameworks_constants_testFrameworkConstants_ts["testFrameworkConstants.ts<br/>~8 lines"]
npackages_browserstack_service_src_cli_index_ts["index.ts<br/>~8 lines"]
npackages_browserstack_service_src_cli_modules_accessibilityModule_ts["accessibilityModule.ts<br/>~8 lines"]
npackages_browserstack_service_src_cli_modules_testHubModule_ts["testHubModule.ts<br/>~8 lines"]
npackages_browserstack_service_src_constants_ts["constants.ts<br/>~8 lines"]
npackages_browserstack_service_src_insights_handler_ts["insights-handler.ts<br/>~8 lines"]
n_changeset_pr_207_md["pr-207.md<br/>~6 lines"]
npackages_browserstack_service_tests_skipAppOverride_test_ts["skipAppOverride.test.ts<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 |
AdityaHirapara
left a comment
AdityaHirapara
left a comment
There was a problem hiding this comment.
PR Review: wdio-browserstack-service PR #207
Summary
Intent: Moves CucumberJS + WebdriverIO (v8 line) off the legacy in-SDK reporting flow onto the CLI/binary gRPC flow, adding a new WdioCucumberTestFramework, widening the shared CLI dispatch gates for it, and fixing two parity defects (sessionNameFormat being ignored on the CLI flow, and Observability losing a feature's file path when a session-name update failed) found during verification.
Risk: Medium — no gRPC/proto changes and no Binary PR pairing was needed, but the change rewires four core WDIO lifecycle hooks (beforeScenario, afterScenario, beforeHook, afterHook) and adds a large (651-line) new framework class.
0 critical · 3 warnings · 0 suggestions | Files reviewed: 12
═══════════════════════════════════════════════════════════════
Findings
Two channels. Blocking = Critical + Warning — the must-fix set the Verdict gates on. Non-blocking = Suggestions. All three findings below are ungrounded (🟡) — genuine open questions I could not settle from the diff alone, not confirmed defects.
🔴 Critical (Blocking)
None.
🟠 Warnings (Blocking)
| # | Finding | File · Symbol | Confidence |
|---|---|---|---|
| 1 | [Graceful Degradation] trackEvent's outer calls aren't covered by its own try/catch, and none of its callers wrap it either | packages/browserstack-service/src/cli/frameworks/wdioCucumberTestFramework.ts · WdioCucumberTestFramework.trackEvent |
🟡 |
| 2 | [Test Coverage] New cucumber dispatch logic in service.ts has no direct test coverage in this PR | packages/browserstack-service/src/service.ts · BrowserstackService._cucumberTestResult / _cliCucumberFramework |
🟡 |
| 3 | [Correctness] afterScenario's legacy Percy call now runs even when the binary/CLI is active, if the cucumber framework instance failed to attach | packages/browserstack-service/src/service.ts · BrowserstackService.afterScenario |
🟡 |
1. [Graceful Degradation] trackEvent's outer calls aren't covered by its own try/catch, and none of its callers wrap it either — packages/browserstack-service/src/cli/frameworks/wdioCucumberTestFramework.ts · WdioCucumberTestFramework.trackEvent
Problem:
trackEvent()wraps only its middle section (theinstance.updateMultipleEntriesblock) in a try/catch.await super.trackEvent(...),this.resolveInstance(...), and the trailingawait this.runHooks(instance, ...)all sit outside that boundary. None of the four call sites inservice.ts(beforeScenario,afterScenario,beforeHook,afterHook) wrapcliFramework.trackEvent(...)in a try/catch either.If any of those three unguarded calls throws for a given scenario, the exception propagates straight into the customer's WDIO hook and fails/errors their scenario — the exact class of defect the sibling fix in
eventDispatcher.ts(also in this PR) was written to prevent, but that fix only protects observer callbacks invoked throughnotifyObserver; it does not reachsuper.trackEvent()orresolveInstance(), which run before any observer is ever reached.Suggested Fix:
Confirm whether this exact shape already exists in the baseTestFrameworkclass or in the siblingWdioMochaTestFramework(not touched by this PR) — if so, this is intentional parity with an already-shipped pattern and can be dismissed. If not, widen the try/catch intrackEvent()to cover the full method body (or wrap the four call sites inservice.ts), so a BrowserStack-side instrumentation failure degrades to a debug log instead of failing the customer's scenario.Confidence: 🟡 — I could not see the base
TestFrameworkclass or the Mocha sibling in this diff, so I can't confirm whether this is a new gap or an established, working pattern being extended to cucumber.
───────────────────────────────────────────────────────────────
2. [Test Coverage] New cucumber dispatch logic in service.ts has no direct test coverage in this PR — packages/browserstack-service/src/service.ts · BrowserstackService._cucumberTestResult / _cliCucumberFramework
Problem:
service.tsgained several new private methods (_cucumberTestView,_cucumberTestResult,_reportCucumberScenariosSkipped) and rewired four public hook methods to drive the CLI/binary cucumber flow. UnlikewdioCucumberTestFramework.ts, which shipped with a dense, assertion-real new test file, none of this orchestration logic has a direct test in this PR. A regression in the view-mapping or in which branch each hook takes would only surface via an end-to-end/regression run.Suggested Fix:
Ifservice.tsalready has an existing test suite (untouched by this PR), add cases for the new cucumber branches — the_cliCucumberFramework()truthy path inbeforeScenario/afterScenario, theBEFORE_ALLcascade trigger inafterHook, and_cucumberTestResult'shookOnlyFailure/failed/skippedmapping. If coverage relies entirely on the BStackAutomation regression suite, that's a reasonable call — just worth being explicit that's the intended coverage story.Confidence: 🟡 — I could not confirm whether a pre-existing
service.tstest file (not touched by this PR) already exercises these paths.
───────────────────────────────────────────────────────────────
3. [Correctness] afterScenario's legacy Percy call now runs even when the binary/CLI is active, if the cucumber framework instance failed to attach — packages/browserstack-service/src/service.ts · BrowserstackService.afterScenario
Problem:
Before this change,this._percyHandler?.afterScenario()in the legacy (non-CLI-framework) branch ofafterScenarioonly ran whenisRunning()was false. That guard was removed, so it now runs unconditionally whenever_cliCucumberFramework()returns null — which includes the edge case where the binary/CLI is running butgetTestFramework()didn't resolve to aWdioCucumberTestFrameworkinstance for some reason. If Percy's teardown is also driven from somewhere in the CLI/binary flow for that same case, this could double-run it.Suggested Fix:
Confirm there's no CLI/binary-side Percy teardown that would now double-fire alongside this legacy call when the cucumber framework fails to attach. If there genuinely isn't one, this is fine as written — worth a one-line comment stating why the guard was dropped, matching how precisely every other change in this file is documented.Confidence: 🟡 —
getTestFramework()'s failure modes live outside this unit's files, and the change could equally be a deliberate fallback-safety improvement rather than a regression; I can't rule either way from the diff.
───────────────────────────────────────────────────────────────
💡 Suggestions (Non-blocking)
None.
═══════════════════════════════════════════════════════════════
External Services
No external-contract changes detected.
═══════════════════════════════════════════════════════════════
Per-File Confidence (for reviewers)
| File | Status | Reason |
|---|---|---|
.changeset/pr-207.md |
✅ All Clear | No issues found |
packages/browserstack-service/src/cli/cliUtils.ts |
✅ All Clear | No issues found |
packages/browserstack-service/src/cli/eventDispatcher.ts |
✅ All Clear | No issues found |
packages/browserstack-service/src/cli/frameworks/constants/testFrameworkConstants.ts |
✅ All Clear | No issues found |
packages/browserstack-service/src/cli/frameworks/wdioCucumberTestFramework.ts |
🔴 Author to Fix 1 | 1 ungrounded finding (trackEvent error-boundary coverage) — verify independently, not a confirmed defect |
packages/browserstack-service/src/cli/index.ts |
✅ All Clear | No issues found |
packages/browserstack-service/src/cli/modules/accessibilityModule.ts |
✅ All Clear | No issues found |
packages/browserstack-service/src/cli/modules/automateModule.ts |
✅ All Clear | No issues found |
packages/browserstack-service/src/cli/modules/testHubModule.ts |
✅ All Clear | No issues found |
packages/browserstack-service/src/service.ts |
🔴 Author to Fix 2 | 2 ungrounded findings (missing test coverage for new dispatch logic; a removed isRunning() guard around the legacy Percy call) — verify independently, not confirmed defects |
packages/browserstack-service/tests/cli/frameworks/wdioCucumberTestFramework.test.ts |
✅ All Clear | No issues found |
packages/browserstack-service/tests/cli/modules/automateModule.test.ts |
✅ All Clear | No issues found |
═══════════════════════════════════════════════════════════════
What's Good
eventDispatcher.notifyObservernow wraps each observer callback in its own try/catch — one failing module can no longer abort delivery to every module registered after it, and the failure is still logged rather than swallowed.automateModule.onAfterTestnow keyssessionData.testResultsontest.fullNameinstead of the session name, fixing a real observability collapse: every scenario in a cucumber feature previously shared one session-name key, so a feature's last scenario silently overwrote every earlier scenario's result.- The new
WdioCucumberTestFrameworkclass ships with dense, assertion-real test coverage (hook classification, scenario identity, result mapping, the BEFORE_ALL skip cascade, log routing) and unusually precise inline documentation of every non-obvious design decision, including deliberate v8-parity choices.
═══════════════════════════════════════════════════════════════
Verdict
wdioCucumberTestFramework.ts · trackEvent— confirm whether the unguarded outer calls mirror an existing pattern in the base TestFramework/Mocha sibling, or need a wider try/catch.service.ts · _cucumberTestResult / _cliCucumberFramework— confirm whether the new dispatch logic already has test coverage elsewhere, or note the regression suite is the intended coverage story.service.ts · afterScenario— confirm the removedisRunning()guard around the legacy Percy call can't double-fire against a CLI-side Percy teardown.
═══════════════════════════════════════════════════════════════
— SDK PR Review Agent
Addresses two findings from the PR review.
service.ts gained the cucumber CLI dispatch without direct unit coverage —
tests/service.test.ts never mocked BrowserstackCLI, so all of its existing
cases ran the legacy branch only. Adds the CLI side:
- beforeScenario / afterScenario raise TEST/PRE and TEST/POST on the
cucumber framework and leave the legacy handlers alone
- afterScenario still runs the legacy Percy teardown when the binary is up
but holds a non-cucumber framework
- the BEFORE_ALL skip cascade fires on a failed BEFORE_ALL only, not on a
passing one and not on BEFORE_EACH
- _cucumberTestResult / _cucumberTestView mapping, folded into the existing
afterScenario suite since that is their only call site
Also documents why the !isRunning() guard was dropped from afterScenario's
legacy Percy call: the CLI branch returns before it, so percyModule's
observer and this teardown are mutually exclusive. The guard additionally
skipped Percy when the binary ran a non-cucumber framework, which raises no
TEST/POST either — the teardown was lost there rather than duplicated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — all three settled. Two needed no change, one did and is fixed in 7d82e6c. 1.
|
wdioMochaTestFramework.ts |
wdioCucumberTestFramework.ts |
|
|---|---|---|
await super.trackEvent(...) |
outside try | outside try |
resolveInstance(...) |
outside try | outside try |
try { … } catch |
middle block | middle block |
await this.runHooks(...) |
outside try | outside try |
Two further points blunt the stated risk:
- the base
TestFramework.trackEvent(src/cli/frameworks/testFramework.ts:83-85) is a two-linelogger.infocall; - cucumber's
resolveInstance(wdioCucumberTestFramework.ts:275-292) returnsnullwith a debug log rather than throwing.
This is the shipped mocha pattern extended to cucumber, not a new gap. No change.
2. No direct test coverage for the new service.ts dispatch — valid, fixed
Confirmed as stated. tests/service.test.ts existed (1,919 lines, 95 it() blocks, 78 references to the four cucumber hooks) but never mocked BrowserstackCLI — so every one of those cases ran the legacy branch, and the CLI branch had no unit coverage at all.
7d82e6c adds 13 cases:
beforeScenario/afterScenarioraiseTEST/PREandTEST/POSTon the cucumber framework and leave the legacy handlers alone;afterScenariostill runs the legacy Percy teardown when the binary is up but holds a non-cucumber framework (the finding-3 guard, below);- the
BEFORE_ALLskip cascade fires on a failedBEFORE_ALLonly — not on a passing one, not onBEFORE_EACH; _cucumberTestResult/_cucumberTestViewmapping — passed, failed, skipped, the synthesised pending reason, and theignoreHooksStatushook-only-failure pair. These are folded into the existingdescribe('afterScenario')block rather than a new one, sinceafterScenariois their only call site.
tests/service.test.ts 112/112 green; tsc --noEmit clean.
3. Dropped isRunning() guard on the legacy Percy call — no double-fire
The guard removal is real, and there is a CLI-side Percy teardown — percyModule.ts:31 registers onAfterTest on TEST/POST. But the two paths are gated on the same condition and are mutually exclusive:
cliFrameworktruthy →trackEvent(TEST, POST)firespercyModule.onAfterTest, thenreturn— the legacy call is never reached;cliFrameworknull → notrackEvent, sopercyModule.onAfterTestnever fires — the legacy call runs.
In the exact edge case raised (binary running, getTestFramework() not a WdioCucumberTestFramework), the old code fired neither — Percy teardown was dropped on the floor. Removing the guard closes that hole rather than duplicating anything.
Cucumber does deviate from mocha here: mocha gates its early return on isRunning() alone (service.ts:528), cucumber on the narrower _cliCucumberFramework(). That is deliberate but was undocumented — 7d82e6c adds the four-line comment, and the regression test in finding 2 pins the behaviour.
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. 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. |
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge. |
1 similar comment
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge. |
| */ | ||
| private _cucumberTestView(world: ITestCaseHookParameter): Frameworks.Test { | ||
| return { | ||
| title: undefined, |
There was a problem hiding this comment.
preferScenarioName never renames the cucumber session on the CLI flow. _cucumberTestView() sets title: undefined for every scenario, so in automateModule.onAfterTest const testTitle = test.title is always undefined → nameData.lastScenarioName = testTitle is always undefined → the onAfterExecute gate preferScenarioName && scenariosRan === 1 && sessionData.lastScenarioName never passes. A single-scenario session keeps the Feature title instead of the scenario name.
Evidence: the v9 sibling (#191) populates title: scenarioName here, which is what makes the rename work. Note the automateModule.test.ts case "renames the session…" passes test: { title: ... } directly, bypassing _cucumberTestView, so it goes green while the integrated path is dead.
Fix: populate title with the scenario name here (as v9 does), or in automateModule read lastScenarioName from world.pickle.name rather than test.title.
There was a problem hiding this comment.
Confirmed and fixed in 4444852 — but not the way #191 does it.
Populating title here would break v8 parity: automateModule passes test.title straight into sessionNameFormat as its fourth argument, and legacy calls _setSessionName(feature.name) with no test argument, so that callback has to receive undefined. title: undefined is load-bearing, and the comment above this method says so.
Took your alternative instead — automateModule now reads the scenario name off the live world rather than test.title. Safe because that observer runs in-process, so world never goes through the binary's JSON round-trip.
Your note about the test was the most useful part: it passed a hand-built test: { title: … }, bypassing _cucumberTestView entirely, so it stayed green while the integrated path was dead. It now uses the real view shape and reads from world, and fails against the unfixed module.
| // last-write-wins entry, so a feature whose last scenario passes reports a passed | ||
| // session however many earlier ones failed. Mocha leaves `fullName` undefined, so its | ||
| // key is unchanged. | ||
| const resultKey = (test && test.fullName) ? String(test.fullName) : name |
There was a problem hiding this comment.
Scenario Outline example rows collide here. test.fullName is the raw pickle.name, which is identical for every Examples row of an Outline, so testResults.set(resultKey, …) is last-write-wins across rows. onAfterExecute aggregates testResults.values(), so an earlier failing row overwritten by a later passing row makes the Automate session report passed. (O11Y is unaffected — it uses per-row UUIDs.)
Evidence: the v9 sibling (#191) keys cucumber results on KEY_TEST_UUID for exactly this reason, with the comment that fullName "is the raw pickle name, shared by every Examples row." The repo already has the correct identity: getUniqueIdentifierForCucumber() in util.ts (pickle.uri + astNodeIds).
Fix: for cucumber, key on the scenario KEY_TEST_UUID (as v9 does) or on getUniqueIdentifierForCucumber(world).
There was a problem hiding this comment.
Confirmed and fixed in 4444852 — cucumber now keys on the scenario's own KEY_TEST_UUID, falling back to the previous key for everything else, so mocha's key is unchanged.
One scope note for the record: #191's own comment carries a qualifier worth keeping — fullName is shared across Examples rows only of an outline whose title carries no placeholder. With a placeholder, Cucumber interpolates and the names differ. Still a real bug, just narrower than "Examples rows collide".
Added a regression test: two rows sharing fullName, first failing and second passing, asserting the session stays failed. It goes red against the unfixed module.
| if (nameData) { | ||
| nameData.scenariosRan++ | ||
| nameData.lastScenarioName = testTitle | ||
| nameData.preferScenarioName = isTrue(args.preferScenarioName) |
There was a problem hiding this comment.
preferScenarioName is newly surfaced for the CLI flow here, but it was not added to NOT_ALLOWED_KEYS_IN_CAPS in constants.ts (still 4 entries). launcher.ts uses that list to strip SDK-only keys from bstack:options / browserstack.* before capabilities reach the hub, so a preferScenarioName placed there can leak into the actual W3C capabilities.
Evidence: the v9 sibling (#191) adds preferScenarioName to NOT_ALLOWED_KEYS_IN_CAPS in the same PR.
Fix: add 'preferScenarioName' to NOT_ALLOWED_KEYS_IN_CAPS.
There was a problem hiding this comment.
Right that it's missing, fixed in 3595271 + 3cdbfec — though it predates this PR: the option and the omission both exist at merge-base.
The useful part was where it pointed. We had this logged internally as needing a binary fix, reasoning that the binary is what injects the key into the capabilities. Wrong: the binary injects it, but the strip two lines later is pure SDK code and simply didn't list it. We'd located the fix at the origin of the defect rather than at the last writer before the wire, and your cross-reference against #191 is what corrected that.
Two keys were needed rather than one — sessionNamePrependTopLevelSuiteTitle reaches the capabilities by the same route and was rejected identically; sessionNameOmitTestTitle is listed with it as the same family. All three are read from testContextOptions, so stripping them from the outgoing capabilities doesn't disable any of them.
Note this also changes mocha and jasmine: an option that is a no-op for them stops crashing the run. Deliberate, not incidental.
| const featurePath = this.featurePath() | ||
| return { | ||
| [TestFrameworkConstants.KEY_TEST_FILE_PATH]: featurePath, | ||
| [TestFrameworkConstants.KEY_TEST_LOCATION]: featurePath, |
There was a problem hiding this comment.
This sends an absolute filesystem path for both KEY_TEST_FILE_PATH and KEY_TEST_LOCATION, and buildBddMetaInfo likewise sends featurePath() (absolute) into bdd_meta_info.feature.path. The v9 sibling (#191) diverges: it sends KEY_TEST_LOCATION cwd-relative (path.relative(process.cwd(), …)) and uses a dedicated cwd-relative helper for the meta path, with a comment that legacy reads gherkinDocument.uri, which is cwd-relative — "NOT the absolute path."
The two siblings resolve SDK-7233 in opposite directions, so at least one is wrong: either v8 leaks each runner's absolute machine path into location/bdd_meta.feature.path, or v9 double-rebases.
Question: the comment above featurePath() says the binary re-bases both file_name and location from the absolute path — but v9 pre-relativises location and the meta path anyway. Which is correct? If the binary only re-bases test_file_path, v8 is leaking absolute paths for location and the BDD meta blob.
There was a problem hiding this comment.
Two halves with opposite answers, and my first read collapsed them into one verdict and got the second one wrong.
bdd_meta.feature.path — you were right, fixed in 0827727 + 6ab845c. I had it filed as a binary-side defect and therefore not ours to fix. Wrong, and wrong the same way as the capabilities finding: the binary re-bases test_file_path/location but never touches this blob, so whatever the SDK puts there reaches the dashboard verbatim — and it measurably carried a developer home directory where the legacy flow published a repo-relative path. #191 already solved it with a dedicated cwd-relative helper used only for the meta blob. Ported.
Auditing every path-bearing emit site afterwards turned up a second instance: the BEFORE_ALL skip cascade builds its own meta blob inline rather than going through the shared builder, so it was still sending the absolute path. #191 has the helper at both sites; the v8 port had it at neither. Both fixed, with tests pinning the split.
KEY_TEST_LOCATION — staying absolute, on measurement. The binary does re-base this one, with the comment that pre-relativising on the SDK side broke both fields (SDK-7233). Measured on both arms: file_name, location and the O11Y filePath all come out features/cp2-a.feature. So the answer to "which is correct" differs per field, which is why the single verdict was wrong.
There was a problem hiding this comment.
Re-checked at head 6ab845c: the bdd_meta_info.feature.path leak is fixed (now cwd-relative via featureUriForMeta(), both normal and skip-cascade blobs, with a test). Approving — not blocking on this.
One residual to confirm (non-blocking): KEY_TEST_LOCATION is still sent absolute here, whereas the v9 sibling (#191) sends it cwd-relative. If the binary really re-bases test_location (as the comment states), absolute is correct; if it only re-bases test_file_path, this field leaks the absolute path the way the meta blob did before this fix. Worth a quick confirm with the binary team or a dashboard check.
07souravkunda
left a comment
07souravkunda
left a comment
There was a problem hiding this comment.
Reviewed as a port of the v9 sibling (#191). The framework state machine and documented legacy-parity deviations (PB-V8-1, SDK-7233) look sound, and the eventDispatcher/testHubModule/KEY_HOOK_STATE additions are improvements over v9 worth backporting. No merge-blockers.
The concerns are all cross-references against v9 — fixes that exist in #191 but did not make it into this port. Four inline comments: two parity gaps I would fix before merge (preferScenarioName never renames the session; Scenario Outline rows collide in the Automate result map and can flip session status to passed), one capability-leak (preferScenarioName missing from NOT_ALLOWED_KEYS_IN_CAPS), and one path-handling divergence to reconcile (KEY_TEST_LOCATION / bdd_meta.feature.path absolute vs v9 cwd-relative).
Two open questions the diff alone could not settle — customTagsModule has no cucumber gate on the CLI path (v9 added one), and insights-handler.setTestData (SDK-4177) is not wired into CLI cucumber beforeScenario (v9 ported it). Worth a look to confirm whether either is a real gap for cucumber on the binary flow.
…e rows Two parity defects found in PR review. preferScenarioName never renamed the session on the CLI flow. onAfterTest read the scenario name from `test.title`, which _cucumberTestView leaves undefined on purpose — legacy calls _setSessionName(feature.name) with no test argument, so sessionNameFormat has to receive undefined as its fourth argument. Populating `title` would have changed what every sessionNameFormat callback is handed, so the name is read off the live `world` instead; this observer runs in-process, so the object has not been through the binary's JSON round-trip. Automate results keyed on `test.fullName`, the raw pickle name, which every Examples row of an outline shares when the outline title carries no placeholder. Those rows collapsed last-write-wins, so a failing row followed by a passing one reported the session passed. Cucumber now keys on the scenario's own uuid; mocha's key is unchanged. The existing preferScenarioName test passed a fabricated `test.title`, bypassing _cucumberTestView, so it stayed green while the integrated path was dead. It now uses the real view shape and reads the name from `world`, and the blanket getState mock is keyed so it stops answering every state identically. Adds an outline-collision regression test. Both tests fail against the unfixed module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SDK-4177. browserCommand resolves the active test through insightsHandler's `_tests` map and returns before the SDK-6277 screenshot branch when it finds no entry. Legacy seeded that map from insights-handler's beforeScenario, which the CLI branch skips, and TestReporter's map is keyed on `testStats.fullTitle` / `getHookIdentifier` — neither matches `getUniqueIdentifierForCucumber(world)`. So screenshots were dropped for cucumber on the binary flow where legacy forwarded them. beforeScenario now reads the scenario uuid after the TEST/PRE event — the uuid is minted by loadScenarioData during that event, so reading earlier would seed the previous scenario's — and hands it to setTestData. The lookup is guarded: getState dereferences the instance, so an absent tracked instance would have thrown out of the customer's hook rather than just losing the attribution. setTestData accepts a world and lets it through to seed `_tests`. The key moves to getIdentifier(), which for every non-pickle test returns getUniqueIdentifier(test, framework) — the previous key unchanged, so mocha and jasmine keep their behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
On the CLI flow service.before() merges the capabilities the binary hands back, and those carry the SDK's own options. NOT_ALLOWED_KEYS_IN_CAPS is the only strip between that merge and the hub, and it did not list preferScenarioName, so the hub rejected the session outright — `additional properties ["preferScenarioName"] outside of the schema` — and the run exited 1 with no session where the legacy flow, which never calls the binary, exited 0. The leak is framework-agnostic, so this changes mocha and jasmine too: an option that is a no-op for them stops crashing the run. preferScenarioName is cucumber-only — _scenariosThatRan is pushed only in afterScenario, so the `length === 1` gate can never fire for mocha. Unblocks the three cucumber session-naming parity rows, which could not be measured while no session was ever created. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The earlier strip covered preferScenarioName only. sessionNamePrependTopLevelSuiteTitle
reaches the outgoing capabilities by the same route and the hub rejects it the same way
("additional properties [...] outside of the schema"), which is what kept the cucumber
session-name parity row unmeasurable. sessionNameOmitTestTitle is listed alongside it:
same family, same route into caps, not a W3C capability.
Both are read from testContextOptions, so stripping them from the outgoing capabilities
does not disable either option.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The binary re-bases test_file_path and location itself but never touches the bdd_meta blob, so the absolute path we put there reached the dashboard verbatim, home directory included — where the legacy flow published a repo-relative path read off the cucumber world's gherkinDocument.uri. Adds featureUriForMeta(), used only for the meta blob; the file-path pair stays absolute because the binary owns re-basing those. The existing test asserted the meta path equalled the absolute feature uri, which encoded the defect; it now asserts cwd-relative and pins the file path as still absolute. Verified at unit level and by source inspection. The O11Y read-back that produced the original evidence for this defect was not reproduced — the testRuns endpoint returned no rows for the fix run — so dashboard confirmation is still outstanding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lob too The BEFORE_ALL cascade builds its own bdd_meta blob inline rather than going through buildBddMetaInfo, so the previous fix missed it and cascade rows kept sending the absolute feature path into a blob the binary never re-bases. Per-field contract, now consistent at all five emit sites: test_file_path and location go out absolute because the binary re-bases them; every bdd_meta feature.path goes out cwd-relative because the binary leaves that blob alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — this was a high-yield review. Three of the four findings were real, one of the two open questions was real, and the note about the 1.
|
07souravkunda
left a comment
07souravkunda
left a comment
There was a problem hiding this comment.
Re-checked at head 6ab845c — all four findings from the prior review are addressed correctly and unit tests pass on node 18/20/22: preferScenarioName now renames off world.pickle.name; the Automate result map keys cucumber on the per-scenario/per-row KEY_TEST_UUID (with a regression test for the outline failing-then-passing case); the SDK-only session-name options are stripped from outgoing caps; and the BDD meta feature path is now cwd-relative. SDK-4177 screenshot attribution is wired via setTestData, and customTags is correctly out of scope (v9-only, mocha-only). Approving. One non-blocking follow-up left inline: confirm the binary re-bases KEY_TEST_LOCATION, which still differs from the v9 sibling.
|
🟢 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 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. |
3 participants
What is this about?
Moves CucumberJS + WebdriverIO on the WDIO v8 line off the legacy in-SDK reporting flow onto the CLI/binary gRPC flow, to functional parity with legacy.
Cucumber joins
CLISupportedFrameworksand reports through a newWdioCucumberTestFrameworkclass, with the shared dispatch gates widened for it. Two parity defects found during verification are fixed here as well.⚠ The gate flip alone is unsafe and is not what this PR does.
service.ts's cucumber hooks are unguarded where mocha's are guarded, so flippingCLISupportedFrameworkswithout addingisRunning()guards in the same change reports every scenario twice — over two transports, under two uuids, with no error and a build that looks populated. The gate and the guards land together here.Zero binary changes — the thick layer already supported this combination.
Verification (7 checkpoints, 50 parity rows, full BSA suite vs matched legacy)
Known limits, stated rather than smoothed: turboscale could not actually be exercised (no grid provisioned on the test account, so runs degrade to regular Automate); Percy matches legacy on what its two tests assert but that is not a broader Percy claim; and the legacy comparison arm ran 8.51.0 in three of five directories, a confound in legacy's favour.
Related Jira task/s
SDK-7606 (epic SDK-7053)
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)
sessionNameFormatbeing ignored, so custom session names now apply on the CLI flow.Release notes (internal): (required — engineer-facing; what actually changed / why)
CLISupportedFrameworksnow includescucumber; scenarios and hooks report via a newWdioCucumberTestFramework, and the shared dispatch gates inservice.tsare widened for it. TheisRunning()guards ship in the same change — without them the unguarded cucumber hooks double-report every scenario across both transports.sessionNameFormatwas silently dropped on the CLI flow: it is a function, soJSON.stringifyremoved it from the config round-tripped through the binary, andautomateModulethen overwrote the correct name with the raw suite title.automateModulenow resolves the formatter from the live in-process service options (injected, not imported — importingcli/index.tsback closes an ESM cycle), keeping all session naming in one place. Also fixes the same loss forwdio_mocha, which never wrote the formatted name at all.beforeFeatureawaited a session-update REST call before emitting the CLI feature-start event, so any rejection of that call (a 4xx/5xx) aborted the hook and Observability lost the feature path. The event is now emitted first; it raises no wire event, so ordering is unaffected.Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.