fix(SDK-4165): keep buildIdentifier when BROWSERSTACK_BUILD_NAME is set - #192
harshit-browserstack wants to merge 3 commits into
Conversation
_handleBuildIdentifier skipped buildIdentifier resolution whenever
BROWSERSTACK_BUILD_NAME was set, even though a buildName was available.
The skip also deleted buildIdentifier from the capabilities only — this
._buildIdentifier kept the unresolved '#${BUILD_NUMBER}' template, which
onPrepare forwards to TestHub as build_identifier.
Consequence: successive runs sharing a build name were never
disambiguated, so their sessions collapsed into a single build.
The guard now keys solely on the absence of a buildName, which is what
its own warning always claimed and what the binary's handleBuildIdentifier
already does (its BROWSERSTACK_BUILD_NAME skip is commented out). The
skip path additionally clears _buildIdentifier so the raw template is
never reported as a value.
Co-Authored-By: Claude Opus 5 (1M context) <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 |
|
Additional regression check — the binary path (wdio + mocha, where This is the path the BStackAutomation wdio_mocha wrappers use, and it is the one behaviourally changed by removing the env disjunct: Post-fix, Two distinct builds, one identifier each — no double application, and the spurious |
|
✅ Good to go
Change map (generated deterministically from the diff)graph LR
subgraph nwdio_service["wdio-service"]
npackages_browserstack_service_src_launcher_ts["⚠ launcher.ts<br/>~20 lines"]
npackages_browserstack_service_tests_launcher_test_ts["⚠ launcher.test.ts<br/>~20 lines"]
n_changeset_pr_192_md["pr-192.md<br/>~5 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 |
|
Re: the v8 gap flagged above — confirmed, _handleBuildIdentifier on the v8 branch has the same guard bug. That's being tracked as a separate PR against v8 (will link here once it's up) rather than bundled |
|
🟢 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. |
|
RUN_TESTS |
|
RUN_TESTS |
…uild-name-env-set
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
|
RUN_TESTS |
|
🟢 SDK 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. |
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. |
What is this about?
Launcher._handleBuildIdentifierskipped buildIdentifier resolution wheneverBROWSERSTACK_BUILD_NAMEwas set — even though abuildNamewas available in that case. Its own warning (Skipping buildIdentifier as buildName is not passed.) contradicted the condition it fired under.The skip was also partial. It deleted
buildIdentifierfrom the capabilities but leftthis._buildIdentifierholding the unresolved#${BUILD_NUMBER}template, andonPrepareforwards that field to TestHub asbuild_identifier(src/launcher.ts:445).Net effect: successive runs sharing a build name were never disambiguated, so their sessions collapsed into a single build instead of
name #1,name #2.The guard now keys solely on the absence of a
buildName— which is exactly what its warning always claimed, and what the binary'shandleBuildIdentifieralready does (the equivalentBROWSERSTACK_BUILD_NAMEskip is commented out there,core/config/index.js:1203-1211). The skip path additionally clears_buildIdentifierso a raw template is never reported as a value.Scope of impact.
CLIUtils.CLISupportedFrameworks = ['mocha'], and the CLI is also disabled for multiremote. For every other configuration — WDIO + jasmine, WDIO + cucumber, and any multiremote run — the classiclaunchTestSessionpath is live and this defect applies.V8 Link :- #235
Related Jira task/s
SDK-4165 —
[Framework- WDIO v8 v9] Builds are getting merged for GRR user for AA session in o11y dashboardRelease (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)
buildIdentifierbeing ignored when theBROWSERSTACK_BUILD_NAMEenvironment variable is set. Successive runs that share a build name are now reported as separate builds (my-build #1,my-build #2) instead of merging into one.Release notes (internal): (required — engineer-facing; what actually changed / why)
_handleBuildIdentifierno longer treats a setBROWSERSTACK_BUILD_NAMEas a reason to skip resolution; the guard keys only on a missingbuildName, matching the binary'shandleBuildIdentifierand the warning's own wording.this._buildIdentifier, so the unresolved#${BUILD_NUMBER}/${DATE_TIME}template is no longer sent to TestHub asbuild_identifier.Checklist
How this was tested
App Automate (Android, Samsung Galaxy S22 Ultra),
buildIdentifier: '#${BUILD_NUMBER}',testObservability: true,BROWSERSTACK_BUILD_NAMEexported, same build name across each pair. Frameworkjasmineso the run takes the classic path. A temporary log line at thelaunchTestSessioncall site captured the value actually sent, and was removed before commit.BROWSERSTACK_BUILD_NAMEbuild_identifiersent"#${BUILD_NUMBER}"sdkFourOneSixFiveMergeDemo"#${BUILD_NUMBER}"08e21249…, 2 sessions"#1"sdkFourOneSixFiveCtrlDemo 1"#2"sdkFourOneSixFiveCtrlDemo 2"#1"sdkFourOneSixFiveFixedDemo 1"#2"sdkFourOneSixFiveFixedDemo 2GET /app-automate/builds/08e21249…/sessions.jsonreturnssession_count = 2(19:05:12Z and 19:06:19Z) — the two D runs merged. Each control and each fixed-state build returnssession_count = 1.Re-verified on WDIO v9 (
webdriverio@9.31.5): runs G1/G2 under the same conditions producedsdkFourOneSixFiveVNineFixed 1and2, and the spuriousSkipping buildIdentifierwarning is gone.Unit tests: all 10
_handleBuildIdentifiercases pass, including the pre-existingshould delete buildIdentifier if buildName is not present in caps. Two_uploadAppfailures intests/launcher.test.tsare pre-existing — confirmed by re-running that block on an unmodified checkout.npx eslintclean on both changed files.Note on the existing env-var test
should delete buildIdentifier if BROWSERSTACK_BUILD_NAME is defined as env varpassed caps with nobuildName, so the!this._buildNamebranch already covered it and the assertion is unchanged. Only its title was corrected, since the env var was never the operative cause. Two tests were added: one locking resolution when abuildNameis present alongside the env var, one asserting the unresolved template is not retained.PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.
🤖 Generated with Claude Code