From 3f699947c9f7285fa039eaf7949f3116697e264c Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 18:19:30 +0000 Subject: [PATCH 1/6] fix(pregate): runner detection for pytest, Playwright and mixed-language roots; literals binds one-character values A missing pytest is PREGATE_ENVIRONMENT, not the diff's failure (#3598). Playwright specs run under playwright test, with missing browsers reported as the machine's gap (#3629). Root tests in two languages each get their runner, and a repo's bun/vitest/jest claims only JS/TS (#3600). literals counts a one-character old value only in a statement naming the constant, and a call-site name also needs the module (#3492, gstack-evals#97). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gsta-212-pregate-runners.md | 10 ++ docs/pregate.md | 12 ++- docs/state-root.md | 4 +- lib/pregate/checks/lanes.ts | 80 ++++++++++++--- lib/pregate/checks/literals.ts | 22 +++- lib/pregate/init-draft.ts | 3 +- test/pregate-environment.test.ts | 78 +++++++++++++++ test/pregate-non-js.test.ts | 2 +- test/pregate-selection.test.ts | 128 ++++++++++++++++++++++++ 9 files changed, 316 insertions(+), 23 deletions(-) create mode 100644 changelog.d/gsta-212-pregate-runners.md diff --git a/changelog.d/gsta-212-pregate-runners.md b/changelog.d/gsta-212-pregate-runners.md new file mode 100644 index 000000000..f641a61b8 --- /dev/null +++ b/changelog.d/gsta-212-pregate-runners.md @@ -0,0 +1,10 @@ +**Pregate stops blaming a clean branch for a missing pytest, Playwright specs and mixed-language root tests, and the literals check stops flagging stray digits (#3598, #3492, gstack-evals#97).** A machine without pytest reported "fix the in-branch failure". Playwright specs in a vitest repo went to vitest, which found no tests and failed. Root tests in `.mjs` and `.py` got no runner at all. And any lone `2` in a file that named a changed constant (a comment, a `$2` in a shell stub, a fixture) failed `literals`. A wrapper bump from 3 to 4 would have raised 5 false hits here, and gstack-evals#97 hit 11. Rewritten from outside PRs #3639 and #3600 by @vilano93 and #3629 by @nfitzsimmons. + +### Fixed +- **Missing pytest is the machine's gap.** Before planning `python3 -m pytest`, `lanes` asks that python3 whether it can import pytest (10 s probe, from the repo root). If it can't, the file is unclaimed and the lane is `PREGATE_ENVIRONMENT` (missing-binary, `pytest` or `python3`): "install pytest on this machine and rerun, or run the lane remotely; the diff is not at fault". +- **Playwright specs run under Playwright.** A JS/TS test file that imports `@playwright/test` at the start of a line runs as ` playwright test --reporter=list` from its nearest package. Other tests keep their runner. A declared `runner` still takes every file. Playwright's summary is read count line by count line: failed and interrupted count as failed, flaky and passed as passed, skipped as not run. A run where every failure is a browser Playwright has not installed is `PREGATE_ENVIRONMENT` naming `npx playwright install`. +- **Mixed-language root tests each get a runner.** A repo's bun, vitest or jest now claims only JS/TS files. When no one runner fits every root test, they are split by language and each part is detected on its own. So `.mjs` with `.py` runs `node --test` and pytest, and a `.py` test in a vitest repo gets pytest instead of vitest. `gstack-pregate init` drops its "a selection mixing these languages needs runner set" note. +- **`literals` binds a one-character old value to its statement.** The value counts on a line that names the constant, or on up to three lines that continue that statement, never on a comment line. Version-history rows count as before. A lowercase call-site name (`attempts`, `limit`) also needs the changed module named in the file. Values of two or more characters are unchanged. + +### Changed +- **docs/pregate.md** describes the new detection and the literals binding. **docs/state-root.md** lists `pregate/runs//`. diff --git a/docs/pregate.md b/docs/pregate.md index 98a1caa0b..7dfe06b7d 100644 --- a/docs/pregate.md +++ b/docs/pregate.md @@ -74,7 +74,7 @@ the byte-pinned `/ship` renders in `test/fixtures/golden/`). | Key | Meaning | |---|---| -| `runner` | the local test command with `{files}` (gstack: `bun test {files}`); unset, it is detected (bun, vitest, jest, pytest, `node --test`, and `flutter test`/`dart test` per Dart package root, through fvm when that package or an ancestor pins it with `.fvmrc` or `.fvm/fvm_config.json` and fvm is on PATH; `cargo test -p ` per Rust crate, run from the root of the workspace the crate is a member of (Cargo's `members` globs, `exclude` and `package.workspace`), else the crate directory; a JS/TS test whose nearest package.json is a nested package declaring vitest or jest (a dependency or its `test` script) runs from that package, with package-relative paths, through the package manager its `packageManager` field or the nearest lockfile up to the repo root names: `pnpm exec`, `yarn`, `bun x`, else `npx`; every other JS/TS test keeps the root detection); nothing detected, a Dart SDK, cargo or package manager missing from PATH, a `.dart` file with no `pubspec.yaml` above it, a `.rs` file outside any crate, or one in a crate whose name Cargo would reject (anything but letters, digits, `_` and `-`), is `incomplete` (the runnable groups still run, so a real failure still fails) | +| `runner` | the local test command with `{files}` (gstack: `bun test {files}`); unset, it is detected (bun, vitest or jest for JS/TS files, pytest when python3 can import it (else `pytest` or `python3` is named missing), `node --test`, and `flutter test`/`dart test` per Dart package root, through fvm when that package or an ancestor pins it with `.fvmrc` or `.fvm/fvm_config.json` and fvm is on PATH; `cargo test -p ` per Rust crate, run from the root of the workspace the crate is a member of (Cargo's `members` globs, `exclude` and `package.workspace`), else the crate directory; a JS/TS test whose nearest package.json is a nested package declaring vitest or jest (a dependency or its `test` script) runs from that package, with package-relative paths, through the package manager its `packageManager` field or the nearest lockfile up to the repo root names: `pnpm exec`, `yarn`, `bun x`, else `npx`; a JS/TS test file that imports `@playwright/test` at the start of a line runs as `playwright test --reporter=list` from its nearest package through that same package manager, whatever the unit runner is, and a run where every failure is a browser Playwright has not installed is `PREGATE_ENVIRONMENT`; every other test keeps the root detection, split by language when no one runner fits them all, so `.mjs` and `.py` root tests each get their own runner); nothing detected, a Dart SDK, cargo or package manager missing from PATH, a `.dart` file with no `pubspec.yaml` above it, a `.rs` file outside any crate, or one in a crate whose name Cargo would reject (anything but letters, digits, `_` and `-`), is `incomplete` (the runnable groups still run, so a real failure still fails) | | `dependencies` | glob → test files for subprocess targets, runtime reads and generated inputs the import graph cannot see; `[]` declares "no lane needed" (docs, licences) | | `empty_lanes` | extra globs for the docs/no-tests empty lane. Any Markdown file already qualifies with no pin file when no test depends on it (the same import graph as other files: a quoted path, a `join()` of path segments, an import, or a `dependencies` entry). reStructuredText and AsciiDoc count as Markdown here. README, CHANGELOG, CONTRIBUTING, LICENSE, AGENTS and CLAUDE qualify even without `.md`, and so does licence text under any vendored name (`roboto-LICENSE.txt`, `LICENSE-MIT`, `licenses/*.txt`). Source code and files under a test directory never qualify. A test that reads the file still selects that test | | `selection` | `workflow.yml/job` → a command printing the lane's file list, for matrices a planner computes (gstack: `scripts/test-free-shards.ts --list`) | @@ -193,6 +193,14 @@ Two stages with separate clocks, both printed: other line holding the old value still fail. Only the row itself is exempt: the old value after its closing bracket on the same line still fails. Rows in object form (`{ v: 45 }`) or spread over several lines are not recognized as history rows. + A one-character old value (`2`) counts only in a statement that names the + constant: on the name's line, or on up to three lines that continue it (the + line before ends with `(`, `,`, `.`, `=` or another operator, or the line + starts with `.`, `)` or `]`), never on a comment line, plus the history rows + above. A `$2` in a shell stub, a comment, or `export default 2` in a fixture + elsewhere in the file is not a consumer. A call-site name such as `attempts` + or `limit` is an ordinary word, so for it the file must also name the changed + module. - `timing`: added or changed test lines that compare an elapsed wall-clock or CPU time against a bound, or sleep and then assert, print @@ -409,7 +417,7 @@ test-tier command: Per-file lines come from a progress adapter where the runner's reporter provides them: `bun test` (its `:` headers) is the only adapter today; a one-file run (each serial file) reports its file from the launch for any - runner. Other runners (node --test, vitest, jest, pytest, flutter test, dart test, cargo test) get heartbeats + runner. Other runners (node --test, vitest, jest, pytest, playwright test, flutter test, dart test, cargo test) get heartbeats only. A heartbeat follows each 60 s with no progress line; - is killed as **stalled** when its log does not grow for the stall timeout (`--stall-timeout `, else `stall_timeout_s`, else 300; `0` disables): the diff --git a/docs/state-root.md b/docs/state-root.md index 376261884..4dc6cb862 100644 --- a/docs/state-root.md +++ b/docs/state-root.md @@ -105,7 +105,9 @@ lives in `projects//`. `bin/gstack-slug` picks the slug from the git - Without a remote, the project root's directory name is used. A bucket can also hold a user-owned `pregate.json` policy, for a repo that keeps -`.gstack/` out of git (see [pregate v1](pregate.md#pregate-v1)). +`.gstack/` out of git (see [pregate v1](pregate.md#pregate-v1)), and +`pregate/runs//`, the scratch each pregate run writes (lane and guard +logs, the remote gate's file list); old runs are pruned when a new one starts. The first slug gstack resolves in a directory sticks (it is cached under `slug-cache/`), so a project keeps its bucket when it adds a remote later. diff --git a/lib/pregate/checks/lanes.ts b/lib/pregate/checks/lanes.ts index f1206404c..1879435be 100644 --- a/lib/pregate/checks/lanes.ts +++ b/lib/pregate/checks/lanes.ts @@ -22,6 +22,7 @@ * Incidents: p0-4-pipe-hidden-failure (`false | tail` read as green), * p0-4-windows-lane-unrun (a Windows-only change merged with its lane unrun). */ +import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as path from 'node:path'; import { baseInputs, sha256 } from '../context'; @@ -37,17 +38,21 @@ import { adapterFor, readRange, supervise, type Counter, type Supervised } from import { matchesAny } from '../../ship-policy'; import type { CheckDef, CheckResult, PregateContext, RemoteObligation } from '../types'; -/** The test command with `{files}`; detected from the repo when the config does not name one. */ +/** The runner a `.py`-only selection gets; it needs the pytest module importable by python3 (`pytestMissing`). */ +const PYTEST = 'python3 -m pytest {files}'; + +/** The test command with `{files}`; detected from the repo when the config does not name one. The repo's bun, vitest or jest claims JS/TS files only. */ export function detectRunner(ctx: Pick, files: readonly string[]): string | null { if (ctx.config.runner) return ctx.config.runner; const has = (f: string) => fs.existsSync(path.join(ctx.repoRoot, f)); let pkg: { devDependencies?: Record; dependencies?: Record; scripts?: Record } = {}; try { pkg = JSON.parse(fs.readFileSync(path.join(ctx.repoRoot, 'package.json'), 'utf8')); } catch { /* no package.json */ } const deps = { ...pkg.dependencies, ...pkg.devDependencies }; - if (has('bunfig.toml') || has('bun.lock') || has('bun.lockb')) return 'bun test {files}'; - if (deps.vitest) return 'npx vitest run {files}'; - if (deps.jest) return 'npx jest {files}'; - if (files.every(f => /\.py$/.test(f)) && files.length) return 'python3 -m pytest {files}'; + const jsts = files.every(f => /\.[cm]?[jt]sx?$/.test(f)); + if (jsts && (has('bunfig.toml') || has('bun.lock') || has('bun.lockb'))) return 'bun test {files}'; + if (jsts && deps.vitest) return 'npx vitest run {files}'; + if (jsts && deps.jest) return 'npx jest {files}'; + if (files.every(f => /\.py$/.test(f)) && files.length) return PYTEST; if (files.every(f => /\.[cm]?js$/.test(f)) && files.length) return 'node --test {files}'; if (files.every(f => /\.[cm]?[jt]sx?$/.test(f)) && files.length && Bun.which('bun')) return 'bun test {files}'; return null; @@ -107,6 +112,18 @@ export function dartRunner(repoRoot: string, root: string, env: NodeJS.ProcessEn return { runner: `${fvm ? 'fvm ' : ''}${tool} test {files}` }; } +/** + * The tool `python3 -m pytest` lacks: `python3` off the PATH of `env`, or `pytest` when that python3, run from the repo root as the + * runner is, cannot import it (a `pytest` script on PATH may belong to another interpreter); null when both are there, or when the + * probe itself times out or dies, so the run decides. + */ +function pytestMissing(repoRoot: string, env: NodeJS.ProcessEnv): string | null { + const python = Bun.which('python3', { PATH: env.PATH ?? '' }); + if (!python) return 'python3'; + const r = spawnSync(python, ['-c', 'import importlib.util, sys; sys.exit(importlib.util.find_spec("pytest") is None)'], { cwd: repoRoot, env, stdio: 'ignore', timeout: 10_000 }); + return r.status === 1 ? 'pytest' : null; +} + /** * `cargo test -p ` for a crate (cargo runs a crate's tests, never a file list), or the missing tool when cargo is * not on PATH. The name comes from the repo's Cargo.toml and reaches a shell and the `{files}` expansion: a name outside @@ -157,6 +174,11 @@ function packageRunner(repoRoot: string, dir: string, env: NodeJS.ProcessEnv): { return { runner: `${PM_EXEC[pm]} ${tool} {files}` }; } +/** A JS/TS test that imports Playwright's runner at the start of a line (a string or a fixture body that mentions it mid-line does not count). */ +const PLAYWRIGHT_IMPORT = /^[\t ]*(?:import\b[^'"`\n]*?\bfrom|\}[\t ]*from|import|(?:const|let|var)\b[^=\n]*=[\t ]*require\()[\t ]*['"]@playwright\/test['"]/m; +/** Playwright's own text when its browsers are not installed on this machine. */ +const PLAYWRIGHT_NO_BROWSERS = /^.*(?:Executable doesn't exist at|download new browsers).*$/m; + /** The run plan: the groups that can run, the files no runner claims, tools missing from PATH, and `.dart` files with no pubspec.yaml above. */ export interface RunPlan { groups: RunGroup[]; unclaimed: string[]; missing: string[]; noPubspec: string[] } @@ -165,8 +187,11 @@ export interface RunPlan { groups: RunGroup[]; unclaimed: string[]; missing: str * Dart tests run from their own package root (a sub-package's imports resolve * against its own pubspec); Rust tests run per crate as `cargo test -p ` * from the crate's workspace root; JS/TS tests owned by a nested package that - * declares vitest or jest run from that package; the rest go to the detected - * runner from the repo root. Files no runner claims (a tool missing from PATH, + * declares vitest or jest run from that package; a JS/TS test that imports + * `@playwright/test` runs through `playwright test` from its nearest package; + * the rest go to the detected runner from the repo root, split by language + * when no one runner fits them all (`.mjs` + `.py` runs both), and pytest only + * when python3 can import it. Files no runner claims (a tool missing from PATH, * a `.dart` file outside any package, a `.rs` file outside any crate or in a * crate whose name Cargo would reject, no detected runner) are `unclaimed`: the lane is `incomplete`, never a pass. */ @@ -176,6 +201,7 @@ export function planRuns(ctx: PregateContext, files: readonly string[]): RunPlan const dart = new Map(); const cargo = new Map(); const packages = new Map(); + const playwright = new Map(); const seen = new Map(); const seenPackage = new Map(); let model: CargoModel | null = null; @@ -198,6 +224,7 @@ export function planRuns(ctx: PregateContext, files: readonly string[]): RunPlan continue; } const pkg = /\.[cm]?[jt]sx?$/.test(f) ? nearestRoot(ctx.repoRoot, f, 'package.json', seenPackage) : null; + if (pkg !== null && PLAYWRIGHT_IMPORT.test(readRepoFile(ctx.repoRoot, f) ?? '')) { add(playwright, pkg, under(pkg, f)); continue; } if (pkg) { if (!pkgRunners.has(pkg)) pkgRunners.set(pkg, packageRunner(ctx.repoRoot, pkg, ctx.env)); const r = pkgRunners.get(pkg); @@ -219,12 +246,24 @@ export function planRuns(ctx: PregateContext, files: readonly string[]): RunPlan if (r) missing.add(r.missing); unclaimed.push(...c.files.map(f => (c.cwd ? `${c.cwd}/${f}` : f))); } - if (rest.length) { - const runner = detectRunner(ctx, rest); - if (runner) groups.push({ runner, cwd: '', files: rest }); - else unclaimed.push(...rest); + const language = (f: string) => (/\.py$/.test(f) ? 'py' : /\.[cm]?js$/.test(f) ? 'js' : /\.[cm]?[jt]sx?$/.test(f) ? 'ts' : f); + const parts = !rest.length ? [] : detectRunner(ctx, rest) ? [rest] : [...Map.groupBy(rest, language).values()]; + const byRunner = new Map(); + for (const part of parts) { + const runner = detectRunner(ctx, part); + const lacking = runner === PYTEST ? pytestMissing(ctx.repoRoot, ctx.env) : null; + if (lacking) missing.add(lacking); + if (runner && !lacking) for (const f of part) add(byRunner, runner, f); + else unclaimed.push(...part); } + for (const [runner, list] of byRunner) groups.push({ runner, cwd: '', files: list }); for (const [root, list] of [...packages].sort(([a], [b]) => a.localeCompare(b))) groups.push({ runner: (pkgRunners.get(root) as { runner: string }).runner, cwd: root, files: list }); + for (const [root, list] of [...playwright].sort(([a], [b]) => a.localeCompare(b))) { + const pm = packageManager(ctx.repoRoot, root); + const bin = pm === 'npm' ? 'npx' : pm; + if (onPath(bin, ctx.env)) groups.push({ runner: `${PM_EXEC[pm]} playwright test --reporter=list {files}`, cwd: root, files: list }); + else { missing.add(bin); unclaimed.push(...list.map(f => (root ? `${root}/${f}` : f))); } + } return { groups, unclaimed: [...unclaimed, ...noPubspec], missing: [...missing].sort(), noPubspec }; } @@ -263,11 +302,20 @@ function parseCargo(output: string): RunnerSummary | null { return { ran: lines.reduce((n, m) => n + Number(m[1]), 0) + failed, failed }; } -/** The runner's own summary line: bun, node --test, vitest, jest, pytest, bats, dart/flutter test, cargo test. A bats, dart/flutter or cargo runner's summary is tried first (cargo's only there: its `N passed` lines would misread as another runner's); for any other runner the bats and dart formats are a last resort. `ran: null` when not recognised. Colour codes are dropped and a carriage return ends a line, so a coloured or redrawn verdict still reads. */ +/** playwright test: its `N failed`, `N interrupted`, `N flaky`, `N passed` and `N skipped` lines, each on its own line with test names between them; skipped tests did not run. */ +function parsePlaywright(output: string): RunnerSummary | null { + if (!/^\s*\d+ (?:passed|failed|flaky|interrupted|skipped)\b/m.test(output)) return null; + const count = (word: string) => Number(new RegExp(`^\\s*(\\d+) ${word}\\b`, 'm').exec(output)?.[1] ?? 0); + const failed = count('failed') + count('interrupted'); + return { ran: count('passed') + count('flaky') + failed, failed }; +} + +/** The runner's own summary line: bun, node --test, vitest, jest, pytest, bats, dart/flutter test, cargo test, playwright test. A bats, dart/flutter, playwright or cargo runner's summary is tried first (cargo's only there: its `N passed` lines would misread as another runner's); for any other runner the bats and dart formats are a last resort. `ran: null` when not recognised. Colour codes are dropped and a carriage return ends a line, so a coloured or redrawn verdict still reads. */ export function parseSummary(raw: string, runner = ''): RunnerSummary { const output = raw.replace(/\x1b\[[0-9;?]*[A-Za-z]/g, '').replace(/\r/g, '\n'); if (/(? `'${f.replace(/'/g, `'\\''`)}'`; @@ -293,7 +341,8 @@ async function runOnce(ctx: PregateContext, env: NodeJS.ProcessEnv, g: RunGroup, const r = await supervise({ command: run, cwd: path.join(ctx.repoRoot, g.cwd), env: { ...env, CI: env.CI ?? '1' }, log, deadline: Date.now() + ctx.timeoutMs, stallMs: ctx.stallTimeoutMs, label: `lanes (${shown.length === 1 ? shown[0] : `${shown.length} files`})`, files, adapter: adapterFor(g.runner), counter, progress: ctx.progress }); const output = readRange(log, r.start, r.end); const summary = parseSummary(output, g.runner); - return { exit: r.exit, outcome: r.outcome, signal: r.signal, observedStart: r.observedStart, ...summary, tail: output.trim().split('\n').slice(-25), runner: g.runner, command, files: shown, lastFile: r.lastFile && g.cwd ? `${g.cwd}/${r.lastFile}` : r.lastFile, ms: r.ms, ...(r.error ? { error: r.error } : {}) }; + const noBrowsers = /(? x.exit !== 0); + if (failedRuns.every(x => x.noBrowsers && x.failed !== null && x.failed === x.ran)) return environmentResult(withRun, { kind: 'missing-binary', evidence: `Playwright browsers not installed (npx playwright install): ${failedRuns[0]!.noBrowsers}`, proof: 'output' }, `every Playwright test failed to launch its browser (${r.failed} of ${r.ran}; ${total} files)`); return { ...withRun, status: 'fail', cause: 'diff', code: 'PREGATE_LANE_FAILED', detail: `local lane exited ${r.exit} (${r.failed ?? '?'} failed of ${r.ran ?? '?'} ran; ${total} files; pipefail)`, fix: `read ${r.log}, fix the in-branch failure, rerun gstack-pregate --stage tests` }; } if (r.ran === 0) return { ...withRun, status: 'fail', cause: 'diff', code: 'PREGATE_LANE_FAILED', detail: `ZERO-RUN: the runner reported 0 tests${r.zero ? ` in ${r.zero}` : ''} (${total} selected file(s))`, fix: 'a bad selector, a missing file or a skipped describe; exit 0 is not a pass' }; diff --git a/lib/pregate/checks/literals.ts b/lib/pregate/checks/literals.ts index a769626e4..4f2cc1622 100644 --- a/lib/pregate/checks/literals.ts +++ b/lib/pregate/checks/literals.ts @@ -65,7 +65,12 @@ function unquote(v: string): string { return /^['"`]/.test(v) ? v.slice(1, -1) : * path fragment (`/retry.js`, `/retry'`): a bare stem such as `install` or `retry` is an * English word in half the test tree, and a one-character old value (`2`) * sits in every test, so that value needs the constant's name (a wrapper - * version 2 → 3 flagged 575 consumers through the stem alone). + * version 2 → 3 flagged 575 consumers through the stem alone), and counts only + * in a statement that names it: on the name's line, or on a line that + * continues it (the line before ends open, `(` `,` `.` `=`, or the line + * starts with `.` `)` `]`), comments excluded (#3492: `$2`, "from revision 2", + * `export default 2`). A call-site name (`attempts`, `limit`) is an ordinary + * word, so for it the file must also name the module (gstack-evals#97). * * A version-history row (`[45, ""]`, the old value first in a * bracketed row) is skipped when the same table, a run of consecutive @@ -86,12 +91,23 @@ export function consumersOf(repoRoot: string, testFiles: readonly string[], chan const moduleRe = new RegExp(`(?:(?:^|[/\\\\])${escape(stem)}|(? /^\s*(?:\/\/|#|\*|\/\*|