Merge fix/sdk-7233-wdio-test-file-path-and-binary-update

.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.

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<boolean> {
+        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<string | null> => {
         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<string | null>((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?.()

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<string, unknown> = {
             [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<string, unknown> = {
                 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

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

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 () => {