Repository navigation
ci: a spec-only pull request runs the tests that read a record it changes (AST-030 FR2) - #9446
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
cixzhang
changed the title
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 3b8771f |
github-actions
Bot
removed
the
needs:spec-owner-review
PR Analysis Report🔎 Preview ·
|
| Before | After | Change | |
|---|---|---|---|
| Job-minutes per PR update | 65.6 | 74.2 | +8.6 |
| Runner pool, runner-minutes a day | 27,180 | 30,721 | +3,542 (+13%) |
| Average concurrent jobs | 18.9 | 21.3 | +2.5 |
| Time to green (median → this run) | 15.7 min | 15.4 min | −0.3 min |
| Runs a day | 414 | 414 | 0 |
Changed jobs: test-node changed: standard · test-ui changed: standard
Not measured (the jobs that use these keep their recent minutes above): .github/scripts/ci-test-routing.test.mjs, .github/scripts/fs-guard-tests.mjs, .github/scripts/fs-guard-tests.test.mjs.
Flags: runner-pool usage +13%.
Assumptions: before is each job's average over the last 20 pull-request runs; after keeps that for unchanged jobs and uses this run's minutes for added or changed ones, weighted by how often each changed job ran in those runs; runs a day averages the seven whole days before today; time to green compares this run with the median of those runs that ran the same jobs.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the spec-reader lane is a good fit for AST-030 FR2, but it still misses some spec-only changes it claims to cover.
- Spec-only changes to family, design and theme records list no readers, because
specReaderTestskeeps onlydocs/specs/paths; extend it to every spec-only record path, or narrow FR2 and the description todocs/specs/. git diff --name-onlyreports only the new name of a renamed record, so a rename of AST-094's directory dropsroles.test.ts; use--no-renamesin--spec-readersmode and add a rename case.test-uiruns setup on everydocs/specs/change even when no UI test is listed; gate each leg on its own project's files, and add FR19's runner-pool usage to the CI impact line.
[Automated review]
| const changedSpecs = | ||
| changedFiles == null | ||
| ? null | ||
| : changedFiles.filter(file => file.startsWith(SPEC_ROOT)); |
There was a problem hiding this comment.
The trusted classifier's spec-only lane also admits docs/families/*.md, docs/design/*.md and packages/themes/<t>/<t>.spec.md (change-scope.cjs:22-27). This filter drops them, so a family-only change lists no readers, even though scripts/check-knowledge.test.mjs reads those records through validateKnowledgeRoot(repoRoot). Either count every spec-only record path (and recognise those roots in SPEC_LITERAL), or narrow FR2's owner text to docs/specs/ records.
| id: spec-readers | ||
| if: matrix.shard == 1 && steps.scope.outputs.spec_only == 'true' | ||
| run: | | ||
| node .github/scripts/fs-guard-tests.mjs --spec-readers --changed HEAD^1 > "$RUNNER_TEMP/spec-readers.txt" |
There was a problem hiding this comment.
git diff --name-only HEAD^1 HEAD (inside the lister) reports only the new name of a renamed record. Renaming docs/specs/AST-094-touch-presentations/ therefore lists five whole-tree readers but not roles.test.ts, which reads the old path and then fails on main. Passing --no-renames in --spec-readers mode lists both names.
| fi | ||
|
|
||
| - uses: ./.github/actions/setup | ||
| if: steps.spec-readers.outputs.any == 'true' |
There was a problem hiding this comment.
any covers both projects' files. Every docs/specs/ change lists the five whole-tree Node readers, so this leg runs setup and then pnpm test:ui finds no test, unless AST-094 changed. Gating on this project's own files (for example, the UI include roots under packages/{core,lab,charts,richtext,vega}/src/) avoids that setup.
… renamed record under both names, and run per Vitest project
|
/approve-spec 0287643 |
There was a problem hiding this comment.
Requesting changes: family, design, theme and renamed records now reach their readers, and the per-project split removes the idle UI setup, but two gaps remain in the lister.
- No test runs
--project: inverting the filter infs-guard-tests.mjskeeps every case green, and both legs then find no test files and pass under--passWithNoTests, so add a CLI case per project and drop--passWithNoTestsfrom the reader runs. - A computed prefix that ends inside a path segment, such as
`docs/specs/AST-${id}/spec.md`, is never selected, although the description says computed prefixes count as the whole tree, so cut such prefixes back to the last/or narrow the wording.
[Automated review]
| throw new Error(`--project must be ui or node, not ${project}`); | ||
| const roots = uiTestRoots(read('vitest.config.ts')); | ||
| const inUi = test => roots.some(root => test.startsWith(root)); | ||
| tests = tests.filter(test => inUi(test) === (project === 'ui')); |
There was a problem hiding this comment.
No test runs this filter: the split case recomputes it inline, so flipping === to !== keeps all 32 lister cases green. With that flip, an AST-094 change sends the Node readers to test-ui and roles.test.ts to test-node, and both legs print "No test files found, exiting with code 0" under --passWithNoTests. A CLI contract case per project (as the rename case does), plus dropping --passWithNoTests from the reader runs, would catch it.
| changed === named || | ||
| changed.startsWith(named.endsWith('/') ? named : `${named}/`), |
There was a problem hiding this comment.
A computed prefix that ends inside a path segment, such as `docs/specs/AST-${id}/spec.md` or 'docs/specs/AST-' + id, names docs/specs/AST-. Appending / here means it matches no record, so that reader is listed with no --changed but never selected for a change. Cutting a truncated literal back to its last / (or matching changed.startsWith(named) for computed prefixes) would keep the description's whole-tree rule.
… CLI case, and a computed prefix inside a segment names its directory
|
/approve-spec 2849fa8 |
There was a problem hiding this comment.
Requesting changes: the spec-reader lane now covers every spec-only record kind, renames and the per-project split with mutation-sensitive cases, but one spelling of a record reader is still never run.
- A test that reads records through a directory glob, such as
fs.globSync('docs/specs/**/spec.md')or'docs/specs/*/spec.md', is never selected for a changed record, becausecode()treats the/**/in the string as a comment and the cut-back keeps the*segment; strip only comments that start a line or follow whitespace, cut a literal at its first glob character, and add a glob fixture.
[Automated review]
|
|
||
| /** The source without block comments and whole-line `//` comments, where prose names records. */ | ||
| function code(source) { | ||
| return source.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^\s*\/\/.*$/gm, ''); |
There was a problem hiding this comment.
This also strips /**/ inside string literals, so a reader written fs.globSync('docs/specs/**/spec.md') becomes 'docs/specsspec.md' and is never listed. Stripping only block comments that start a line or follow whitespace keeps it: source.replace(/(^|\s)\/\*[\s\S]*?\*\//g, '$1').
| const namesSpecFile = /\.spec\.md['"`]/.test(body); | ||
| const counts = root => root !== 'packages/themes/' || namesSpecFile; | ||
| for (const [, , literal] of body.matchAll(RECORD_LITERAL)) { | ||
| const fixed = literal.split('${')[0]; |
There was a problem hiding this comment.
A glob literal such as 'docs/specs/*/spec.md' is cut back to docs/specs/*/, which no changed path starts with. Cutting at the first glob character too (literal.split(/\$\{|[*?[{]/)[0]) names docs/specs/; a ** and a */ glob fixture would pin it.
… glob literal at its first wildcard
|
/approve-spec 68d0d61 |
There was a problem hiding this comment.
Recommending approval: readers that name records through a ** or * glob are now selected and pinned by fixtures, and the spec-reader steps run each project's readers of a changed record under the AST-030 FR2 amendment an engineering owner approved at this head. Two optional description fixes: #9423's change lists roles.test.ts among 16 tests rather than 6, and the reader lister shares the guard lister's resolver and patterns but not its traversal.
[Automated review]
Maintainer impact
docs/specs/, family and design contracts, and theme package specs. Some tests read records from disk:packages/core/src/motion/roles.test.tsreads AST-094's FR4 table, andicon-set-checks.test.mjsreads AST-084. A record change can break them, and the break shows up only onmain. spec(ContextMenu): touch-opened menus lift with a tint, grow from the finger, and use named springs #9423 changed AST-094's FR4 table, andmainwent red until it was fixed.test-uiand leg 1 oftest-nodeeach list the tests in their own Vitest project that read a changed record, and run them. Project membership is read fromvitest.config.ts. A leg with no reader adds only the check-out and the listing (about 10 s). A renamed record counts under its old and new names. Every other lane is unchanged.docs/specs/,docs/families/,docs/design/, orpackages/themes/together with a.spec.mdname, so theme-source readers don't count). The path may be relative to the test (../../docs/specs/…) or from the root. Prose in comments doesn't count./. That covers a computed one such asdocs/specs/AST-${id}/spec.md, and a record since renamed. A glob is cut at its first wildcard. So a computed prefix, a glob such asdocs/specs/**/spec.md, orpath.join(…, 'docs', 'specs', …)names the whole tree.roles.test.ts) and 22 Node.Authority
spec:AST-030FR2: a surface uses the broad check set until it has an admitted, complete specialized owner route. A record that tests read is not completely owned by knowledge validation alone. The knowledge row's owners now name "the Vitest tests that read a changed record, directly or through a module they import", which completes the route; it adds no new policy. The FR1–FR2 Verification row adds the spec-reader cases. That needs an owner's/approve-specat this head.Change
.github/scripts/fs-guard-tests.mjs:specReaderTests(),uiTestRoots(), and the--spec-readers [--project ui|node]flags (the diff turns renames off in that mode), beside the existing guard lister and sharing its import walker..github/workflows/ci.yml: intest-uiandtest-node, four steps on leg 1 when the scope is spec-only: check out the change, list that project's readers, set up (only when there are any), and run them withpnpm test:ui|test:node. There is no--passWithNoTests: the list holds that project's tests only, so a run that finds none fails the step..github/scripts/ci-test-routing.test.mjs: the lane-shape checks skip the new reader step, whose contract lives with the lister's.docs/specs/AST-030/spec.md: FR2's knowledge owners, and the FR1–FR2 Verification row.Evidence
fs-guard-tests.test.mjs:roles.test.ts, and a change to AST-084 listsicon-set-checks.test.mjs.--no-renamesfails it.vitest.config.ts, and an unexpected config shape fails the lister. A real git repository runs--project uiand--project node, and each lists only its own reader; inverting the filter fails it.docs/specs/AST-${n}/spec.md) is selected for any spec record; without the cut-back it is not.docs/specs/**/spec.md,docs/specs/*/spec.md) are selected for any spec record. Stripping/**/inside strings, or keeping the wildcard segment, fails them.roles.test.tsamong 6 tests.ci-test-routing,fs-guard-testsand the CI contract suites pass (461 tests).test-uiandtest-nodestay on standard GitHub-hosted runners, and no job is added. Only spec-only pull requests change; every other pull request andmainare unchanged.test-nodegoes from about 1.2 to about 3.0 (leg 1: setup plus about 14 readers for a component record).test-uigoes from about 1.2 to about 1.35 (only the check-out and listing, unless a UI test reads the record).test-nodetakes about 2 minutes instead of about 15 s, so up to about +2 minutes.Scope
Writing rubric results