feat(SDK-7063): report bail-skipped tests as skipped - #122
Open
harshit-browserstack wants to merge 8 commits into
Open
feat(SDK-7063): report bail-skipped tests as skipped#122harshit-browserstack wants to merge 8 commits into
harshit-browserstack wants to merge 8 commits into
Conversation
Tests that never run because of `bail` were absent from the Test Run entirely. Report them as `skipped` so the report accounts for every declared test. WebdriverIO has two unrelated bail mechanisms, handled separately: - `mochaOpts.bail` halts the failing spec. `service.afterTest` walks `test.ctx.test.parent` to the spec's root suite and reuses the existing `reportSuiteSkipped` cascade, so sibling describes are covered. Gated strictly on mocha's own option -- under wdio's `bail` every in-spec test still runs, so triggering on it would report tests that are about to execute. - wdio's `bail: <n>` counts failed spec files and stops scheduling more. `launcher.onComplete` diffs dispatched specs against the run's spec list and enumerates the rest via mocha's declaration pass, without executing any test body. Launcher-side reporting only happens when bail demonstrably fired (`failedRunners >= bail`, counted the way wdio counts it -- a failed spec with retries left is requeued, not counted). It is skipped entirely under sharding, when capabilities define their own specs/exclude, and for excluded specs. A spec can fail to run for many reasons unrelated to bail, and reporting one of those as skipped would corrupt the report; under-reporting is recoverable, phantom skips are not. Events are relayed through the binary's EnqueueTestEvent RPC. The launcher's TESTHUB_JWT is the binary's account token on the CLI path and carries no build claim, so posting to the collector directly returns 401; the binary holds the build-scoped credential. The RPC already existed in the proto -- only the client wrapper was missing. Reported identity matches `getUniqueIdentifier` so a test reported skipped and the same test on a later run share one identity. Covers the CLI/gRPC path only. Enumerating never-started specs re-executes their top-level code.
Wire evidence showed a test that runs emits `name` and `identifier` as the bare title, with only `scope` carrying the parent: identifier: 'MA-TC1 passes' scope: 'Multi Spec A - MA-TC1 passes' Skipped tests reported for never-launched specs were setting `identifier` to the parent-qualified form, so the same test reported skipped and later run would not have matched on that field. Verified on build bpt0jvulwfonuoucepdu892xhzborork2hy7tgqb: executed and skipped tests now emit identical conventions for all three fields.
The customer's config is `bail: 0` with `mochaOpts.bail: true`, so only Mocha's in-spec bail applies to them. WebdriverIO's spec-file-level bail is disabled in their setup and the launcher-side reporting never runs. Drop the never-launched-spec work from this PR so the change that unblocks them is small and built on the already-proven `reportSuiteSkipped` path. That work is preserved on `feat/sdk-7063-unlaunched-specs` and can land separately on its own review. Removes: specEnumerator, unlaunchedSpecReporter, the launcher hooks, the EnqueueTestEvent client wrapper, and the mocha devDependency they needed. README and changeset narrowed to match.
`results.retries` only tracks wdio's spec-file retries. @wdio/utils builds
it as `{attempts: 0, limit: repeatTest}` and @wdio/mocha-framework never
feeds `mochaOpts.retries` into it, so under mocha-level retries it stays
`{0, 0}` and the guard evaluated `0 < 0` — it never suppressed anything.
The cascade therefore fired on the first failed attempt, before the retry
ran. When the retry then passed, those tests reported `skipped` and later
`passed` — two TestRunFinished events with different uuids for one test.
The dashboard looked correct only because TRA collapses same-name rows and
the later result wins.
Read mocha's own runnable state (`currentRetry()` vs `retries()`) and keep
the wdio check for spec-file retries.
Verified on build zynxn48dbs3fsbfhd975tghdrsj2550xdtwenqlm: zero `skipped`
emissions on the wire where there were previously three, dashboard 5/0/0.
Collaborator
Author
|
✅ Good to go
↻ This verdict comment is the review anchor — it's updated in place on each run (the gate posts its status separately). — SDK PR Review Agent |
hasRetryPending reads mocha's own runnable via currentRetry()/retries(). It ran before the try block, so a throw from either escaped uncaught into the awaited afterTest hook and would surface as a framework-level error in the user's run. Move the check inside the boundary that already wraps every other call in the method. The tests asserted only trackEvent call counts, so a cascade that swept the wrong tests still produced the expected total. Assert which titles were reported skipped as well — counts catch a test emitted twice, titles catch the wrong test emitted. Adds the two uncovered cases: the results.skipped gate branch, and a runnable whose retry accessors throw. A mocha root can span several spec files after all: MochaAdapter creates one Mocha and addFile()s every spec it is handed, so grouped specs share a root. Cascading across them stays correct, since bail aborts that whole runner. Correct the comment that claimed the opposite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this about?
With
mochaOpts: { bail: true }, Mocha halts a spec on its first failure and the remaining tests in that file never run. Today those tests are not reported in any state — they simply disappear from the Test Run, so the report silently under-counts. This PR reports them asskipped.Tests in sibling
describeblocks in the same file are covered too, since Mocha's bail aborts the whole spec rather than just the failing block.How it works
afterTestalready fires for the failing test. From there we reach the live Mocha suite viatest.ctx.test.parent, walk up to the spec's root suite, and hand it to the existingreportSuiteSkippedcascade — the same code path already used when abeforehook fails and Mocha silently drops the rest of the suite. That is the identical shape of problem, so this is a second trigger on a proven path rather than new machinery.Mocha builds its full suite tree at file-load time, before running anything, so tests that never execute are already known and carry
state === undefined.Deliberately gated on
mochaOpts.bailonlyWebdriverIO's top-level
bailis a different setting: it counts failed spec files and stops scheduling further ones, and is never forwarded to Mocha. Under it, every test in the running spec still executes — so triggering on it would report tests that are about to run. Reporting is also skipped while a retry is still queued, for the same reason.Not covered here
Tests in spec files that never started (WebdriverIO's
bail) are not reported. That is a separate, launcher-side change and is kept out of this PR to keep the blast radius small; the work exists onfeat/sdk-7063-unlaunched-specsand can land on its own review.Also scoped to the CLI/gRPC reporting path — the legacy direct-HTTP path and multi-remote runs are unchanged.
Related Jira task/s
Release (mandatory for every PR — required for the
ready-for-reviewlabel)Version bump:
Release notes type:
Release notes (customer-facing):
bailhalted a spec are now reported asskipped, instead of being left out of the report entirely — so a Test Run accounts for every test in that spec.Release notes (internal):
service.afterTest: whenmochaOpts.bailis set and a test fails, resolve the spec's root suite fromtest.ctx.test.parentand run the existingreportSuiteSkippedcascade over it. Root-walk (rather than starting at the failing test's own suite) is what covers siblingdescribeblocks.mochaOpts.bailwith Mocha's own truthiness rather thanisTrue(), which is a strict'true'string compare and would miss a numericbail: 1.config.bail: in-spec tests still run under it, so reporting them would double-count. Also skipped whileresults.retries.attempts < limit.Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.
Testing evidence
Verified against real BrowserStack builds with Observability assertions, not just console output. Each scenario was run twice — once on the published SDK, once with the fix.
describe, tests in a siblingdescribeThe sibling-
describerun also confirmed the reportedscopesstay per-suite (['Suite B']) rather than being flattened, and that skipped tests carry the same Automate session id as the tests that ran — so they attribute to the session instead of orphaning.Identity fields match what an executed test emits (
name/identifier= bare title,scope=parent - title), so a test reported skipped and the same test on a later successful run are recognised as one test.4 unit tests added;
service.test.tsfailure count unchanged frommain(those are pre-existing and environmental —vi.mock('fetch')is a no-op, so they make real network calls with dummy credentials).