fix: report skipped and hook-aborted tests in the CLI flow

[kamal-kaur04]

Why

In the CLI (gRPC) pipeline, tests that never reach beforeTest/afterTest emit 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 (its own usage stats report skipped: { triggeredCount: N, sentCount: 0, failedCount: N }), so such tests are invisible on the Test Observability dashboard and their Automate sessions are never linked to the build (surfacing as session-linking noise in build-stability metrics).

Separately, a failed before hook produced a green build while CI exited 1: hook results read testResult.status, which does not exist on Frameworks.TestResult, leaving hook_result at 'pending' — which downstream coerces to 'passed' for any finished hook.

What

• cli/skipReporter.ts (new): 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). Reports are serialized through a single chain — wdio does not await reporter hooks, and unserialized sequences interleave on the tracker's single per-worker instance. Started/reported dedup sets prevent double-reporting runtime this.skip() (which already reports via afterTest).
• reporter.ts: onTestSkip CLI branch — reports the skipped test with a resolved spec file (events without a file location are rejected downstream) and the suite chain passed via ctx for hierarchy extraction (parent stays a string; the Automate session name interpolates it).
• service.ts: afterHook CLI branch — on a non-passed BEFORE_ALL/BEFORE_EACH/AFTER_EACH hook, reports the suite's undetermined tests as skipped (port of the legacy insights-handler cascade); beforeTest marks started tests for the dedup.
• cli/frameworks/wdioMochaTestFramework.ts: hook results mapped from the actual Frameworks.TestResult fields — passed / skipped: true / else → passed / skipped / failed, so genuine hook failures reach the dashboard while deliberate this.skip() hooks stay non-failing.

Validation

Live-validated against BrowserStack with a 6-spec matrix (2 normal specs, it.skip, runtime this.skip(), this.skip() in before and beforeEach, and a throwing before hook), verified via the Observability API:

• Before: build passed, stats {passed: 3} or {passed: 4} — skipped/aborted suites entirely absent from the hierarchy, their sessions unlinked.
• After: stats {passed: 4, failed: 1, skipped: 8} — all 12 tests present exactly once, every test attributed to its Automate session, build status failed driven only by the genuinely-thrown hook (conditional-skip suites stay green), hook results on the wire: 1 failed, 2 skipped, 5 passed.

npm run build (buf generate + esbuild + tsc) passes. New unit tests for skipReporter (6, all passing); pre-existing fetch-mock test failures on main are unchanged by this PR.

🤖 Generated with Claude Code

[xxshubhamxx]

Short answer: the overall direction of the PR is solid and consistent with the rest of the repo, but I would ask for a few concrete changes before merging, mainly around state lifecycle, identifiers, and a small control-flow edge case. Functionally the fix does what it claims.

Below I’ll walk through the main areas.

---

Core behavior changes

The PR introduces a new CLI-only skip reporter, hooks it into reporter.ts and service.ts, and fixes hook result mapping in WdioMochaTestFramework.

Conceptually:

• Skipped tests that never reach beforeTest/afterTest (static it.skip, this.skip() in before*, suites dropped after a failed hook) now emit events over the gRPC tracker and show up in Observability, with Automate sessions linked.
• Hook results now read passed/skipped from Frameworks.TestResult instead of a non-existent status field, so failing before-hooks no longer show as “pending→passed”.
• The CLI path mirrors the legacy insights-handler semantics for hook cascades and for the INIT_TEST → TEST PRE → LOG_REPORT POST → TEST POST sequence.

That all lines up logically, and is consistent with how the rest of the service treats mocha and the CLI.

---

New skipReporter module

State & lifecycle

skipReporter.ts maintains:

• startedTests: Set<string>
• reportedSkips: Set<string>
• reportChain: Promise<void> to serialize trackEvent calls.

Issues / suggestions:

1. State never reset across worker lifetime

   The sets and reportChain are module-level and never cleared. In a long-lived worker that runs multiple specs sequentially, any identifiers added in the first spec will keep suppressing later reports with the same identifier (for example, if another file defines the same “<suite> - <title>” combination).

   This is subtle, but given BrowserStack often runs multiple specs per worker, it’s safer to expose and use a reset:

   // skipReporter.ts
   export function resetSkipReporter() {
     startedTests.clear()
     reportedSkips.clear()
     reportChain = Promise.resolve()
   }

   And call resetSkipReporter() from a suitable lifecycle (e.g. onWorkerEnd or at least in the unit test beforeEach).

