From 99ec2b18659fb9131ff0a0e9a66b198569729930 Mon Sep 17 00:00:00 2001 From: Bhargavi-BS Date: Thu, 13 Aug 2026 09:35:00 +0530 Subject: [PATCH 1/2] fix(cli): send absolute test_file_path and make binary update version-aware (SDK-7233) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WebdriverIO-mocha test results never reached Test Reporting when the project was not a git checkout. test_file_path was gated on a resolvable git root, so outside a git checkout it went out as undefined. The binary dereferences that field in MochaModule.makeFileDetails (path.relative) and throws before the event is uploaded, so every TestRunStarted/TestRunFinished was dropped while CBTSessionCreated still went through — sessions appeared on the dashboard but the test rows never resolved. Send the absolute spec path instead. The binary re-bases it itself against pathProject and versionControlInfo.root (its local is named absoluteTestFilePath), so a pre-relativised value also produced a wrong file_name. The binary-side guard for this has shipped since 1.22.x, but affected users never received it: downloadLatestBinary short-circuits when a binary merely exists on disk, never comparing versions. It is only ever reached after the server reports an update, so the binary it finds is stale by construction and the update is skipped on every run — pinning a machine to the first binary it ever downloaded. Compare against the target version instead, preferring the server-reported one so a custom BROWSERSTACK_BINARY_URL keeps its peer-race handling. Also fixes two latent faults on the same path: a fetch failure left the download promise pending forever, and the write stream's error listener was registered after the first await, so an early failure surfaced as an uncaught exception in the user's test process. --- .../browserstack-service/src/cli/cliUtils.ts | 90 ++++++++++++--- .../cli/frameworks/wdioMochaTestFramework.ts | 28 +++-- .../tests/cli/cliUtils.staleBinary.test.ts | 104 ++++++++++++++++++ .../tests/cli/cliUtils.test.ts | 6 +- .../wdioMochaTestFramework.filePath.test.ts | 78 +++++++++++++ 5 files changed, 282 insertions(+), 24 deletions(-) create mode 100644 packages/browserstack-service/tests/cli/cliUtils.staleBinary.test.ts create mode 100644 packages/browserstack-service/tests/cli/wdioMochaTestFramework.filePath.test.ts diff --git a/packages/browserstack-service/src/cli/cliUtils.ts b/packages/browserstack-service/src/cli/cliUtils.ts index 4d68079..03b335f 100644 --- a/packages/browserstack-service/src/cli/cliUtils.ts +++ b/packages/browserstack-service/src/cli/cliUtils.ts @@ -270,6 +270,7 @@ export class CLIUtils { const finalBinaryPath = await this.downloadLatestBinary( nestedKeyValue(response, ['url']), cliDir, + nestedKeyValue(response, ['updated_cli_version']), ) PerformanceTester.end(PerformanceEvents.SDK_CLI_CHECK_UPDATE) return finalBinaryPath @@ -456,11 +457,46 @@ export class CLIUtils { }) } + /** + * Version encoded in a binary download URL, e.g. + * `.../binary-macos-arm64-1.48.0.zip` -> `1.48.0`. Null when the URL does + * not carry one (custom BROWSERSTACK_BINARY_URL). + */ + static getVersionFromBinaryUrl(binDownloadUrl: string): string | null { + return /-(\d+\.\d+\.\d+)\.zip(?:\?|$)/.exec(binDownloadUrl || '')?.[1] ?? null + } + + /** + * Whether the binary on disk is already the version we were asked to fetch. + * A peer worker winning the download race leaves the *target* version here; + * a merely-pre-existing binary is stale. Only the former may short-circuit + * the download — see `downloadLatestBinary`. + */ + static async isBinaryAtVersion( + binaryPath: string, + expectedVersion: string | null, + ): Promise { + if (!expectedVersion) { + return false + } + try { + const actual = await CLIUtils.runShellCommand(`${binaryPath} version`) + return actual.trim() === expectedVersion + } catch { + return false + } + } + static downloadLatestBinary = async ( binDownloadUrl: string, cliDir: string, + expectedVersion?: string | null, ): Promise => { const lockPath = path.join(cliDir, 'download.lock') + // Prefer the version the server reported; the URL is only a fallback and + // carries no version when BROWSERSTACK_BINARY_URL overrides it. + const targetVersion = + expectedVersion || CLIUtils.getVersionFromBinaryUrl(binDownloadUrl) const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)) @@ -534,16 +570,20 @@ export class CLIUtils { continue } } - // Check if existing binary appeared while waiting + // Check if the target binary appeared while waiting const existingBinary = CLIUtils.getExistingCliPath(cliDir) if ( existingBinary && fs.existsSync(existingBinary) && - fs.statSync(existingBinary).size > 0 + fs.statSync(existingBinary).size > 0 && + (await CLIUtils.isBinaryAtVersion( + existingBinary, + targetVersion, + )) ) { logger.debug( - `Binary appeared while waiting for lock: ${existingBinary}`, + `Binary v${targetVersion} appeared while waiting for lock: ${existingBinary}`, ) return { alreadyExists: existingBinary } } @@ -631,15 +671,19 @@ export class CLIUtils { releaseLock = lockResult - // Re-check after acquiring lock + // Re-check after acquiring lock. Only a binary already at the target + // version means a peer won the race; any other binary here is the + // stale one we were sent to replace, so it must not short-circuit + // the download. const existingBinary = CLIUtils.getExistingCliPath(cliDir) if ( existingBinary && fs.existsSync(existingBinary) && - fs.statSync(existingBinary).size > 0 + fs.statSync(existingBinary).size > 0 && + (await CLIUtils.isBinaryAtVersion(existingBinary, targetVersion)) ) { logger.debug( - `Binary already exists after acquiring lock: ${existingBinary}`, + `Binary already at v${targetVersion} after acquiring lock: ${existingBinary}`, ) endDownload() releaseLock() @@ -655,6 +699,19 @@ export class CLIUtils { const downloadedFileStream = fs.createWriteStream(zipFilePath) return new Promise((resolve, reject) => { + // Registered before the first await: createWriteStream opens + // asynchronously, so an error can land before processDownload + // gets far enough to attach its own handler — with no listener + // that becomes an uncaught exception in the user's test process. + downloadedFileStream.on('error', function (err: Error) { + logger.error( + `Got Error while downloading cli binary file: ${err}`, + ) + endDownload(false, util.format(err)) + releaseLock?.() + reject(err) + }) + const processDownload = async () => { const abortController = new AbortController() const timeout = setTimeout( @@ -673,15 +730,6 @@ export class CLIUtils { throw new Error('No response body received') } - downloadedFileStream.on('error', function (err: Error) { - logger.error( - `Got Error while downloading cli binary file: ${err}`, - ) - endDownload(false, util.format(err)) - releaseLock?.() - reject(err) - }) - try { const arrayBuffer = await response.arrayBuffer() const nodeStream = Readable.from([ @@ -716,7 +764,17 @@ export class CLIUtils { } } - processDownload() + // A rejection before the stream handlers are wired (e.g. fetch + // itself failing) would otherwise leave this promise pending + // forever and hang the launcher. + processDownload().catch((err: Error) => { + // Nothing is piped into the stream on this path, so close it + // explicitly — otherwise the fd and the temp zip both leak. + downloadedFileStream.destroy() + endDownload(false, util.format(err)) + releaseLock?.() + reject(err) + }) }) } catch (err) { releaseLock?.() diff --git a/packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts b/packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts index 76e4b88..9065858 100644 --- a/packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts +++ b/packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts @@ -10,9 +10,27 @@ import TrackedInstance from '../instances/trackedInstance.js' import { TestFrameworkConstants } from './constants/testFrameworkConstants.js' import { BStackLogger as logger } from '../cliLogger.js' import type { Frameworks } from '@wdio/types' -import { getGitMetaData, getMochaTestHierarchy, getUniqueIdentifier, isUndefined, removeAnsiColors } from '../../util.js' +import { getMochaTestHierarchy, getUniqueIdentifier, isUndefined, removeAnsiColors } from '../../util.js' import { TEST_ANALYTICS_ID } from '../../constants.js' +/** + * File-path pair sent with every test/hook event. + * + * `test_file_path` must be the ABSOLUTE spec path: the binary re-bases it + * itself, as `path.relative(session.pathProject, v)` for `file_name` and + * `path.relative(versionControlInfo.root, v)` for `vc_filepath` (its local is + * literally named `absoluteTestFilePath`). Sending a pre-relativised path made + * both come out wrong; sending `undefined` — which is what happened whenever + * there was no resolvable git root — threw inside the binary and dropped the + * event entirely (SDK-7233). + */ +const resolveTestFilePaths = (filename: string | undefined) => ({ + [TestFrameworkConstants.KEY_TEST_FILE_PATH]: filename, + [TestFrameworkConstants.KEY_TEST_LOCATION]: filename + ? path.relative(process.cwd(), filename) + : undefined, +}) + export default class WdioMochaTestFramework extends TestFramework { static KEY_HOOK_LAST_STARTED = 'test_hook_last_started' static KEY_HOOK_LAST_FINISHED = 'test_hook_last_finished' @@ -197,15 +215,13 @@ export default class WdioMochaTestFramework extends TestFramework { async getTestData(instance: TestFrameworkInstance, test: Frameworks.Test) { const framework = TestFramework.getState(instance, TestFrameworkConstants.KEY_TEST_FRAMEWORK_NAME) const fullTitle = getUniqueIdentifier(test, framework) - const gitConfig = await getGitMetaData() const filename = test.file // || this._suiteFile const testData: Record = { [TestFrameworkConstants.KEY_TEST_ID]: getUniqueIdentifier(test, framework), [TestFrameworkConstants.KEY_TEST_NAME]: test.title || test.description, [TestFrameworkConstants.KEY_TEST_CODE]: test.body || '', - [TestFrameworkConstants.KEY_TEST_FILE_PATH]: (gitConfig?.root && filename) ? path.relative(gitConfig.root, filename) : undefined, - [TestFrameworkConstants.KEY_TEST_LOCATION]: filename ? path.relative(process.cwd(), filename) : undefined, + ...resolveTestFilePaths(filename), [TestFrameworkConstants.KEY_TEST_SCOPE]: fullTitle, [TestFrameworkConstants.KEY_TEST_SCOPES]: getMochaTestHierarchy(test), } @@ -389,7 +405,6 @@ export default class WdioMochaTestFramework extends TestFramework { } if (hookState === HookState.PRE) { - const gitConfig = await getGitMetaData() const filename = test.file const hook: Record = { key, @@ -398,8 +413,7 @@ export default class WdioMochaTestFramework extends TestFramework { [TestFrameworkConstants.KEY_EVENT_STARTED_AT]: new Date().toISOString(), [TestFrameworkConstants.KEY_HOOK_LOGS]: [], [TestFrameworkConstants.KEY_HOOK_NAME]: test.title || test.description, - [TestFrameworkConstants.KEY_TEST_FILE_PATH]: (gitConfig?.root && filename) ? path.relative(gitConfig.root, filename) : undefined, - [TestFrameworkConstants.KEY_TEST_LOCATION]: filename ? path.relative(process.cwd(), filename) : undefined, + ...resolveTestFilePaths(filename), } hooksStarted.get(key)?.push(hook) updates[WdioMochaTestFramework.KEY_HOOK_LAST_STARTED] = key diff --git a/packages/browserstack-service/tests/cli/cliUtils.staleBinary.test.ts b/packages/browserstack-service/tests/cli/cliUtils.staleBinary.test.ts new file mode 100644 index 0000000..c9e0bb5 --- /dev/null +++ b/packages/browserstack-service/tests/cli/cliUtils.staleBinary.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' +import fs from 'node:fs' +import path from 'node:path' +import os from 'node:os' +import * as bstackLogger from '../../src/bstackLogger.js' + +import { CLIUtils } from '../../src/cli/cliUtils.js' + +vi.spyOn(bstackLogger.BStackLogger, 'logToFile').mockImplementation(() => {}) + +const DOWNLOAD_URL = 'https://sdk-assets.browserstack.com/binary-macos-arm64-1.48.0.zip' + +describe('SDK-7233 — binary update must not be skipped for a stale binary', () => { + let cliDir: string + + beforeEach(() => { + cliDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sdk7233-cli-')) + }) + + afterEach(() => { + vi.restoreAllMocks() + fs.rmSync(cliDir, { recursive: true, force: true }) + }) + + it('parses the target version out of the download URL', () => { + expect(CLIUtils.getVersionFromBinaryUrl(DOWNLOAD_URL)).toBe('1.48.0') + expect(CLIUtils.getVersionFromBinaryUrl('https://x/custom-build.zip')).toBeNull() + }) + + describe('isBinaryAtVersion', () => { + it('is true only when the binary reports exactly the expected version', async () => { + vi.spyOn(CLIUtils, 'runShellCommand').mockResolvedValue('1.48.0\n') + await expect(CLIUtils.isBinaryAtVersion('/bin/bs', '1.48.0')).resolves.toBe(true) + await expect(CLIUtils.isBinaryAtVersion('/bin/bs', '1.22.1')).resolves.toBe(false) + }) + + it('fails open (false ⇒ download) on a busy or erroring binary', async () => { + const shell = vi.spyOn(CLIUtils, 'runShellCommand') + shell.mockResolvedValue('SHELL_EXECUTE_ERROR') + await expect(CLIUtils.isBinaryAtVersion('/bin/bs', '1.48.0')).resolves.toBe(false) + shell.mockResolvedValue('text file busy') + await expect(CLIUtils.isBinaryAtVersion('/bin/bs', '1.48.0')).resolves.toBe(false) + shell.mockRejectedValue(new Error('spawn failed')) + await expect(CLIUtils.isBinaryAtVersion('/bin/bs', '1.48.0')).resolves.toBe(false) + }) + + it('returns false when no expected version is known', async () => { + const shell = vi.spyOn(CLIUtils, 'runShellCommand') + await expect(CLIUtils.isBinaryAtVersion('/bin/bs', null)).resolves.toBe(false) + expect(shell).not.toHaveBeenCalled() + }) + }) + + it('uses the server-reported version when the URL carries none (custom BROWSERSTACK_BINARY_URL)', async () => { + // Without this, a custom binary URL would make targetVersion null, which + // disables the peer-race short-circuit and re-downloads on every run. + const peerDownloaded = path.join(cliDir, 'binary-macos-arm64') + fs.writeFileSync(peerDownloaded, 'fresh-binary-v1.48.0') + const atVersion = vi.spyOn(CLIUtils, 'isBinaryAtVersion').mockResolvedValue(true) + const fetchSpy = vi.spyOn(globalThis, 'fetch') + + const returned = await CLIUtils.downloadLatestBinary( + 'https://mirror.internal/custom-build.zip', + cliDir, + '1.48.0', + ) + + expect(atVersion).toHaveBeenCalledWith(peerDownloaded, '1.48.0') + expect(returned).toBe(peerDownloaded) + expect(fetchSpy).not.toHaveBeenCalled() + }) + + it('downloads the update when the on-disk binary is a stale version', async () => { + // downloadLatestBinary is only reached once the server has said an update + // is required, so a pre-existing binary here is stale by construction. + fs.writeFileSync(path.join(cliDir, 'binary-macos-arm64'), 'stale-binary-v1.22.1') + vi.spyOn(CLIUtils, 'isBinaryAtVersion').mockResolvedValue(false) + const fetchSpy = vi + .spyOn(globalThis, 'fetch') + .mockRejectedValue(new Error('network disabled in test')) + + // Now rejects instead of hanging: processDownload() propagates the + // fetch failure to the caller. + await expect( + CLIUtils.downloadLatestBinary(DOWNLOAD_URL, cliDir), + ).rejects.toThrow('network disabled in test') + expect(fetchSpy).toHaveBeenCalledWith(DOWNLOAD_URL, expect.anything()) + // let the aborted write stream finish tearing down before afterEach + // removes the temp dir out from under its pending open() + await new Promise((r) => setTimeout(r, 50)) + }) + + it('skips the download when a peer already fetched the target version', async () => { + const peerDownloaded = path.join(cliDir, 'binary-macos-arm64') + fs.writeFileSync(peerDownloaded, 'fresh-binary-v1.48.0') + vi.spyOn(CLIUtils, 'isBinaryAtVersion').mockResolvedValue(true) + const fetchSpy = vi.spyOn(globalThis, 'fetch') + + const returned = await CLIUtils.downloadLatestBinary(DOWNLOAD_URL, cliDir) + + expect(returned).toBe(peerDownloaded) + expect(fetchSpy).not.toHaveBeenCalled() + }) +}) diff --git a/packages/browserstack-service/tests/cli/cliUtils.test.ts b/packages/browserstack-service/tests/cli/cliUtils.test.ts index 77f1605..aa82167 100644 --- a/packages/browserstack-service/tests/cli/cliUtils.test.ts +++ b/packages/browserstack-service/tests/cli/cliUtils.test.ts @@ -427,7 +427,11 @@ describe('CLIUtils', () => { const result = await CLIUtils.checkAndUpdateCli(mockExistingPath, mockCliDir, mockConfig) expect(result).toBe(mockNewBinaryPath) - expect(CLIUtils.downloadLatestBinary).toHaveBeenCalledWith(mockResponse.url, mockCliDir) + expect(CLIUtils.downloadLatestBinary).toHaveBeenCalledWith( + mockResponse.url, + mockCliDir, + mockResponse.updated_cli_version, + ) }) it('uses SHELL_EXECUTE_ERROR when runShellCommand fails', async () => { diff --git a/packages/browserstack-service/tests/cli/wdioMochaTestFramework.filePath.test.ts b/packages/browserstack-service/tests/cli/wdioMochaTestFramework.filePath.test.ts new file mode 100644 index 0000000..6f89454 --- /dev/null +++ b/packages/browserstack-service/tests/cli/wdioMochaTestFramework.filePath.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' +import path from 'node:path' +import * as bstackLogger from '../../src/bstackLogger.js' +import * as util from '../../src/util.js' + +import WdioMochaTestFramework from '../../src/cli/frameworks/wdioMochaTestFramework.js' +import TestFramework from '../../src/cli/frameworks/testFramework.js' +import { TestFrameworkConstants } from '../../src/cli/frameworks/constants/testFrameworkConstants.js' + +vi.spyOn(bstackLogger.BStackLogger, 'logToFile').mockImplementation(() => {}) + +// Deliberately NOT under process.cwd(), so an absolute value and a cwd-relative +// value cannot coincide and mask a regression. +const SPEC = '/Users/someone/projects/arenaclub/test/specs/smoke/homePage.test.ts' + +describe('SDK-7233 — test_file_path must be the absolute spec path', () => { + beforeEach(() => { + vi.spyOn(TestFramework, 'getState').mockReturnValue('WebdriverIO-mocha') + vi.spyOn(util, 'getUniqueIdentifier').mockReturnValue('Verify the homepage elements') + vi.spyOn(util, 'getMochaTestHierarchy').mockReturnValue([]) + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + // NB: not a default parameter — passing `undefined` explicitly must stay undefined. + const getTestData = (...args: [file?: string | undefined]) => + getTestDataWith(args.length ? args[0] : SPEC) + + const getTestDataWith = (file: string | undefined) => + // @ts-expect-error — partial TestFrameworkInstance / Frameworks.Test are enough here + WdioMochaTestFramework.prototype.getTestData.call(WdioMochaTestFramework.prototype, {}, { + file, + title: 'Verify the homepage elements', + body: 'async () => {}', + }) + + it('sends the absolute path, so the binary can re-base it itself', async () => { + // Previously this field was gated on a git root being resolvable; the + // customer's project had none ("Unable to find a Git directory"), so it + // went out as `undefined`, threw inside the binary, and dropped every + // TestRunStarted/TestRunFinished event. + const data = await getTestData() + + expect(data[TestFrameworkConstants.KEY_TEST_FILE_PATH]).toBe(SPEC) + expect(path.isAbsolute(data[TestFrameworkConstants.KEY_TEST_FILE_PATH] as string)).toBe(true) + }) + + it('does not consult git metadata for the file path at all', async () => { + // Re-basing is the binary's job (it holds pathProject and + // versionControlInfo.root). The emitter must not pre-relativise, and no + // longer pays for a git lookup per test. + const git = vi.spyOn(util, 'getGitMetaData') + + await getTestData() + + expect(git).not.toHaveBeenCalled() + }) + + it('keeps test_location cwd-relative and distinct from test_file_path', async () => { + const data = await getTestData() + + expect(data[TestFrameworkConstants.KEY_TEST_LOCATION]).toBe(path.relative(process.cwd(), SPEC)) + expect(data[TestFrameworkConstants.KEY_TEST_LOCATION]).not.toBe( + data[TestFrameworkConstants.KEY_TEST_FILE_PATH], + ) + }) + + it('leaves both fields undefined when the spec filename is unknown', async () => { + // Documented residual hole: a construct with no `test.file` still yields + // undefined. Not reachable for normal Mocha specs, which always carry one. + const data = await getTestData(undefined) + + expect(data[TestFrameworkConstants.KEY_TEST_FILE_PATH]).toBeUndefined() + expect(data[TestFrameworkConstants.KEY_TEST_LOCATION]).toBeUndefined() + }) +}) From 52f0cf4127a93292bb8c8010430f90e0f0c97999 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 13 Aug 2026 04:06:32 +0000 Subject: [PATCH 2/2] chore(changeset): auto-generate from PR template (patch) --- .changeset/pr-135.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/pr-135.md diff --git a/.changeset/pr-135.md b/.changeset/pr-135.md new file mode 100644 index 0000000..7a7bde0 --- /dev/null +++ b/.changeset/pr-135.md @@ -0,0 +1,6 @@ +--- +"@wdio/browserstack-service": patch +--- + +- Fixed test results not appearing in Test Reporting for WebdriverIO + Mocha when the project is not a git repository. +- Fixed the BrowserStack binary not updating once a copy was already present, which could leave a machine on an old binary indefinitely.