From 6d8c086546671ecc63e2e55bc4d43673f5c19977 Mon Sep 17 00:00:00 2001 From: Kamalpreet Kaur Date: Fri, 17 Jul 2026 01:03:55 +0530 Subject: [PATCH 1/2] fix(browserstack-service): report skipped and hook-aborted tests in the CLI flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the CLI (gRPC) pipeline, tests that never reach beforeTest/afterTest emitted no events at all: static it.skip, this.skip() inside before/beforeEach hooks, and suites aborted by a failed before hook. The legacy Listener -> api/v1/batch path these flowed through previously is not functional in the CLI pipeline, so such tests were invisible on the dashboard and their Automate sessions were never linked to the build. - add cli/skipReporter: routes skipped tests through the TestFramework tracker using the same INIT_TEST -> TEST PRE -> LOG_REPORT POST -> TEST POST sequence afterTest uses (LOG_REPORT/POST is what loads the result), serialized through a single chain since wdio does not await reporter hooks - reporter.onTestSkip: CLI branch reporting the skipped test with a resolved spec file (events without a location are rejected downstream) and the suite chain passed via ctx for hierarchy extraction - service.afterHook: on a non-passed BEFORE_ALL/BEFORE_EACH/AFTER_EACH hook, report the suite's undetermined tests as skipped (port of the legacy insights-handler cascade); service.beforeTest marks started tests so runtime this.skip() is never double-reported - wdioMochaTestFramework: hook results read `.status`, which does not exist on Frameworks.TestResult, leaving hook_result at 'pending' (coerced to 'passed' downstream) — failed before-hooks produced green builds while CI exited 1. Map passed/skipped/failed from the actual result fields. Co-Authored-By: Claude Fable 5 --- .../cli/frameworks/wdioMochaTestFramework.ts | 9 +- .../src/cli/skipReporter.ts | 96 +++++++++++++++++++ packages/browserstack-service/src/reporter.ts | 30 +++++- packages/browserstack-service/src/service.ts | 12 +++ .../tests/cli/skipReporter.test.ts | 93 ++++++++++++++++++ 5 files changed, 238 insertions(+), 2 deletions(-) create mode 100644 packages/browserstack-service/src/cli/skipReporter.ts create mode 100644 packages/browserstack-service/tests/cli/skipReporter.test.ts diff --git a/packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts b/packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts index 60c6ddb..a45104b 100644 --- a/packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts +++ b/packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts @@ -410,7 +410,14 @@ export default class WdioMochaTestFramework extends TestFramework { if (hooksList.length > 0) { const hook = hooksList.pop() as Record - const result = testResult.status + // Frameworks.TestResult carries `passed`/`skipped`, not `status` — reading `.status` + // left hook_result at 'pending', which the binary coerces to 'passed' for any + // finished hook, so failed before-hooks showed green builds while CI exited 1. + // A this.skip() hook arrives as {passed: false, skipped: true} — a deliberate + // skip, not a failure. + const result = testResult + ? (testResult.passed ? 'passed' : (testResult.skipped ? 'skipped' : 'failed')) + : TestFrameworkConstants.DEFAULT_HOOK_RESULT if (result !== TestFrameworkConstants.DEFAULT_HOOK_RESULT) { hook[TestFrameworkConstants.KEY_HOOK_RESULT] = result } diff --git a/packages/browserstack-service/src/cli/skipReporter.ts b/packages/browserstack-service/src/cli/skipReporter.ts new file mode 100644 index 0000000..5ada0da --- /dev/null +++ b/packages/browserstack-service/src/cli/skipReporter.ts @@ -0,0 +1,96 @@ +import path from 'node:path' + +import type { Frameworks } from '@wdio/types' + +import type TestFramework from './frameworks/testFramework.js' +import { TestFrameworkState } from './states/testFrameworkState.js' +import { HookState } from './states/hookState.js' +import { BStackLogger } from '../bstackLogger.js' + +/** + * Reports tests that never reach the beforeTest/afterTest lifecycle (static `it.skip`, + * `this.skip()` inside before hooks, suites aborted by a failed before hook) through the + * CLI gRPC tracker, so they land on the dashboard and attribute their Automate session. + * Without this, such tests emit no events at all in the CLI flow (the legacy + * Listener -> api/v1/batch path is dead here) and their sessions surface as + * `session_linking_issue_build` in TRA stability. + */ + +interface MochaRuntimeTest { + title: string + state?: string + body?: string + file?: string + parent?: { title?: string, parent?: unknown, tests?: unknown[], suites?: unknown[] } +} + +// tests that entered the CLI beforeTest lifecycle — those report their own finish +// (including runtime `this.skip()` inside a test body) and must not be re-reported +const startedTests = new Set() +const reportedSkips = new Set() +// wdio does not await reporter hooks, so back-to-back skips would interleave on the +// tracker's single mutable per-worker instance — serialize every report through one chain +let reportChain: Promise = Promise.resolve() + +export function markTestStarted(identifier: string) { + startedTests.add(identifier) +} + +export function reportSkippedTest(framework: TestFramework, identifier: string, test: Frameworks.Test, suiteTitle?: string): Promise { + if (startedTests.has(identifier) || reportedSkips.has(identifier)) { + return reportChain + } + reportedSkips.add(identifier) + const result = { passed: false, skipped: true } as Frameworks.TestResult + reportChain = reportChain.then(async () => { + // LOG_REPORT/POST is what loads the result into the instance (loadTestResult is + // gated on it, not on TEST/POST) — same sequence afterTest uses + await framework.trackEvent(TestFrameworkState.INIT_TEST, HookState.PRE, { test }) + await framework.trackEvent(TestFrameworkState.TEST, HookState.PRE, { test, suiteTitle }) + await framework.trackEvent(TestFrameworkState.LOG_REPORT, HookState.POST, { test, result }) + await framework.trackEvent(TestFrameworkState.TEST, HookState.POST, { test, result, suiteTitle }) + }).catch((err: unknown) => { + BStackLogger.debug(`Failed reporting skipped test '${identifier}': ${err}`) + }) + return reportChain +} + +/** + * Port of the legacy insights-handler skip propagation: when a BEFORE_ALL/BEFORE_EACH/ + * AFTER_EACH hook fails (or skips), mocha silently drops the remaining tests in the + * suite — report each state-undefined test as skipped, recursing into nested describes. + */ +export async function reportSuiteSkipped(framework: TestFramework, suite: { tests?: unknown[], suites?: unknown[] }): Promise { + for (const t of (suite.tests || []) as MochaRuntimeTest[]) { + if (t.state !== undefined) { + continue + } + const parentTitle = t.parent?.title ?? '' + const identifier = `${parentTitle} - ${t.title}` + // keep `parent` a string (the Automate session name interpolates it) and pass the + // real suite chain via ctx for scope/hierarchy extraction; `file` must resolve or + // the binary drops the event on path.relative(cwd, undefined) + const synthetic = { + title: t.title, + parent: parentTitle, + body: t.body || '', + file: t.file, + ctx: { test: { parent: t.parent } } + } as unknown as Frameworks.Test + await reportSkippedTest(framework, identifier, synthetic, parentTitle) + } + for (const sub of (suite.suites || []) as { tests?: unknown[], suites?: unknown[] }[]) { + await reportSuiteSkipped(framework, sub) + } +} + +/** Resolve a spec file path usable by the binary (it rejects events with no location). */ +export function resolveSpecFile(candidate: string | undefined, runnerSpec: string | undefined): string | undefined { + if (candidate) { + return candidate + } + if (runnerSpec) { + return runnerSpec.startsWith('file://') ? runnerSpec.replace(/^file:\/\//, '') : path.resolve(runnerSpec) + } + return undefined +} diff --git a/packages/browserstack-service/src/reporter.ts b/packages/browserstack-service/src/reporter.ts index 4fd6b73..1c48f50 100644 --- a/packages/browserstack-service/src/reporter.ts +++ b/packages/browserstack-service/src/reporter.ts @@ -2,7 +2,9 @@ import path from 'node:path' import type { SuiteStats, TestStats, RunnerStats, HookStats } from '@wdio/reporter' import WDIOReporter from '@wdio/reporter' -import type { Options } from '@wdio/types' +import type { Options, Frameworks } from '@wdio/types' +import { BrowserstackCLI } from './cli/index.js' +import { reportSkippedTest, resolveSpecFile } from './cli/skipReporter.js' import * as url from 'node:url' import { v4 as uuidv4 } from 'uuid' @@ -40,10 +42,12 @@ class _TestReporter extends WDIOReporter { public static currentTest: CurrentRunInfo = {} private _userCaps?: Capabilities.ResolvedTestrunnerCapabilities = {} private listener = Listener.getInstance() + private _runnerSpec?: string public static hashCodeToHandleTestSkip: Record = {} async onRunnerStart (runnerStats: RunnerStats) { this._capabilities = runnerStats.capabilities as WebdriverIO.Capabilities + this._runnerSpec = runnerStats.specs?.[0] this._userCaps = this.getUserCaps(runnerStats) this._config = runnerStats.config as BrowserstackConfig & Options.Testrunner this._sessionId = runnerStats.sessionId @@ -217,6 +221,30 @@ class _TestReporter extends WDIOReporter { return } + // CLI flow: the legacy Listener -> api/v1/batch path below is dead here, so route the + // skipped test through the gRPC tracker instead (static `it.skip` and hook-level skips + // never reach beforeTest/afterTest; without this they are invisible on the dashboard + // and their session is never linked to the build) + if (BrowserstackCLI.getInstance().isRunning()) { + const framework = BrowserstackCLI.getInstance().getTestFramework() + if (framework) { + const stats = testStats as TestStats & { parent?: string, file?: string, body?: string, ctx?: unknown } + const identifier = `${stats.parent} - ${stats.title}` + stats.file = resolveSpecFile(stats.file, this._runnerSpec) + // hierarchy travels via ctx (getMochaTestHierarchy); `parent` stays a string — + // the Automate session name interpolates it directly + let parentChain: { title: string, parent: unknown } | null = null + for (const s of this._suites) { + parentChain = { title: s.title, parent: parentChain } + } + if (parentChain) { + stats.ctx = { test: { parent: parentChain } } + } + await reportSkippedTest(framework, identifier, stats as unknown as Frameworks.Test, this._suites.at(-1)?.title) + } + return + } + testStats.start ||= new Date() testStats.end ||= new Date() const testData = await this.getRunData(testStats, 'TestRunSkipped') diff --git a/packages/browserstack-service/src/service.ts b/packages/browserstack-service/src/service.ts index c5f21d1..8a92cf0 100644 --- a/packages/browserstack-service/src/service.ts +++ b/packages/browserstack-service/src/service.ts @@ -30,6 +30,7 @@ import PerformanceTester from './instrumentation/performance/performance-tester. import * as PERFORMANCE_SDK_EVENTS from './instrumentation/performance/constants.js' import { EVENTS } from './instrumentation/performance/constants.js' import { BrowserstackCLI } from './cli/index.js' +import { markTestStarted, reportSuiteSkipped } from './cli/skipReporter.js' import { CLIUtils } from './cli/cliUtils.js' import { _fetch as fetch } from './fetchWrapper.js' @@ -435,6 +436,14 @@ export default class BrowserstackService implements Services.ServiceInstance { if (hookFrameworkState) { await framework.trackEvent(hookFrameworkState, HookState.POST, { test, result }) } + // a failed (or skipping) before/each hook silently drops the suite's remaining + // tests in mocha — report them as skipped so they surface on the dashboard and + // attribute their Automate session (port of the legacy insights-handler cascade) + const hookType = getHookType((test as Frameworks.Test).title) + const suite = (test as Frameworks.Test).ctx?.test?.parent + if (result && !result.passed && ['BEFORE_ALL', 'BEFORE_EACH', 'AFTER_EACH'].includes(hookType) && suite) { + await reportSuiteSkipped(framework, suite) + } } return } @@ -467,6 +476,9 @@ export default class BrowserstackService implements Services.ServiceInstance { if (this._config.framework === 'mocha' && uuid) { this._cliTestUuids.set(getUniqueIdentifier(test, this._config.framework), uuid as string) } + // this test reports its own finish (incl. runtime `this.skip()`), so the + // skip reporter must never re-report it from onTestSkip + markTestStarted(getUniqueIdentifier(test, this._config.framework)) this._insightsHandler?.setTestData(test, uuid) await BrowserstackCLI.getInstance().getTestFramework()!.trackEvent(TestFrameworkState.TEST, HookState.PRE, { test, suiteTitle }) return diff --git a/packages/browserstack-service/tests/cli/skipReporter.test.ts b/packages/browserstack-service/tests/cli/skipReporter.test.ts new file mode 100644 index 0000000..12aea7e --- /dev/null +++ b/packages/browserstack-service/tests/cli/skipReporter.test.ts @@ -0,0 +1,93 @@ +import path from 'node:path' + +import { describe, expect, it, vi, beforeEach } from 'vitest' +import type { Frameworks } from '@wdio/types' + +vi.mock('@wdio/logger', () => import(path.join(process.cwd(), '__mocks__', '@wdio/logger'))) + +import { markTestStarted, reportSkippedTest, reportSuiteSkipped, resolveSpecFile } from '../../src/cli/skipReporter.js' +import { TestFrameworkState } from '../../src/cli/states/testFrameworkState.js' +import { HookState } from '../../src/cli/states/hookState.js' +import type TestFramework from '../../src/cli/frameworks/testFramework.js' + +const makeFramework = () => ({ trackEvent: vi.fn().mockResolvedValue(undefined) }) as unknown as TestFramework + +const makeTest = (title: string, parent = 'suite') => ({ title, parent }) as unknown as Frameworks.Test + +describe('skipReporter', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('reports a skipped test through the INIT_TEST/TEST/LOG_REPORT sequence', async () => { + const framework = makeFramework() + await reportSkippedTest(framework, 'suite - reports once', makeTest('reports once'), 'suite') + + const calls = vi.mocked(framework.trackEvent).mock.calls + expect(calls.map(([state, hook]) => [state, hook])).toEqual([ + [TestFrameworkState.INIT_TEST, HookState.PRE], + [TestFrameworkState.TEST, HookState.PRE], + [TestFrameworkState.LOG_REPORT, HookState.POST], + [TestFrameworkState.TEST, HookState.POST], + ]) + expect(calls[2][2]).toMatchObject({ result: { passed: false, skipped: true } }) + }) + + it('does not re-report the same identifier', async () => { + const framework = makeFramework() + await reportSkippedTest(framework, 'suite - dedup', makeTest('dedup'), 'suite') + await reportSkippedTest(framework, 'suite - dedup', makeTest('dedup'), 'suite') + expect(framework.trackEvent).toHaveBeenCalledTimes(4) + }) + + it('does not report tests that entered the beforeTest lifecycle', async () => { + const framework = makeFramework() + markTestStarted('suite - runtime skip') + await reportSkippedTest(framework, 'suite - runtime skip', makeTest('runtime skip'), 'suite') + expect(framework.trackEvent).not.toHaveBeenCalled() + }) + + it('serializes concurrent reports through one chain', async () => { + const order: string[] = [] + const framework = { + trackEvent: vi.fn().mockImplementation(async (_s: unknown, _h: unknown, args: { test: { title: string } }) => { + order.push(args.test.title) + await new Promise(resolve => setTimeout(resolve, 1)) + }) + } as unknown as TestFramework + + await Promise.all([ + reportSkippedTest(framework, 'suite - first', makeTest('first'), 'suite'), + reportSkippedTest(framework, 'suite - second', makeTest('second'), 'suite'), + ]) + expect(order).toEqual(['first', 'first', 'first', 'first', 'second', 'second', 'second', 'second']) + }) + + it('walks nested suites and skips already-determined tests', async () => { + const framework = makeFramework() + const parent = { title: 'outer' } + const suite = { + tests: [ + { title: 'ran already', state: 'passed', parent }, + { title: 'undetermined', parent, file: '/spec/a.spec.js' }, + ], + suites: [{ + tests: [{ title: 'nested undetermined', parent: { title: 'inner' } }], + suites: [], + }], + } + await reportSuiteSkipped(framework, suite) + // 2 undetermined tests x 4 tracker events + expect(framework.trackEvent).toHaveBeenCalledTimes(8) + const reported = vi.mocked(framework.trackEvent).mock.calls + .filter(([state]) => state === TestFrameworkState.INIT_TEST) + .map(([, , args]) => (args as { test: { title: string } }).test.title) + expect(reported).toEqual(['undetermined', 'nested undetermined']) + }) + + it('resolves spec file from the runner spec when the test has none', () => { + expect(resolveSpecFile('/abs/spec.js', 'file:///runner/spec.js')).toBe('/abs/spec.js') + expect(resolveSpecFile(undefined, 'file:///runner/spec.js')).toBe('/runner/spec.js') + expect(resolveSpecFile(undefined, undefined)).toBeUndefined() + }) +}) From 756c67592989dc98a78704d964356e214ae036ac Mon Sep 17 00:00:00 2001 From: Kamalpreet Kaur Date: Fri, 17 Jul 2026 19:05:24 +0530 Subject: [PATCH 2/2] fix: clearer batch-upload failures; guard sync-skip hook shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - batchAndPostEvents: read the response as text and check response.ok — error responses (401/5xx) and empty bodies are not JSON, and the blind response.json() surfaced every failure as a misleading "Unexpected end of JSON input". Failures now carry the HTTP status and a body snippet. - listener: include the failure reason when a batch is marked failed instead of only the event count. - trackHookEvents: also treat mocha's sync-skip error shape as a skipped hook (wdio v8 delivers this.skip() hooks without a `skipped` flag; harmless on v9, keeps both lines aligned). Co-Authored-By: Claude Fable 5 --- .../src/cli/frameworks/wdioMochaTestFramework.ts | 5 ++++- packages/browserstack-service/src/testOps/listener.ts | 4 ++-- packages/browserstack-service/src/util.ts | 8 +++++++- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts b/packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts index a45104b..76e4b88 100644 --- a/packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts +++ b/packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts @@ -415,8 +415,11 @@ export default class WdioMochaTestFramework extends TestFramework { // finished hook, so failed before-hooks showed green builds while CI exited 1. // A this.skip() hook arrives as {passed: false, skipped: true} — a deliberate // skip, not a failure. + // guard the sync-skip error shape too (wdio v8 delivers this.skip() hooks without + // a `skipped` flag; harmless on v9, keeps the lines aligned) + const skippedHook = testResult?.skipped || !!testResult?.error?.message?.includes('sync skip; aborting execution') const result = testResult - ? (testResult.passed ? 'passed' : (testResult.skipped ? 'skipped' : 'failed')) + ? (testResult.passed ? 'passed' : (skippedHook ? 'skipped' : 'failed')) : TestFrameworkConstants.DEFAULT_HOOK_RESULT if (result !== TestFrameworkConstants.DEFAULT_HOOK_RESULT) { hook[TestFrameworkConstants.KEY_HOOK_RESULT] = result diff --git a/packages/browserstack-service/src/testOps/listener.ts b/packages/browserstack-service/src/testOps/listener.ts index 160a2ae..29ec61e 100644 --- a/packages/browserstack-service/src/testOps/listener.ts +++ b/packages/browserstack-service/src/testOps/listener.ts @@ -228,8 +228,8 @@ class Listener { await batchAndPostEvents(DATA_BATCH_ENDPOINT, 'BATCH_DATA', data) BStackLogger.debug('callback: marking events success ' + data.length) this.eventsSuccess(data) - } catch { - BStackLogger.debug('callback: marking events failed ' + data.length) + } catch (err) { + BStackLogger.debug(`callback: marking events failed ${data.length} reason: ${err}`) this.eventsFailed(data) } finally { this.pendingUploads -= 1 diff --git a/packages/browserstack-service/src/util.ts b/packages/browserstack-service/src/util.ts index 136eebd..ca200b4 100644 --- a/packages/browserstack-service/src/util.ts +++ b/packages/browserstack-service/src/util.ts @@ -1257,7 +1257,13 @@ export async function batchAndPostEvents (eventUrl: string, kind: string, data: }, body: JSON.stringify(data) }) - BStackLogger.debug(`[${kind}] Success response: ${JSON.stringify(await response.json())}`) + // read as text first: error responses (401/5xx) and empty bodies are not JSON, and a blind + // response.json() surfaced them as a misleading "Unexpected end of JSON input" + const responseBody = await response.text() + if (!response.ok) { + throw new Error(`HTTP ${response.status} ${response.statusText}${responseBody ? ` — ${responseBody.slice(0, 300)}` : ' (empty body)'}`) + } + BStackLogger.debug(`[${kind}] Success response: ${responseBody}`) } catch (error) { BStackLogger.debug(`[${kind}] EXCEPTION IN ${kind} REQUEST TO TEST REPORTING AND ANALYTICS : ${error}`) throw new Error('Exception in request ' + error)