2. Tests don’t reset this state

   skipReporter.test.ts only calls vi.clearAllMocks() in beforeEach, so the sets and reportChain leak between test cases. That makes some expectations brittle and could hide issues if you reorder/add tests. Once you add resetSkipReporter, it should be used there as well.

3. Serialization strategy

   The reportChain pattern to serialize trackEvent calls is appropriate, given wdio does not await reporter hooks and the CLI tracker instance is single and mutable. The test that asserts ordering of concurrent reports exercises this nicely.

Identifier choice

Both reportSuiteSkipped and the CLI onTestSkip branch use:

const identifier = `${parentTitle} - ${t.title}`

or the equivalent in reporter.ts.

• This matches the legacy pattern in insights-handler where the same string is used to key _tests when sending skipped tests for hook cascades.
• However, skipReporter uses this string in reportedSkips as a global dedup key.

Risk: if two distinct tests in the same run share the same parentTitle and title (e.g., same nested describe structure across two files), the second will never be reported as skipped in the CLI path because reportedSkips already contains that identifier.

I’d recommend making the key more specific, e.g.:

const identifier = `${stats.file ?? ''}:${parentTitle} - ${stats.title}`

and mirroring that in reportSuiteSkipped (including file) so dedup is per-file+suite+title rather than just suite+title.

---

resolveSpecFile and path handling

resolveSpecFile does:

