diff --git a/.changeset/pr-99.md b/.changeset/pr-99.md new file mode 100644 index 0000000..a1e933b --- /dev/null +++ b/.changeset/pr-99.md @@ -0,0 +1,5 @@ +--- +"@wdio/browserstack-service": patch +--- + +- Fixed WebdriverIO test results sometimes not appearing (builds staying "in progress") when the build-completion signal failed or the test runner was interrupted. diff --git a/packages/browserstack-service/src/launcher.ts b/packages/browserstack-service/src/launcher.ts index e74e98d..00fb918 100644 --- a/packages/browserstack-service/src/launcher.ts +++ b/packages/browserstack-service/src/launcher.ts @@ -629,8 +629,12 @@ export default class BrowserstackLauncherService implements Services.ServiceInst // SDK-4671: before stopping the build, synthesize TestRunFinished for any // test runs whose worker died mid-test, else they stay 'in progress' on TRA. await finalizeOrphanedRuns() + // SDK-7061: capture the stop result so we only mark the build stopped when it + // actually succeeded. A failed stop must leave buildStopped=false so the + // process-exit cleanup re-stop path can still close the build. + let stopResult: { status?: string } | undefined try { - await (isCLIEnabled ? BrowserstackCLI.getInstance().stop() : stopBuildUpstream()) + stopResult = await (isCLIEnabled ? BrowserstackCLI.getInstance().stop() : stopBuildUpstream()) as { status?: string } | undefined PerformanceTester.end(PERFORMANCE_SDK_EVENTS.FRAMEWORK_EVENTS.STOP) } catch (err) { BStackLogger.error(`Error while stopping CLI ${err}`) @@ -639,7 +643,11 @@ export default class BrowserstackLauncherService implements Services.ServiceInst if (process.env[BROWSERSTACK_OBSERVABILITY] && process.env[BROWSERSTACK_TESTHUB_UUID]) { console.log(`\nVisit https://automation.browserstack.com/builds/${process.env[BROWSERSTACK_TESTHUB_UUID]} to view build report, insights, and many more debugging information all at one place!\n`) } - this.browserStackConfig.testObservability.buildStopped = true + // CLI path manages its own build lifecycle; for the direct-HTTP path only + // mark stopped when stopBuildUpstream returned success (SDK-7061). + if (isCLIEnabled || stopResult?.status === 'success') { + this.browserStackConfig.testObservability.buildStopped = true + } await PerformanceTester.stopAndGenerate('performance-launcher.html') if (process.env[PERF_MEASUREMENT_ENV]) { diff --git a/packages/browserstack-service/src/util.ts b/packages/browserstack-service/src/util.ts index f8d0b82..651487a 100644 --- a/packages/browserstack-service/src/util.ts +++ b/packages/browserstack-service/src/util.ts @@ -743,30 +743,46 @@ export const stopBuildUpstream = PerformanceTester.measureWrapper(PERFORMANCE_SD data.finished_metadata = [{ reason: 'user_killed', signal: killSignal }] } - try { - const url = `${APIUtils.DATA_ENDPOINT}/api/v1/builds/${process.env[BROWSERSTACK_TESTHUB_UUID]}/stop` - const response = await fetch(url, { - method: 'PUT', - headers: { - ...DEFAULT_REQUEST_CONFIG.headers, - 'Authorization': `Bearer ${process.env[BROWSERSTACK_TESTHUB_JWT]}` - }, - body: JSON.stringify(data) - }) - BStackLogger.debug(`[STOP_BUILD] Success response: ${await response.text()}`) - stopBuildUsage.success() - return { - status: 'success', - message: '' - } - } catch (error: unknown) { - stopBuildUsage.failed(error) - BStackLogger.debug(`[STOP_BUILD] Failed. Error: ${error}`) - return { - status: 'error', - message: (error as Error).message + const url = `${APIUtils.DATA_ENDPOINT}/api/v1/builds/${process.env[BROWSERSTACK_TESTHUB_UUID]}/stop` + // SDK-7061: the build-stop PUT is the signal that closes the TRA build. A single + // best-effort request means a transient failure/timeout leaves the build running + // until the ~60-min server-side inactivity timeout. Retry with backoff and treat a + // non-2xx response as a failure so the build reliably reaches a terminal state. + const maxAttempts = 3 + let lastError: unknown + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + try { + const response = await fetch(url, { + method: 'PUT', + headers: { + ...DEFAULT_REQUEST_CONFIG.headers, + 'Authorization': `Bearer ${process.env[BROWSERSTACK_TESTHUB_JWT]}` + }, + body: JSON.stringify(data) + }) + if (!response.ok) { + throw new Error(`HTTP ${response.status} ${response.statusText}`) + } + BStackLogger.debug(`[STOP_BUILD] Success response: ${await response.text()}`) + stopBuildUsage.success() + return { + status: 'success', + message: '' + } + } catch (error: unknown) { + lastError = error + BStackLogger.debug(`[STOP_BUILD] Attempt ${attempt}/${maxAttempts} failed. Error: ${error}`) + if (attempt < maxAttempts) { + await new Promise((resolve) => setTimeout(resolve, 500 * attempt)) + } } } + stopBuildUsage.failed(lastError) + BStackLogger.debug(`[STOP_BUILD] Failed after ${maxAttempts} attempts. Error: ${lastError}`) + return { + status: 'error', + message: (lastError as Error)?.message ?? 'stop build failed' + } })) export function getCiInfo () { diff --git a/packages/browserstack-service/tests/launcher.test.ts b/packages/browserstack-service/tests/launcher.test.ts index cad0b94..292cae4 100644 --- a/packages/browserstack-service/tests/launcher.test.ts +++ b/packages/browserstack-service/tests/launcher.test.ts @@ -17,6 +17,7 @@ import * as FunnelInstrumentation from '../src/instrumentation/funnelInstrumenta import { RERUN_TESTS_ENV, BROWSERSTACK_TESTHUB_UUID, RERUN_ENV } from '../src/constants.js' import * as thUtils from '../src/testHub/utils.js' import TestOpsConfig from '../src/testOps/testOpsConfig.js' +import { BrowserstackCLI } from '../src/cli/index.js' vi.mock('@wdio/logger', () => import(path.join(process.cwd(), '__mocks__', '@wdio/logger'))) vi.mock('browserstack-local') @@ -728,6 +729,29 @@ describe('onComplete', () => { await service.onComplete() expect(stopBuildUpstreamSpy).toHaveBeenCalledTimes(1) }) + + // SDK-7061: on the direct-HTTP path buildStopped must reflect whether the stop PUT + // actually succeeded, so the process-exit cleanup re-stop can still close the build + // when the in-hook stop failed. + it('marks buildStopped true when the direct-HTTP stop succeeds', async () => { + vi.spyOn(BrowserstackCLI.getInstance(), 'isRunning').mockReturnValue(false) + vi.spyOn(utils, 'stopBuildUpstream').mockResolvedValue({ status: 'success', message: '' } as any) + + const service = new BrowserstackLauncher({} as any, [{}] as any, {} as any) + ;(service as any).browserStackConfig.testObservability.buildStopped = false + await service.onComplete() + expect((service as any).browserStackConfig.testObservability.buildStopped).toBe(true) + }) + + it('leaves buildStopped false when the direct-HTTP stop fails', async () => { + vi.spyOn(BrowserstackCLI.getInstance(), 'isRunning').mockReturnValue(false) + vi.spyOn(utils, 'stopBuildUpstream').mockResolvedValue({ status: 'error', message: 'boom' } as any) + + const service = new BrowserstackLauncher({} as any, [{}] as any, {} as any) + ;(service as any).browserStackConfig.testObservability.buildStopped = false + await service.onComplete() + expect((service as any).browserStackConfig.testObservability.buildStopped).toBe(false) + }) }) describe('constructor', () => { diff --git a/packages/browserstack-service/tests/util.test.ts b/packages/browserstack-service/tests/util.test.ts index 1260156..af292e2 100644 --- a/packages/browserstack-service/tests/util.test.ts +++ b/packages/browserstack-service/tests/util.test.ts @@ -824,6 +824,17 @@ describe('getScenarioExamples', () => { }) describe('stopBuildUpstream', () => { + // Fake setTimeout (in addition to the module-level Date) so the SDK-7061 retry + // backoff can be advanced deterministically without real wall-clock delay. + // performance is intentionally NOT faked (negative perf.now() under the 2020 + // system clock breaks perf_hooks on Node 18) — mirrors the module-level config. + // useRealTimers() first is required: re-calling useFakeTimers over an already-active + // fake clock does NOT widen `toFake`, so setTimeout would otherwise stay real. + const useBackoffTimers = () => { + vi.useRealTimers() + vi.useFakeTimers({ toFake: ['Date', 'setTimeout', 'clearTimeout'] }).setSystemTime(new Date('2020-01-01')) + } + it('return error if completed but jwt token not present', async () => { process.env[TESTOPS_BUILD_COMPLETED_ENV] = 'true' delete process.env[BROWSERSTACK_TESTHUB_JWT] @@ -839,10 +850,13 @@ describe('stopBuildUpstream', () => { process.env[TESTOPS_BUILD_COMPLETED_ENV] = 'true' process.env[BROWSERSTACK_TESTHUB_JWT] = 'jwt' + // SDK-7061: the stop path now checks response.ok and reads response.text(), + // so the mock must yield an ok (2xx) response with a readable body. vi.mocked(fetch).mockReturnValueOnce(Promise.resolve(Response.json({}))) const result: any = await stopBuildUpstream() expect(vi.mocked(fetch).mock.calls[0][1]?.method).toEqual('PUT') + expect(vi.mocked(fetch).mock.calls.length).toEqual(1) expect(result.status).toEqual('success') }) @@ -850,15 +864,71 @@ describe('stopBuildUpstream', () => { process.env[TESTOPS_BUILD_COMPLETED_ENV] = 'true' process.env[BROWSERSTACK_TESTHUB_JWT] = 'jwt' - vi.mocked(fetch).mockReturnValueOnce(Promise.reject(Response.json({}))) + // SDK-7061: the stop PUT is now retried up to 3x with a 500ms*attempt backoff. + // Reject every attempt so the loop exhausts and returns an error. Fake timers keep + // the ~1.5s of cumulative backoff out of wall-clock and make the run deterministic. + useBackoffTimers() + vi.mocked(fetch) + .mockImplementationOnce(() => Promise.reject(new Error('network'))) + .mockImplementationOnce(() => Promise.reject(new Error('network'))) + .mockImplementationOnce(() => Promise.reject(new Error('network'))) + + const promise = stopBuildUpstream() + await vi.advanceTimersByTimeAsync(2000) + const result: any = await promise - const result: any = await stopBuildUpstream() expect(vi.mocked(fetch).mock.calls[0][1]?.method).toEqual('PUT') + expect(vi.mocked(fetch).mock.calls.length).toEqual(3) + expect(result.status).toEqual('error') + }) + + it('retries on a non-2xx response and returns error when all attempts fail', async () => { + process.env[TESTOPS_BUILD_COMPLETED_ENV] = 'true' + process.env[BROWSERSTACK_TESTHUB_JWT] = 'jwt' + + // SDK-7061: a non-2xx response is a failure and must be retried (previously a + // best-effort PUT logged any response as success). + useBackoffTimers() + vi.mocked(fetch) + .mockReturnValueOnce(Promise.resolve(Response.json({}, { status: 500, statusText: 'Server Error' }))) + .mockReturnValueOnce(Promise.resolve(Response.json({}, { status: 502, statusText: 'Bad Gateway' }))) + .mockReturnValueOnce(Promise.resolve(Response.json({}, { status: 503, statusText: 'Service Unavailable' }))) + + const promise = stopBuildUpstream() + await vi.advanceTimersByTimeAsync(2000) + const result: any = await promise + + expect(vi.mocked(fetch).mock.calls.length).toEqual(3) expect(result.status).toEqual('error') }) + it('returns success when a later retry attempt succeeds', async () => { + process.env[TESTOPS_BUILD_COMPLETED_ENV] = 'true' + process.env[BROWSERSTACK_TESTHUB_JWT] = 'jwt' + + // SDK-7061: a transient failure followed by a 2xx must resolve to success without + // exhausting all attempts. + useBackoffTimers() + vi.mocked(fetch) + .mockReturnValueOnce(Promise.resolve(Response.json({}, { status: 500, statusText: 'Server Error' }))) + .mockReturnValueOnce(Promise.resolve(Response.json({}))) + + const promise = stopBuildUpstream() + await vi.advanceTimersByTimeAsync(2000) + const result: any = await promise + + expect(vi.mocked(fetch).mock.calls.length).toEqual(2) + expect(result.status).toEqual('success') + }) + afterEach(() => { + // mockClear (not mockReset) so the shared global fetch mock keeps its default + // implementation for the rest of the file; the *Once queues above are fully + // consumed within each test, so nothing leaks. vi.mocked(fetch).mockClear() + // Restore the module-level clock config (only Date faked) so later suites in this + // file are unaffected by the setTimeout faking these retry tests turn on. + vi.useFakeTimers({ toFake: ['Date'] }).setSystemTime(new Date('2020-01-01')) }) })