diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ac996e1..72b569ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,22 @@ Semver applies from 1.0.0. A breaking change to a documented API needs a major ## [Unreleased] -Nothing yet. +### Fixed + +- **A gate step whose suite executed nothing reports as skipped, never as passed** (#434). `x verify` + printed `✓ e2e 46ms` and `"skipped": false` over the one `e2eTest` a scaffold writes, which + `test.skip`s itself until the app registers a browser driver — so a customer's green gate included + a lane that had never run, and nothing in the table or in `--json` said so. `bun test` exits 0 over + an all-skipped file, but it prints its own counts and `packages/cli/src/test-counts.ts` was already + reading them: the rule is now `tests.ran === 0` in `packages/cli/src/verify-run.ts`, read once and + answered two ways — a step the committed `x.verify.json` requires stays the `X_VERIFY_SUITE_VANISHED` + failure it already was, and a step no floor requires is a skip beside `roadmap`'s, counted apart + from the passes in the summary line. A missing browser driver is a skip rather than an error code + because the framework ships no browser by design (`packages/scraping/src/cdp-port.ts`: the app + installs `puppeteer-core`, the CLI asks for it) — unlike Bun's own floor, which is `X_BUN_VERSION` + and a refusal. `StepResult.tests` now reaches `--json` and the human line, so `- e2e found 1 + test(s) and every one skipped itself` is distinguishable from `- roadmap`, which has no suite at + all. ## 20.1.0 - 2026-09-12 diff --git a/packages/cli/src/cmd-verify.test.ts b/packages/cli/src/cmd-verify.test.ts index 1421a421..5ef8cd08 100644 --- a/packages/cli/src/cmd-verify.test.ts +++ b/packages/cli/src/cmd-verify.test.ts @@ -216,12 +216,15 @@ describe('unit · x verify', () => { }); }); - // The floor is the claim; without one there is nothing to be measured against, and a repo that - // never committed one is not ratcheted in either direction. - test('with no floor, an all-skipped suite stays the honest pass it was', async () => { + // The floor is the claim; without one there is nothing to be measured against, so nothing is + // FAILED here. It is still not a pass: a suite that executed nothing is the same "nothing to + // check" a step with no files reports, and issue #434 is what a green check over it costs. + test('with no floor, an all-skipped suite is a skip rather than a pass', async () => { await withFloor(undefined, async (root) => { const result = await runVerify([...green, allSkipped], { ...ctx, root }); expect(result.ok).toBe(true); + expect(result.data).toMatchObject({ failed: [], skipped: ['live'] }); + expect(result.steps?.find((step) => step.name === 'live')?.skipped).toBe(true); }); }); diff --git a/packages/cli/src/e2e-driver.ts b/packages/cli/src/e2e-driver.ts index ac9e559f..a16e089f 100644 --- a/packages/cli/src/e2e-driver.ts +++ b/packages/cli/src/e2e-driver.ts @@ -70,7 +70,8 @@ export const e2eFixtures = (page: PageLike, browser: E2eBrowserPage): E2eFixture * declaration `driverFixtures()` registered — the ordinary way a driver arrives, last registration * wins — so every `test('…', async ({ page }) => …)` in the suite gets a browser. `useE2eDriver` * is the other half: it is what makes `hasE2eDriver()` answer true and stops `e2eTest` becoming a - * `test.skip` a green gate reports over. + * `test.skip` — which the gate now reports as a SKIPPED step rather than the green check it + * printed until #434, and which a repo whose `x.verify.json` names `e2e` gets red for. * * `budget`, `signIn` and `deploy` are deliberately NOT registered here. Each needs something a * page cannot supply — byte counts off a built `dist/`, an app's own sign-in route, a second build diff --git a/packages/cli/src/messages.ts b/packages/cli/src/messages.ts index 0c064d22..edd711e2 100644 --- a/packages/cli/src/messages.ts +++ b/packages/cli/src/messages.ts @@ -279,6 +279,14 @@ const CATALOG = { // The `errors` step's own coverage, in `output`: a scan without a parser reads most fix lines // and not all of them, and a step that reports findings alone claims a completeness it lacks. 'cli.verify.fixCoverage': 'checked {checked} fix line(s), could not read {unreadable}', + // Why a skipped step is a dash when the step can say so: the suite exists and not one of its + // tests ran. Each test's own name carries the reason it skipped, and `bun test` prints only the + // counts — so the line says how many and `x test ` prints the names. + 'cli.verify.allSkipped': 'found {skipped} test(s) and every one skipped itself', + // The other shape the same counts can take: a suite that reported no test at all, skipped ones + // included. Its own line and not `allSkipped` with a zero, because the repair differs — there is + // no skipped test to read a reason off, so the files the step selected hold no test. + 'cli.verify.ranNothing': 'found no test to run in the file(s) it selected', 'cli.verify.serial': 'serial', 'cli.verify.workers': '{workers} workers', 'cli.env.checked': '{count} declared variable(s), all present and valid', diff --git a/packages/cli/src/output.test.ts b/packages/cli/src/output.test.ts index 9ffaca28..5cd3ab3d 100644 --- a/packages/cli/src/output.test.ts +++ b/packages/cli/src/output.test.ts @@ -199,6 +199,28 @@ describe('unit · renderHuman says how a step was run', () => { expect(skipped).toContain('- unit'); }); + // #434: the whole point of reporting a zero-run suite as a skip is that a reader can tell it + // from a step with no suite at all, and the two shapes the counts take are two different + // repairs — read a skipped test's name, or find out why the files hold no test. + test('a skipped step that ran a suite says which kind of skip it is', () => { + const line = (tests: StepResult['tests']): string => + renderHuman({ + ok: true, + command: 'verify', + summary: 'ok', + steps: [ + stepResult({ name: 'e2e', skipped: true, ...(tests === undefined ? {} : { tests }) }), + ], + }); + expect(line({ ran: 0, skipped: 1 })).toContain(msg('cli.verify.allSkipped', { skipped: 1 })); + expect(line({ ran: 0, skipped: 0 })).toContain(msg('cli.verify.ranNothing')); + expect(line({ ran: 0, skipped: 0 })).not.toContain( + msg('cli.verify.allSkipped', { skipped: 0 }), + ); + // A step with no suite — `roadmap` — has nothing to say beyond the dash. + expect(line(undefined).split('\n')[0]?.trim()).toBe('- e2e 40ms'); + }); + test('a step with no workers field says nothing either', () => { expect( renderHuman({ ok: true, command: 'verify', summary: 'ok', steps: [stepResult({})] }), diff --git a/packages/cli/src/output.ts b/packages/cli/src/output.ts index 4d33c124..169f138f 100644 --- a/packages/cli/src/output.ts +++ b/packages/cli/src/output.ts @@ -4,6 +4,7 @@ import { ERROR_DOCS_URL, renderThrowable, singleLine, stringField } from '@ultimat3/core'; import { msg } from './messages'; +import type { TestCounts } from './test-counts'; export interface Finding { readonly code: string; @@ -24,6 +25,13 @@ export interface StepResult { readonly output?: string; /** Worker processes the step used; `1` means it ran serially. Absent for a non-test step. */ readonly workers?: number; + /** + * What the step's suite executed. Absent for a step that spawned no test process, which is NOT + * the same state as `{ ran: 0 }` — a step with no suite and a suite whose every test skipped + * itself both report `skipped`, and this is the only thing that tells a reader which one it is + * looking at (#434). + */ + readonly tests?: TestCounts; } export type JsonValue = @@ -164,10 +172,27 @@ const width = (step: StepResult): string => { return ` ${step.workers === 1 ? msg('cli.verify.serial') : msg('cli.verify.workers', { workers: step.workers })}`; }; +/** + * Why a step is a dash, when the step itself can say. `- roadmap` is "there is nothing here to + * check"; a step that spawned a suite and executed none of it is a different state, which is what + * issue #434 reported as a green check — so a step carrying counts always says which it is, in one + * of the two shapes its counts can take. The reason each test skipped lives in that test's own + * NAME — `bun test` prints only the counts — so the line points at the suite and `x test ` + * is what prints the names. + */ +const why = (step: StepResult): string => { + const tests = step.tests; + if (step.skipped !== true || tests === undefined) return ''; + if (tests.skipped === 0) return ` ${msg('cli.verify.ranNothing')}`; + return ` ${msg('cli.verify.allSkipped', { skipped: tests.skipped })}`; +}; + export function renderHuman(result: CommandResult, verbose = false): string { const out: string[] = []; for (const step of result.steps ?? []) { - out.push(` ${mark(step)} ${step.name.padEnd(18)} ${step.durationMs}ms${width(step)}`); + out.push( + ` ${mark(step)} ${step.name.padEnd(18)} ${step.durationMs}ms${width(step)}${why(step)}`, + ); for (const finding of step.findings) out.push(renderFinding(finding, ' ')); // NOT escaped, and that is the one exception: `output` is this process's own captured // subprocess stdout — `bun test`'s colour is the reason a human reads it at all, and it is @@ -196,6 +221,9 @@ export function renderJson(result: CommandResult): string { skipped: step.skipped === true, findings: step.findings, ...(step.workers === undefined ? {} : { workers: step.workers }), + // The counts the human line's `why()` renders, as numbers: a `--json` reader deciding whether + // a skipped lane is missing a suite or missing a prerequisite needs the same fact CI's log has. + ...(step.tests === undefined ? {} : { tests: step.tests }), // A FAILED step carries its captured stdout, exactly as the human renderer prints it. CI runs // `--json`, and without this the log said only "one or more unit tests failed" with a generic // fix line — the failing test's name and its assertion diff existed and were thrown away, so diff --git a/packages/cli/src/templates/scaffold-repo.ts b/packages/cli/src/templates/scaffold-repo.ts index 61c19ed6..b58b820b 100644 --- a/packages/cli/src/templates/scaffold-repo.ts +++ b/packages/cli/src/templates/scaffold-repo.ts @@ -295,7 +295,9 @@ const biome = (): string => `{ * * Two remain absent. `e2e` has a scaffolded file and it is an `e2eTest` — `test.skip` until the app * registers a browser driver, so the step would run zero tests and fail the ratchet on the - * scaffold's own placeholder. `contract-diff` needs a committed `x.manifest.json`, which + * scaffold's own placeholder. Its absence is visible rather than silent: a suite that executed + * nothing reports as a SKIPPED step, not the green check it printed until #434, so the app's own + * gate line says `- e2e` and names it in the summary until a driver makes it run. `contract-diff` needs a committed `x.manifest.json`, which * `x manifest` writes later. Each joins the list in the commit that makes the app's gate run it. */ const SCAFFOLD_FLOOR: readonly VerifyStepName[] = [ diff --git a/packages/cli/src/verify-run.test.ts b/packages/cli/src/verify-run.test.ts index 262b4e74..93006f09 100644 --- a/packages/cli/src/verify-run.test.ts +++ b/packages/cli/src/verify-run.test.ts @@ -10,10 +10,11 @@ import { tmpdir } from 'node:os'; // why: Bun exposes no path-join primitive; Bun.file and import() take one already joined. import { join } from 'node:path'; import { msg } from './messages'; -import { exitCodeFor } from './output'; +import { exitCodeFor, renderHuman, renderJson } from './output'; import { VERIFY_FLOOR_FILE } from './verify-floor'; import { runVerify } from './verify-run'; import type { VerifyContext, VerifyStep } from './verify-step'; +import { resetTestDiscovery, TEST_STEPS } from './verify-tests'; /** The banner a narrowed run carries, from the catalog that renders it — never a second literal. */ const NOT_A_GATE_RUN = msg('cli.verify.notAGateRun', { summary: '' }).trim(); @@ -169,3 +170,84 @@ describe('skips are counted apart from passes, and named', () => { expect(result.data).toMatchObject({ failed: ['drift'], skipped: ['e2e'] }); }); }); + +// Issue #434: `x verify`'s `e2e` step printed `✓ e2e 46ms` and `"skipped": false` over a suite +// whose only test skipped itself, because `bun test` exits 0 on a skip and the exit code was the +// only thing the step's `ok` was read from. A gate that cannot tell a lane that ran from a lane +// that did not is the one failure the step table exists to prevent, and it is the SAME question +// the floor already answers one layer up (`X_VERIFY_SUITE_VANISHED`): the counts, not the code. +describe('a suite that executed nothing is a skip, never a pass', () => { + // Bun's own summary for the scaffold's one e2e test on a box with no browser driver, verbatim. + // The reason lives in the test's NAME, which bun does not print — so the counts are the only + // channel, and this is exactly the output the gate had to read to be wrong about it. + const ALL_SKIPPED = ' 0 pass\n 1 skip\n 0 fail\nRan 1 test across 1 file. [46.00ms]'; + const ONE_RAN = ' 1 pass\n 0 skip\n 0 fail\nRan 1 test across 1 file. [46.00ms]'; + + const suite = + (stdout: string): VerifyContext['runner'] => + async (command) => ({ + command, + code: 0, + ok: true, + stdout, + stderr: '', + durationMs: 46, + }); + + /** The real `e2e` step, over a root holding the file the scaffold writes for a new route. */ + const e2eOver = async (stdout: string, root: string) => { + await Bun.write(join(root, 'apps/web/app/posts/page.e2e.test.ts'), '// e2e\n'); + resetTestDiscovery(); + const step = TEST_STEPS.find((candidate) => candidate.name === 'e2e'); + if (step === undefined) return expect.unreachable('the e2e step is registered'); + return runVerify([...STEPS.slice(0, 2), step], { root, runner: suite(stdout) }); + }; + + const inTempRoot = async (run: (root: string) => Promise): Promise => { + const root = await mkdtemp(join(tmpdir(), 'x-verify-e2e-')); + try { + await run(root); + } finally { + resetTestDiscovery(); + await rm(root, { recursive: true, force: true }); + } + }; + + test('the step reports itself skipped, the way roadmap does', async () => { + await inTempRoot(async (root) => { + const result = await e2eOver(ALL_SKIPPED, root); + const e2e = result.steps?.find((step) => step.name === 'e2e'); + expect(e2e?.skipped).toBe(true); + expect(dataOf(result.data).skipped).toEqual(['e2e']); + }); + }); + + // Both renderers, because `--json` is what CI reads and the check mark is what a human reads, + // and the defect was visible in each. + test('neither renderer claims it passed', async () => { + await inTempRoot(async (root) => { + const result = await e2eOver(ALL_SKIPPED, root); + expect(result.summary).toContain('1 skipped: e2e'); + expect(renderHuman(result)).toContain('- e2e'); + expect(renderHuman(result)).not.toContain('✓ e2e'); + const payload = JSON.parse(renderJson(result)) as { + readonly steps: readonly { readonly name: string; readonly skipped: boolean }[]; + }; + expect(payload.steps.find((step) => step.name === 'e2e')).toMatchObject({ + skipped: true, + ok: true, + tests: { ran: 0, skipped: 1 }, + }); + }); + }); + + // The other side of the enumeration: this may not turn a suite that ran into a skip. + test('one test that actually ran keeps the step a pass', async () => { + await inTempRoot(async (root) => { + const result = await e2eOver(ONE_RAN, root); + expect(result.steps?.find((step) => step.name === 'e2e')?.skipped).not.toBe(true); + expect(dataOf(result.data).skipped).toEqual([]); + expect(result.ok).toBe(true); + }); + }); +}); diff --git a/packages/cli/src/verify-run.ts b/packages/cli/src/verify-run.ts index c49dbb68..d55b34da 100644 --- a/packages/cli/src/verify-run.ts +++ b/packages/cli/src/verify-run.ts @@ -53,20 +53,29 @@ export async function runVerify( findings: [findingOf(error, step.name)], }), ); + // A suite that executed nothing did not run, whatever its exit code says: `bun test` exits 0 + // over an all-skipped file, so the counts are the only channel that can tell the two apart. + // ONE definition of "nothing ran", read twice, because the floor decides which of the two + // things it means — exactly as it already does for a step whose `applies` said no. + const tests = outcome.tests; + const nothingRan = tests !== undefined && tests.ran === 0; + const required = floorRequires(floor, step.name); // A step the floor requires whose suite executed nothing is the same vanished suite as a step // with no files at all — the run just had to finish before it could be seen. Appended to the // step's own findings so `data.failed`, the counts and every gate reading this table carry it. - const vanished = - floorRequires(floor, step.name) && outcome.tests !== undefined && outcome.tests.ran === 0 - ? [skippedSuiteFinding(step.name, outcome.tests.skipped)] - : []; + const vanished = nothingRan && required ? [skippedSuiteFinding(step.name, tests.skipped)] : []; results.push({ name: step.name, ok: outcome.ok && vanished.length === 0, durationMs: Math.round(performance.now() - started), + // Without a floor to require it, a suite that ran nothing is a SKIP and not a pass (#434): + // the `e2e` step printed `✓ e2e 46ms` over its one skipped test, which is the one thing a + // step table may never do — a reader cannot tell a lane that ran from a lane that did not. + skipped: nothingRan && !required, findings: [...outcome.findings, ...vanished], ...(outcome.output === undefined ? {} : { output: outcome.output }), ...(outcome.workers === undefined ? {} : { workers: outcome.workers }), + ...(tests === undefined ? {} : { tests }), }); } const failedSteps = results.filter((step) => !step.ok).map((step) => step.name); diff --git a/packages/testing/src/test-types.ts b/packages/testing/src/test-types.ts index 0304186d..0a505ec3 100644 --- a/packages/testing/src/test-types.ts +++ b/packages/testing/src/test-types.ts @@ -96,13 +96,14 @@ let e2eDriver: ((name: string, body: E2eBody) => void) | undefined; * Register the browser-backed driver. Without one, `e2eTest` skips loudly — the skipped test's own * NAME carries the reason and the command that would build what it drives. * - * What the gate then reports is a **pass over an all-skipped suite**, and that is stated here - * rather than claimed away: this used to say "`x verify` reports the step as skipped rather than - * green", which nothing implements. The step shells out to `bun test`, `bun test` exits 0 on a - * skip, and an exit code is the only channel between the two — the driver is registered inside the - * CHILD process, so the step cannot ask. Closing it means a channel the step can read, which is a - * design decision and not a docstring. `As of 2026-08` there are zero registered drivers, so every - * `e2eTest` in the tree is a skip; the framework's own `e2e` suites use plain `bun:test`. + * The gate's rule over that skip: a step whose suite reports `ran === 0` is SKIPPED, unless the + * repo's `x.verify.json` requires the step — then it is `X_VERIFY_SUITE_VANISHED` + * (`packages/cli/src/verify-run.ts`). Never a pass either way. This block used to say the opposite + * and argue for it — that an exit code was the only channel, so the gate could not tell — which + * `packages/cli/src/test-counts.ts` had already disproved by reading `bun test`'s own counts (#434). + * + * `As of 2026-08` there are zero registered drivers, so every `e2eTest` in the tree is a skip; the + * framework's own `e2e` suites use plain `bun:test`. */ export function useE2eDriver(driver: (name: string, body: E2eBody) => void): void { e2eDriver = driver;