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. 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() + }) +})