if (runnerSpec) {
  return runnerSpec.startsWith('file://')
    ? runnerSpec.replace(/^file:\/\//, '')
    : path.resolve(runnerSpec)
}

Concerns:

• On Unix, file:///runner/spec.js → /runner/spec.js, which is fine.
• On Windows, file:///C:/spec.js → /C:/spec.js, which is not a valid native path and won’t match what path.relative or the binary expects.

You already import node:path; consider importing fileURLToPath from node:url and doing:

import { fileURLToPath } from 'node:url'

if (runnerSpec?.startsWith('file://')) {
  return fileURLToPath(runnerSpec)
}

This will be correct cross-platform and consistent with how reporter.ts already uses url.fileURLToPath for Jasmine.

---

Reporter integration (reporter.ts)

The new onTestSkip branch for CLI is:

• Guarded by needToSendData('test', 'skip') (true for mocha).
• Uses BrowserstackCLI.getInstance().isRunning() to decide between CLI and legacy paths.
• Constructs identifier and ctx hierarchy, resolves the spec file via resolveSpecFile, then calls reportSkippedTest.

This is structurally sound, but there are two nuances.

Control flow if CLI is “running” but framework is null

You have:

if (BrowserstackCLI.getInstance().isRunning()) {
  const framework = BrowserstackCLI.getInstance().getTestFramework()
  if (framework) {
    // ... await reportSkippedTest(...)
  }
  return
}

• The return is inside the isRunning() block but outside the if (framework) guard.
• In normal operation, isRunning() should only be true when a test framework is registered, but getTestFramework() can be null during startup races or for non-mocha frameworks.

If that ever happens, the code returns without emitting a CLI event and without falling back to the legacy path, silently dropping the skip. A safer pattern:

if (BrowserstackCLI.getInstance().isRunning()) {
  const framework = BrowserstackCLI.getInstance().getTestFramework()
  if (framework) {
    // normal CLI handling
    await reportSkippedTest(...)

    return
  }
  // fall through to legacy path if framework is unexpectedly null
}

That preserves behavior in the expected case but avoids a hard swallow on unusual CLI states.

Hierarchy via _suites

The way you reconstruct parentChain from _suites and pass it via ctx is consistent with how insights-handler builds hierarchy for mocha tests. That should give the CLI the same describe-chain as the legacy path.

---

Service integration (service.ts)

Two main changes:

1. beforeTest: mark the test as “started” for dedup.

   if (this._config.framework === 'mocha' && uuid) {
     this._cliTestUuids.set(getUniqueIdentifier(test, this._config.framework), uuid as string)
   }
   markTestStarted(getUniqueIdentifier(test, this._config.framework))

   • Using getUniqueIdentifier(test, framework) keeps this aligned with how the legacy path keys tests in insights-handler.
   • This ensures runtime this.skip() tests (which do go through beforeTest) aren’t double-reported via the onTestSkip branch — good.

2. afterHook: cascade skip for undetermined tests after failing hooks.

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

   • This is a direct CLI port of the legacy insights-handler.afterHook behavior, including AFTER_EACH.
   • Semantically, it means: if any of beforeAll, beforeEach, or afterEach fails, remaining undetermined tests in the suite hierarchy are reported as skipped, matching the non-CLI path.

This is consistent with the existing behavior; if you ever decide that cascading on AFTER_EACH is not desired, you should change both insights-handler and skipReporter together. For the purpose of this PR, mirroring the legacy logic is correct.

Also nice: after() now clears _cliTestUuids after sweeping unfinished tests, ensuring snapshots don’t leak across workers. skipReporter’s sets are the only state left not tied into teardown.

---

Hook result mapping (wdioMochaTestFramework.ts)

Previously, hook results read testResult.status, which doesn’t exist on Frameworks.TestResult, leaving hooks in the default "pending" state and causing the binary to treat them as passed.

The new mapping:

const result = testResult
  ? (testResult.passed ? 'passed' : (testResult.skipped ? 'skipped' : 'failed'))
  : TestFrameworkConstants.DEFAULT_HOOK_RESULT

This is the right thing to do:

• It correctly distinguishes passed, skipped, and failed based on the actual WDIO result flags.
• The comment about this.skip() arriving as { passed: false, skipped: true } matches WDIO’s behavior for skipped hooks.
• The default remains “pending” when no result exists, which is reasonable for hooks that genuinely never ran.

---

Tests and coverage

The new skipReporter.test.ts suite covers several key behaviors:

• Correct INIT/TEST/LOG_REPORT/TEST sequence and result shape.
• Deduplication of repeated identifiers.
• Respecting markTestStarted (no double-report for tests that hit beforeTest).
• Serialization of concurrent reports (ensuring order).
• Walking nested suites and skipping already-determined tests.
• resolveSpecFile behavior.

Gaps to consider filling:

• A test that simulates two distinct skipped tests with the same parentTitle + title in different suites/files, to make the dedup behavior explicit (either as expected or as a regression test after tightening identifiers).
• A test that exercises the onTestSkip CLI branch end-to-end via a small harness (even if partially mocked), to catch any future regressions in the framework null/fall-through logic.

---

Repo-wide fit

Looking at the broader repo structure and existing patterns:

• Logging: use of BStackLogger in skipReporter matches the rest of the service.
• “Legacy Listener vs CLI” split: the new code mirrors existing guards (BrowserstackCLI.getInstance().isRunning()) and keeps the legacy Listener → api/v1/batch path intact for non-CLI flows.
• getHookType, getUniqueIdentifier, and hierarchy-building are reused consistently from util.ts and insights-handler.ts.
• The comments are long, but in this repo that’s the established style (detailed behavioral notes and references to upstream Mocha/CLI behavior).

I don’t see any obvious violations of existing architectural conventions.

---

Concrete changes I’d request before approval

1. Add a reset entry point to skipReporter and use it in tests; consider also calling it at worker teardown to avoid state bleeding between runs.
2. Make the skip identifier more specific (include file), to avoid cross-file collisions when different tests share the same suite/title string.
3. Adjust the onTestSkip control flow to only return early when framework is non-null, letting unexpected getTestFramework() === null fall back to the legacy path.
4. Use fileURLToPath for file:// URLs in resolveSpecFile for cross-platform correctness.

Everything else looks consistent and, once these points are addressed, I’d be comfortable with this landing.

[kamal-kaur04]

Addendum (commit 756c675): batch-upload diagnostics + sync-skip hook guard

• 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 with the batch silently dropped. Failures now carry the HTTP status + body snippet, and the listener logs the failure reason. (Note for reviewers: unlike the v8 line's got-based sender, this fetch-based path has no retry — left as-is here since retry semantics deserve their own review.)
• 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 release lines aligned.

🤖 Generated with Claude Code