From afb8ade70a586896e005e5c307c5fb12c2c92558 Mon Sep 17 00:00:00 2001 From: Shivam Kumar Date: Sun, 27 Sep 2026 14:51:41 +0530 Subject: [PATCH 1/5] fix(SDK-4748): honour buildIdentifier precedence in _handleBuildIdentifier v8 port of the same fix on main. _handleBuildIdentifier is byte-identical across the two branches apart from the capabilities type (Capabilities.RemoteCapabilities here, Capabilities.TestrunnerCapabilities on main), so the change is the same. - Skip the identifier only when there is no buildName at all, instead of also whenever BROWSERSTACK_BUILD_NAME happens to be exported. - Resolve BROWSERSTACK_BUILD_IDENTIFIER, then BROWSERSTACK_BUILD_RUN_IDENTIFIER, ahead of the service options / capabilities value. - Sweep any remaining ${ENV_VAR} placeholder against process.env after the ${DATE_TIME} / ${BUILD_NUMBER} substitutions, leaving unset variables literal. Co-Authored-By: Claude Opus 5 (1M context) --- .../sdk-4748-build-identifier-precedence.md | 7 + packages/browserstack-service/src/launcher.ts | 68 ++++++-- .../tests/launcher.test.ts | 145 +++++++++++++++++- 3 files changed, 201 insertions(+), 19 deletions(-) create mode 100644 .changeset/sdk-4748-build-identifier-precedence.md diff --git a/.changeset/sdk-4748-build-identifier-precedence.md b/.changeset/sdk-4748-build-identifier-precedence.md new file mode 100644 index 00000000..7a70ce51 --- /dev/null +++ b/.changeset/sdk-4748-build-identifier-precedence.md @@ -0,0 +1,7 @@ +--- +"@wdio/browserstack-service": patch +--- + +- Fixed a configured `buildIdentifier` being silently dropped whenever `BROWSERSTACK_BUILD_NAME` was exported. The identifier is now only skipped when there is genuinely no build name in the capabilities, so builds keep the suffix the user asked for. +- Added `BROWSERSTACK_BUILD_IDENTIFIER` and `BROWSERSTACK_BUILD_RUN_IDENTIFIER` support. Either env var now sets the build identifier, taking precedence over the value in the service options or capabilities, matching the other BrowserStack SDKs. +- `buildIdentifier` now resolves any `${ENV_VAR}` placeholder against the environment, not just `${BUILD_NUMBER}` and `${DATE_TIME}`. An unset variable is left as-is rather than blanked. diff --git a/packages/browserstack-service/src/launcher.ts b/packages/browserstack-service/src/launcher.ts index 4db272ea..0cf3aae5 100644 --- a/packages/browserstack-service/src/launcher.ts +++ b/packages/browserstack-service/src/launcher.ts @@ -1062,17 +1062,44 @@ export default class BrowserstackLauncherService implements Services.ServiceInst } _handleBuildIdentifier(capabilities?: Capabilities.RemoteCapabilities) { + /** + * buildIdentifier resolution precedence, per the SDK-wide contract + * (CLI args > env vars > config file > script): + * 1. BROWSERSTACK_BUILD_IDENTIFIER - explicit env override + * 2. BROWSERSTACK_BUILD_RUN_IDENTIFIER - per-run signal, typically injected by CI + * 3. service options in wdio.conf.js / bstack:options in the capabilities, both of + * which onPrepare has already folded into this._buildIdentifier + * wdio exposes no CLI arg for buildIdentifier, so tier 1 of the contract is absent here. + * Mirrors browserstack-node-agent computeBuildIdentifier(), browserstack-python-sdk + * ENV_CAPS_TO_CONFIG['buildIdentifier'] and browserstack-csharp-sdk GetBuildIdentifier(). + */ + const envBuildIdentifier = [ + process.env.BROWSERSTACK_BUILD_IDENTIFIER, + process.env.BROWSERSTACK_BUILD_RUN_IDENTIFIER + ].find((value) => value && value.trim()) + if (envBuildIdentifier) { + this._buildIdentifier = envBuildIdentifier.trim() + } + if (!this._buildIdentifier) { return } - if ((!this._buildName || process.env.BROWSERSTACK_BUILD_NAME) && this._buildIdentifier) { + /** + * A buildIdentifier is only meaningful next to a buildName - the dashboard appends it + * to that name. BROWSERSTACK_BUILD_NAME used to force this branch as well, which + * silently discarded every explicitly configured buildIdentifier whenever that env var + * happened to be exported (SDK-4748). This service never reads BROWSERSTACK_BUILD_NAME + * as a buildName source, and unlike the yml-driven SDKs it has no default identifier to + * suppress, so the env var no longer takes part in this decision. + */ + if (!this._buildName) { this._updateCaps(capabilities, 'buildIdentifier') BStackLogger.warn('Skipping buildIdentifier as buildName is not passed.') return } - if (this._buildIdentifier && this._buildIdentifier.includes('${DATE_TIME}')){ + if (this._buildIdentifier.includes('${DATE_TIME}')) { const formattedDate = new Intl.DateTimeFormat('en-GB', { month: 'short', day: '2-digit', @@ -1082,24 +1109,33 @@ export default class BrowserstackLauncherService implements Services.ServiceInst .format(new Date()) .replace(/ |, /g, '-') this._buildIdentifier = this._buildIdentifier.replace('${DATE_TIME}', formattedDate) - this._updateCaps(capabilities, 'buildIdentifier', this._buildIdentifier) - } - - if (!this._buildIdentifier.includes('${BUILD_NUMBER}')) { - return } - const ciInfo = getCiInfo() - if (ciInfo !== null && ciInfo.build_number) { - this._buildIdentifier = this._buildIdentifier.replace('${BUILD_NUMBER}', 'CI '+ ciInfo.build_number) - this._updateCaps(capabilities, 'buildIdentifier', this._buildIdentifier) - } else { - const localBuildNumber = this._getLocalBuildNumber() - if (localBuildNumber) { - this._buildIdentifier = this._buildIdentifier.replace('${BUILD_NUMBER}', localBuildNumber) - this._updateCaps(capabilities, 'buildIdentifier', this._buildIdentifier) + if (this._buildIdentifier.includes('${BUILD_NUMBER}')) { + const ciInfo = getCiInfo() + if (ciInfo !== null && ciInfo.build_number) { + this._buildIdentifier = this._buildIdentifier.replace('${BUILD_NUMBER}', 'CI '+ ciInfo.build_number) + } else { + const localBuildNumber = this._getLocalBuildNumber() + if (localBuildNumber) { + this._buildIdentifier = this._buildIdentifier.replace('${BUILD_NUMBER}', localBuildNumber) + } } } + + /** + * Resolve any remaining ${ENV_VAR} placeholder against process.env, so an identifier + * such as '${CUSTOM_DATE}' behaves the same whatever source it arrived from. An unset + * variable is left as its literal placeholder rather than blanked, so nothing is + * silently lost. + */ + this._buildIdentifier = this._buildIdentifier.replace( + /\$\{([A-Za-z_][A-Za-z0-9_]*)\}/g, + (match, varName) => process.env[varName] ?? match + ) + + this._updateCaps(capabilities, 'buildIdentifier', this._buildIdentifier) + this.browserStackConfig.buildIdentifier = this._buildIdentifier } /** diff --git a/packages/browserstack-service/tests/launcher.test.ts b/packages/browserstack-service/tests/launcher.test.ts index ec6b0157..f722893a 100644 --- a/packages/browserstack-service/tests/launcher.test.ts +++ b/packages/browserstack-service/tests/launcher.test.ts @@ -2,7 +2,7 @@ import fs from 'node:fs' import os from 'node:os' import path from 'node:path' -import { describe, expect, it, vi, beforeEach } from 'vitest' +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' // @ts-expect-error mock feature import { Local, mockStart } from 'browserstack-local' import got from 'got' @@ -1157,6 +1157,12 @@ describe('_handleBuildIdentifier', () => { capabilities: [] } + afterEach(() => { + delete process.env.BROWSERSTACK_BUILD_NAME + delete process.env.BROWSERSTACK_BUILD_IDENTIFIER + delete process.env.BROWSERSTACK_BUILD_RUN_IDENTIFIER + }) + it('should update ${BUILD_NUMBER}', async() => { const caps: any = [{ 'bstack:options': { @@ -1243,7 +1249,43 @@ describe('_handleBuildIdentifier', () => { expect(caps[0]).toMatchObject(updatedcaps[0]) }) - it('should delete buildIdentifier if BROWSERSTACK_BUILD_NAME is defined as env var', async() => { + /** + * SDK-4748: BROWSERSTACK_BUILD_NAME used to delete an explicitly configured + * buildIdentifier outright. It is not a buildName source for this service, so it must + * not influence the identifier at all once a buildName is present in the caps. + */ + it('should keep buildIdentifier when BROWSERSTACK_BUILD_NAME is defined as env var and buildName is in caps', async() => { + process.env.BROWSERSTACK_BUILD_NAME = 'browserstack wdio build' + const caps: any = [{ + 'bstack:options': { + buildName: 'browserstack wdio build', + buildIdentifier: '#${BUILD_NUMBER}' + } + }] + const service = new BrowserstackLauncher(options as any, caps, config) + + vi.spyOn(utils, 'getCiInfo').mockReturnValueOnce(null) + vi.spyOn(service, '_getLocalBuildNumber').mockReturnValueOnce('1') + vi.spyOn(service, '_updateLocalBuildCache').mockImplementation(() => {}) + service._handleBuildIdentifier(caps) + expect(caps[0]['bstack:options']?.buildIdentifier).toEqual('#1') + }) + + it('should keep a literal buildIdentifier untouched when BROWSERSTACK_BUILD_NAME is defined as env var', async() => { + process.env.BROWSERSTACK_BUILD_NAME = 'browserstack wdio build' + const caps: any = [{ + 'bstack:options': { + buildName: 'browserstack wdio build', + buildIdentifier: '2026-09-27_14-35-36' + } + }] + const service = new BrowserstackLauncher(options as any, caps, config) + + service._handleBuildIdentifier(caps) + expect(caps[0]['bstack:options']?.buildIdentifier).toEqual('2026-09-27_14-35-36') + }) + + it('should still delete buildIdentifier if buildName is absent and BROWSERSTACK_BUILD_NAME is defined as env var', async() => { process.env.BROWSERSTACK_BUILD_NAME = 'browserstack wdio build' const caps: any = [{ 'bstack:options': { @@ -1259,7 +1301,104 @@ describe('_handleBuildIdentifier', () => { service._handleBuildIdentifier(caps) expect(caps[0]).toMatchObject(updatedcaps[0]) - delete process.env.BROWSERSTACK_BUILD_NAME + expect(caps[0]['bstack:options']?.buildIdentifier).toBeUndefined() + }) + + it('should prefer BROWSERSTACK_BUILD_RUN_IDENTIFIER over the configured buildIdentifier', async() => { + process.env.BROWSERSTACK_BUILD_NAME = 'browserstack wdio build' + process.env.BROWSERSTACK_BUILD_RUN_IDENTIFIER = 'test_run_20260927_143536' + const caps: any = [{ + 'bstack:options': { + buildName: 'browserstack wdio build', + buildIdentifier: '#${BUILD_NUMBER}' + } + }] + const service = new BrowserstackLauncher(options as any, caps, config) + + service._handleBuildIdentifier(caps) + expect(caps[0]['bstack:options']?.buildIdentifier).toEqual('test_run_20260927_143536') + }) + + it('should prefer BROWSERSTACK_BUILD_IDENTIFIER over BROWSERSTACK_BUILD_RUN_IDENTIFIER', async() => { + process.env.BROWSERSTACK_BUILD_IDENTIFIER = 'explicit-env-identifier' + process.env.BROWSERSTACK_BUILD_RUN_IDENTIFIER = 'per-run-identifier' + const caps: any = [{ + 'bstack:options': { + buildName: 'browserstack wdio build', + buildIdentifier: 'from-caps' + } + }] + const service = new BrowserstackLauncher(options as any, caps, config) + + service._handleBuildIdentifier(caps) + expect(caps[0]['bstack:options']?.buildIdentifier).toEqual('explicit-env-identifier') + }) + + it('should set buildIdentifier from env when none is configured anywhere', async() => { + process.env.BROWSERSTACK_BUILD_RUN_IDENTIFIER = 'per-run-identifier' + const caps: any = [{ + 'bstack:options': { + buildName: 'browserstack wdio build' + } + }] + const service = new BrowserstackLauncher(options as any, caps, config) + + service._handleBuildIdentifier(caps) + expect(caps[0]['bstack:options']?.buildIdentifier).toEqual('per-run-identifier') + }) + + it('should ignore a blank env buildIdentifier and keep the configured one', async() => { + process.env.BROWSERSTACK_BUILD_RUN_IDENTIFIER = ' ' + const caps: any = [{ + 'bstack:options': { + buildName: 'browserstack wdio build', + buildIdentifier: 'from-caps' + } + }] + const service = new BrowserstackLauncher(options as any, caps, config) + + service._handleBuildIdentifier(caps) + expect(caps[0]['bstack:options']?.buildIdentifier).toEqual('from-caps') + }) + + it('should not set buildIdentifier from env when buildName is absent', async() => { + process.env.BROWSERSTACK_BUILD_RUN_IDENTIFIER = 'per-run-identifier' + const caps: any = [{ + 'bstack:options': {} + }] + const service = new BrowserstackLauncher(options as any, caps, config) + + service._handleBuildIdentifier(caps) + expect(caps[0]['bstack:options']?.buildIdentifier).toBeUndefined() + }) + + it('should substitute an arbitrary ${ENV_VAR} placeholder in buildIdentifier', async() => { + process.env.CUSTOM_DATE = '2026-09-27_14-35-36' + const caps: any = [{ + 'bstack:options': { + buildName: 'browserstack wdio build', + buildIdentifier: 'run-${CUSTOM_DATE}' + } + }] + const service = new BrowserstackLauncher(options as any, caps, config) + + service._handleBuildIdentifier(caps) + expect(caps[0]['bstack:options']?.buildIdentifier).toEqual('run-2026-09-27_14-35-36') + delete process.env.CUSTOM_DATE + }) + + it('should leave an unset ${ENV_VAR} placeholder literal', async() => { + delete process.env.NOT_SET_ANYWHERE + const caps: any = [{ + 'bstack:options': { + buildName: 'browserstack wdio build', + buildIdentifier: 'run-${NOT_SET_ANYWHERE}' + } + }] + const service = new BrowserstackLauncher(options as any, caps, config) + + service._handleBuildIdentifier(caps) + expect(caps[0]['bstack:options']?.buildIdentifier).toEqual('run-${NOT_SET_ANYWHERE}') }) it('should not evaluate buildIdentifier if buildIdentifier is not present in the caps', async() => { From c7641d717302dcb59e4bb8050b3855e325452f52 Mon Sep 17 00:00:00 2001 From: Shivam Kumar Date: Sun, 27 Sep 2026 21:12:06 +0530 Subject: [PATCH 2/5] =?UTF-8?q?chore:=20drop=20manual=20changeset=20?= =?UTF-8?q?=E2=80=94=20CI=20generates=20it=20from=20the=20Release=20sectio?= =?UTF-8?q?n?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .changeset/sdk-4748-build-identifier-precedence.md | 7 ------- 1 file changed, 7 deletions(-) delete mode 100644 .changeset/sdk-4748-build-identifier-precedence.md diff --git a/.changeset/sdk-4748-build-identifier-precedence.md b/.changeset/sdk-4748-build-identifier-precedence.md deleted file mode 100644 index 7a70ce51..00000000 --- a/.changeset/sdk-4748-build-identifier-precedence.md +++ /dev/null @@ -1,7 +0,0 @@ ---- -"@wdio/browserstack-service": patch ---- - -- Fixed a configured `buildIdentifier` being silently dropped whenever `BROWSERSTACK_BUILD_NAME` was exported. The identifier is now only skipped when there is genuinely no build name in the capabilities, so builds keep the suffix the user asked for. -- Added `BROWSERSTACK_BUILD_IDENTIFIER` and `BROWSERSTACK_BUILD_RUN_IDENTIFIER` support. Either env var now sets the build identifier, taking precedence over the value in the service options or capabilities, matching the other BrowserStack SDKs. -- `buildIdentifier` now resolves any `${ENV_VAR}` placeholder against the environment, not just `${BUILD_NUMBER}` and `${DATE_TIME}`. An unset variable is left as-is rather than blanked. From 4a49c0783f7321530abf5d0b966e79ae112bd9a4 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun, 27 Sep 2026 15:45:01 +0000 Subject: [PATCH 3/5] chore(changeset): auto-generate from PR template (patch) --- .changeset/pr-237.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/pr-237.md diff --git a/.changeset/pr-237.md b/.changeset/pr-237.md new file mode 100644 index 00000000..9eba345c --- /dev/null +++ b/.changeset/pr-237.md @@ -0,0 +1,7 @@ +--- +"@wdio/browserstack-service": patch +--- + +- Fixed a configured `buildIdentifier` being dropped from the build name when `BROWSERSTACK_BUILD_NAME` was set via environment variable. +- `BROWSERSTACK_BUILD_RUN_IDENTIFIER` and `BROWSERSTACK_BUILD_IDENTIFIER` are now honoured as build-identifier overrides. +- Placeholders such as `${CUSTOM_DATE}` in `buildIdentifier` are now substituted from the environment. From 98500451a8d3bfd6686cff033828bbab3fb25e9f Mon Sep 17 00:00:00 2001 From: Shivam Kumar Date: Mon, 28 Sep 2026 00:04:18 +0530 Subject: [PATCH 4/5] =?UTF-8?q?fix(SDK-4748):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20clear=20stale=20identifier,=20guard=20reserved=20to?= =?UTF-8?q?kens?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical: the !buildName branch stripped buildIdentifier from capabilities but left this._buildIdentifier set. launchTestSession reads that field for the build-start payload's build_identifier, so an env-sourced value was reported to O11y while never being applied to any capability. Pre-existing as a shape, but the new env tier makes it reachable with zero user configuration, since CI commonly injects BROWSERSTACK_BUILD_RUN_IDENTIFIER globally. Both the field and browserStackConfig.buildIdentifier are now cleared alongside the caps update. Warning: the generic ${ENV_VAR} sweep reprocessed ${BUILD_NUMBER}. When neither getCiInfo() nor _getLocalBuildNumber() could resolve it the token was left literal by design, and the sweep then substituted a raw process.env.BUILD_NUMBER - without the 'CI ' prefix every other path applies. getCiInfo() recognises a fixed vendor list that excludes GitHub Actions, and TeamCity exports a bare BUILD_NUMBER, so this was reachable in practice. DATE_TIME and BUILD_NUMBER are now excluded from the sweep. Suggestion: `?? match` guarded only nullish, so an exported-but-empty variable blanked that part of the identifier - contradicting the adjacent comment and inconsistent with the tier-1 check, which treats whitespace-only as absent. Empty and whitespace-only values now leave the literal placeholder. Tests: the "buildName absent" case asserted only the caps side-effect, which the pre-PR code would also have satisfied; it now asserts _buildIdentifier too. Added regression tests for the reserved-token and empty-env cases. afterEach now clears BUILD_NUMBER, so the pre-existing "stays literal" assertions no longer depend on the ambient environment. _handleBuildIdentifier block 17 -> 19 tests. Full suite 1347 passed (3 errors pre-existing in uploadLogsArchive.test.ts, unchanged on a clean tree). Co-Authored-By: Claude Opus 5 (1M context) --- packages/browserstack-service/src/launcher.ts | 35 +++++++++++++-- .../tests/launcher.test.ts | 45 +++++++++++++++++++ 2 files changed, 76 insertions(+), 4 deletions(-) diff --git a/packages/browserstack-service/src/launcher.ts b/packages/browserstack-service/src/launcher.ts index 0cf3aae5..b4af91ab 100644 --- a/packages/browserstack-service/src/launcher.ts +++ b/packages/browserstack-service/src/launcher.ts @@ -72,6 +72,10 @@ type BrowserstackLocal = BrowserstackLocalLauncher.Local & { stop(callback: (err?: Error) => void): void } +// Tokens with dedicated resolution inside _handleBuildIdentifier; the generic ${ENV_VAR} +// sweep must not reprocess them. +const RESERVED_BUILD_IDENTIFIER_TOKENS = new Set(['DATE_TIME', 'BUILD_NUMBER']) + export default class BrowserstackLauncherService implements Services.ServiceInstance { browserstackLocal?: BrowserstackLocal private _buildName?: string @@ -1095,6 +1099,15 @@ export default class BrowserstackLauncherService implements Services.ServiceInst */ if (!this._buildName) { this._updateCaps(capabilities, 'buildIdentifier') + /** + * Clear the in-memory field as well as the capability. launchTestSession reads + * this._buildIdentifier for the build-start payload's build_identifier, so leaving a + * resolved value here would report an identifier that was never applied anywhere + * visible. With the env tier above this is reachable with no user configuration at + * all, since CI commonly injects BROWSERSTACK_BUILD_RUN_IDENTIFIER globally. + */ + this._buildIdentifier = undefined + this.browserStackConfig.buildIdentifier = undefined BStackLogger.warn('Skipping buildIdentifier as buildName is not passed.') return } @@ -1125,13 +1138,27 @@ export default class BrowserstackLauncherService implements Services.ServiceInst /** * Resolve any remaining ${ENV_VAR} placeholder against process.env, so an identifier - * such as '${CUSTOM_DATE}' behaves the same whatever source it arrived from. An unset - * variable is left as its literal placeholder rather than blanked, so nothing is - * silently lost. + * such as '${CUSTOM_DATE}' behaves the same whatever source it arrived from. + * + * DATE_TIME and BUILD_NUMBER are excluded: both are resolved above by dedicated logic, + * and BUILD_NUMBER is deliberately left literal when neither getCiInfo() nor + * _getLocalBuildNumber() can supply one. Without the exclusion this sweep would pick up + * a raw process.env.BUILD_NUMBER on CI vendors getCiInfo() does not recognise, yielding a + * value without the 'CI ' prefix every other resolution path applies. + * + * A variable that is unset, empty or whitespace-only leaves its literal placeholder + * rather than blanking that part of the identifier, so nothing is silently lost. */ this._buildIdentifier = this._buildIdentifier.replace( /\$\{([A-Za-z_][A-Za-z0-9_]*)\}/g, - (match, varName) => process.env[varName] ?? match + (match, varName) => { + if (RESERVED_BUILD_IDENTIFIER_TOKENS.has(varName)) { + return match + } + const envValue = process.env[varName] + + return envValue && envValue.trim() ? envValue : match + } ) this._updateCaps(capabilities, 'buildIdentifier', this._buildIdentifier) diff --git a/packages/browserstack-service/tests/launcher.test.ts b/packages/browserstack-service/tests/launcher.test.ts index f722893a..26e2ca95 100644 --- a/packages/browserstack-service/tests/launcher.test.ts +++ b/packages/browserstack-service/tests/launcher.test.ts @@ -1161,6 +1161,10 @@ describe('_handleBuildIdentifier', () => { delete process.env.BROWSERSTACK_BUILD_NAME delete process.env.BROWSERSTACK_BUILD_IDENTIFIER delete process.env.BROWSERSTACK_BUILD_RUN_IDENTIFIER + // BUILD_NUMBER is not a BrowserStack variable, but the ${BUILD_NUMBER} token is + // resolved from it. Leaving it set would make the "stays literal" assertions depend + // on the ambient environment rather than on the code. + delete process.env.BUILD_NUMBER }) it('should update ${BUILD_NUMBER}', async() => { @@ -1370,6 +1374,47 @@ describe('_handleBuildIdentifier', () => { service._handleBuildIdentifier(caps) expect(caps[0]['bstack:options']?.buildIdentifier).toBeUndefined() + // Also assert the in-memory field: launchTestSession forwards it as the build-start + // payload's build_identifier, so a stale value here would report an identifier that + // was never applied to any capability. + expect((service as any)._buildIdentifier).toBeUndefined() + }) + + it('should leave ${BUILD_NUMBER} literal rather than reading a raw BUILD_NUMBER env var', async() => { + // getCiInfo() recognises a fixed vendor list; on CI it does not know (GitHub Actions, + // TeamCity) a bare BUILD_NUMBER may still be exported. The generic ${ENV_VAR} sweep must + // not pick that up, or the identifier renders without the 'CI ' prefix every other + // resolution path applies. + process.env.BUILD_NUMBER = '394' + vi.spyOn(utils, 'getCiInfo').mockReturnValue(null as any) + const caps: any = [{ + 'bstack:options': { + buildName: 'browserstack wdio build', + buildIdentifier: '#${BUILD_NUMBER}' + } + }] + const service = new BrowserstackLauncher(options as any, caps, config) + vi.spyOn(service, '_getLocalBuildNumber').mockReturnValue(null) + + service._handleBuildIdentifier(caps) + expect(caps[0]['bstack:options']?.buildIdentifier).toEqual('#${BUILD_NUMBER}') + }) + + it('should leave a placeholder literal when its env var is set but empty', async() => { + // `?? match` would only guard nullish, so an exported-but-empty variable would blank + // that part of the identifier instead of leaving the placeholder visible. + process.env.CUSTOM_DATE = ' ' + const caps: any = [{ + 'bstack:options': { + buildName: 'browserstack wdio build', + buildIdentifier: 'run-${CUSTOM_DATE}' + } + }] + const service = new BrowserstackLauncher(options as any, caps, config) + + service._handleBuildIdentifier(caps) + expect(caps[0]['bstack:options']?.buildIdentifier).toEqual('run-${CUSTOM_DATE}') + delete process.env.CUSTOM_DATE }) it('should substitute an arbitrary ${ENV_VAR} placeholder in buildIdentifier', async() => { From 0495376e0915a90d80242fe6fbd15a9858612e2b Mon Sep 17 00:00:00 2001 From: Shivam Kumar Date: Tue, 29 Sep 2026 18:13:29 +0530 Subject: [PATCH 5/5] chore: trim verbose comments in _handleBuildIdentifier and its tests Review feedback on #237: condense the explanatory comments added by this PR. Five blocks in launcher.ts and four in launcher.test.ts, each down to 2-4 lines. The rationale is kept in each - why DATE_TIME/BUILD_NUMBER are excluded from the sweep, and why the in-memory field is cleared alongside the capability - since those are what stop the defects being reintroduced. Only the background narration is dropped. No code change; 45/46 test files pass, the one failure being the pre-existing crash-reporter.test.ts build dependency. Co-Authored-By: Claude Opus 5 (1M context) --- packages/browserstack-service/src/launcher.ts | 47 +++++-------------- .../tests/launcher.test.ts | 19 +++----- 2 files changed, 19 insertions(+), 47 deletions(-) diff --git a/packages/browserstack-service/src/launcher.ts b/packages/browserstack-service/src/launcher.ts index b4af91ab..5d3ee474 100644 --- a/packages/browserstack-service/src/launcher.ts +++ b/packages/browserstack-service/src/launcher.ts @@ -72,8 +72,7 @@ type BrowserstackLocal = BrowserstackLocalLauncher.Local & { stop(callback: (err?: Error) => void): void } -// Tokens with dedicated resolution inside _handleBuildIdentifier; the generic ${ENV_VAR} -// sweep must not reprocess them. +// Resolved by dedicated logic in _handleBuildIdentifier; the generic ${ENV_VAR} sweep skips them. const RESERVED_BUILD_IDENTIFIER_TOKENS = new Set(['DATE_TIME', 'BUILD_NUMBER']) export default class BrowserstackLauncherService implements Services.ServiceInstance { @@ -1067,15 +1066,9 @@ export default class BrowserstackLauncherService implements Services.ServiceInst _handleBuildIdentifier(capabilities?: Capabilities.RemoteCapabilities) { /** - * buildIdentifier resolution precedence, per the SDK-wide contract - * (CLI args > env vars > config file > script): - * 1. BROWSERSTACK_BUILD_IDENTIFIER - explicit env override - * 2. BROWSERSTACK_BUILD_RUN_IDENTIFIER - per-run signal, typically injected by CI - * 3. service options in wdio.conf.js / bstack:options in the capabilities, both of - * which onPrepare has already folded into this._buildIdentifier - * wdio exposes no CLI arg for buildIdentifier, so tier 1 of the contract is absent here. - * Mirrors browserstack-node-agent computeBuildIdentifier(), browserstack-python-sdk - * ENV_CAPS_TO_CONFIG['buildIdentifier'] and browserstack-csharp-sdk GetBuildIdentifier(). + * Precedence per the SDK-wide contract (env > config file): BROWSERSTACK_BUILD_IDENTIFIER, + * then BROWSERSTACK_BUILD_RUN_IDENTIFIER, then the service options / caps value already + * folded into this._buildIdentifier. wdio has no CLI arg, so that tier is absent here. */ const envBuildIdentifier = [ process.env.BROWSERSTACK_BUILD_IDENTIFIER, @@ -1090,22 +1083,14 @@ export default class BrowserstackLauncherService implements Services.ServiceInst } /** - * A buildIdentifier is only meaningful next to a buildName - the dashboard appends it - * to that name. BROWSERSTACK_BUILD_NAME used to force this branch as well, which - * silently discarded every explicitly configured buildIdentifier whenever that env var - * happened to be exported (SDK-4748). This service never reads BROWSERSTACK_BUILD_NAME - * as a buildName source, and unlike the yml-driven SDKs it has no default identifier to - * suppress, so the env var no longer takes part in this decision. + * The dashboard appends the identifier to the buildName, so it needs one. SDK-4748: + * BROWSERSTACK_BUILD_NAME used to force this branch too, discarding any configured + * identifier; this service never reads that var as a buildName source, so it no longer does. */ if (!this._buildName) { this._updateCaps(capabilities, 'buildIdentifier') - /** - * Clear the in-memory field as well as the capability. launchTestSession reads - * this._buildIdentifier for the build-start payload's build_identifier, so leaving a - * resolved value here would report an identifier that was never applied anywhere - * visible. With the env tier above this is reachable with no user configuration at - * all, since CI commonly injects BROWSERSTACK_BUILD_RUN_IDENTIFIER globally. - */ + // Clear the field too, not just the cap: launchTestSession sends it as the + // build-start build_identifier, which would otherwise report a value never applied. this._buildIdentifier = undefined this.browserStackConfig.buildIdentifier = undefined BStackLogger.warn('Skipping buildIdentifier as buildName is not passed.') @@ -1137,17 +1122,9 @@ export default class BrowserstackLauncherService implements Services.ServiceInst } /** - * Resolve any remaining ${ENV_VAR} placeholder against process.env, so an identifier - * such as '${CUSTOM_DATE}' behaves the same whatever source it arrived from. - * - * DATE_TIME and BUILD_NUMBER are excluded: both are resolved above by dedicated logic, - * and BUILD_NUMBER is deliberately left literal when neither getCiInfo() nor - * _getLocalBuildNumber() can supply one. Without the exclusion this sweep would pick up - * a raw process.env.BUILD_NUMBER on CI vendors getCiInfo() does not recognise, yielding a - * value without the 'CI ' prefix every other resolution path applies. - * - * A variable that is unset, empty or whitespace-only leaves its literal placeholder - * rather than blanking that part of the identifier, so nothing is silently lost. + * Resolve remaining ${ENV_VAR} placeholders (e.g. ${CUSTOM_DATE}) against process.env. + * Reserved tokens are skipped - BUILD_NUMBER is deliberately left literal when unresolvable, + * and picking up a raw one here would drop the 'CI ' prefix. Unset/blank stays literal. */ this._buildIdentifier = this._buildIdentifier.replace( /\$\{([A-Za-z_][A-Za-z0-9_]*)\}/g, diff --git a/packages/browserstack-service/tests/launcher.test.ts b/packages/browserstack-service/tests/launcher.test.ts index 26e2ca95..e2ae01e5 100644 --- a/packages/browserstack-service/tests/launcher.test.ts +++ b/packages/browserstack-service/tests/launcher.test.ts @@ -1161,9 +1161,8 @@ describe('_handleBuildIdentifier', () => { delete process.env.BROWSERSTACK_BUILD_NAME delete process.env.BROWSERSTACK_BUILD_IDENTIFIER delete process.env.BROWSERSTACK_BUILD_RUN_IDENTIFIER - // BUILD_NUMBER is not a BrowserStack variable, but the ${BUILD_NUMBER} token is - // resolved from it. Leaving it set would make the "stays literal" assertions depend - // on the ambient environment rather than on the code. + // ${BUILD_NUMBER} resolves from it, so leaving it set makes the + // "stays literal" assertions depend on the ambient environment. delete process.env.BUILD_NUMBER }) @@ -1374,17 +1373,14 @@ describe('_handleBuildIdentifier', () => { service._handleBuildIdentifier(caps) expect(caps[0]['bstack:options']?.buildIdentifier).toBeUndefined() - // Also assert the in-memory field: launchTestSession forwards it as the build-start - // payload's build_identifier, so a stale value here would report an identifier that - // was never applied to any capability. + // launchTestSession forwards this field as build_identifier, so a stale + // value would report an identifier never applied to any capability. expect((service as any)._buildIdentifier).toBeUndefined() }) it('should leave ${BUILD_NUMBER} literal rather than reading a raw BUILD_NUMBER env var', async() => { - // getCiInfo() recognises a fixed vendor list; on CI it does not know (GitHub Actions, - // TeamCity) a bare BUILD_NUMBER may still be exported. The generic ${ENV_VAR} sweep must - // not pick that up, or the identifier renders without the 'CI ' prefix every other - // resolution path applies. + // getCiInfo() knows a fixed vendor list; GitHub Actions / TeamCity still export a bare + // BUILD_NUMBER. The sweep must not pick it up, or the 'CI ' prefix is lost. process.env.BUILD_NUMBER = '394' vi.spyOn(utils, 'getCiInfo').mockReturnValue(null as any) const caps: any = [{ @@ -1401,8 +1397,7 @@ describe('_handleBuildIdentifier', () => { }) it('should leave a placeholder literal when its env var is set but empty', async() => { - // `?? match` would only guard nullish, so an exported-but-empty variable would blank - // that part of the identifier instead of leaving the placeholder visible. + // `?? match` guards only nullish, so an exported-but-empty var would blank the identifier. process.env.CUSTOM_DATE = ' ' const caps: any = [{ 'bstack:options': {