Repository navigation
Conversation
… the Vitest lanes A change to internal/interaction-harness/, internal/eslint-plugin-astryx/, scripts/, .github/scripts/ or apps/sandbox/scripts/ is shared infrastructure, so it ran every Vitest test beside every other owner. Only Vitest's module graph and the source-reading guards can reach these files, so the classifier now reports vitest_full_suite=false for them: the UI and Node lanes run the tests the change reaches plus the guards, and every other owner (interaction proofs, browser contracts, builds, audits) still runs as for shared infrastructure. The scripts the Vitest configuration loads and admitted Node tooling stay on every Vitest test. AST-030 FR13 says so.
|
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.
|
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report🔎 Preview ·
|
| Before | After | Change | |
|---|---|---|---|
| Job-minutes per PR update | 81.4 | 90.5 | +9.1 |
| Runner pool, runner-minutes a day | 31,595 | 35,125 | +3,530 (+11%) |
| Average concurrent jobs | 21.9 | 24.4 | +2.5 |
| Time to green (median → this run) | 26.8 min | 19.5 min | −7.3 min |
| Runs a day | 388 | 388 | 0 |
Changed jobs: test-node changed: standard · test-ui changed: standard
Not measured (the jobs that use these keep their recent minutes above): .github/actions/test-scope/action.yml, .github/scripts/change-scope.cjs, .github/scripts/change-scope.test.mjs, .github/scripts/ci-test-routing.test.mjs, .github/scripts/spec-owner-workflow.test.mjs, .github/scripts/test-scope-action.test.mjs.
Flags: runner-pool usage +11%.
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
|
/approve-spec eeaec00 |
github-actions
Bot
removed
the
needs:spec-owner-review
![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 FR13 direction is approved, but the new output breaks an existing test, and the narrowing covers some files whose effect reaches tests outside Vitest's module graph, so their pull requests could go green while skipping tests they break.
spec-owner-workflow.test.mjs(line 95) expects the classifier's exact output, which now ends withvitest_full_suite=false, sotest-node (4/4)fails on this head; add that line to the expected string.scripts/generate-i18n-runtime.mjsruns duringpnpm installand writes the generated i18n modules that components import, yet a change to it alone selects no UI test, so about 310 component tests that render its output (for exampleAvatar.test.tsxandBanner.test.tsx) would not run.- Vitest's
--changeddoes not follow CommonJSrequireandfs-guard-tests.mjscounts onlycreateRequire, so a change to.github/scripts/run-labels.cjsruns neitherlib/pr-status.test.mjsnorlib/ci-impact.test.mjs, which load it throughrequire. .github/scripts/fs-guard-tests.mjsis narrowed too, so a pull request that changes it uses its own copy to decide which source-reading tests run.- The description and commit say only Vitest's module graph and the source-reading tests can reach these files, which the cases above disprove, so please narrow that claim to what the fix makes true.
- The
CI impactline is missing each changed job's runner class, average concurrent jobs and a time-to-green figure, which AST-030 FR19 asks for.
Keeping these files on every Vitest test, as VITEST_CONFIG_SCRIPTS already does for the config's imports, with a case for each, would close the gap; the fail-closed default and the workflow wiring otherwise look right.
[Automated review]
| /^\.github\/scripts\//, | ||
| /^apps\/sandbox\/scripts\//, | ||
| ]; | ||
| const VITEST_CONFIG_SCRIPTS = new Set([ |
There was a problem hiding this comment.
This exclusion covers only what vitest.config.ts imports. Scripts that prepare runs (scripts/generate-i18n-runtime.mjs writes the generated i18n modules most components import), CommonJS files reached through require (Vitest's --changed does not follow it), and .github/scripts/fs-guard-tests.mjs (the scoped steps run the pull request's copy) also reach tests outside the module graph, so they need to stay on every Vitest test.
| fs.appendFileSync( | ||
| outputPath, | ||
| `spec_only=${result.specOnly}\ndocsite_only=${result.docsiteOnly}\ntooling_only=${result.toolingOnly}\nlab_only=${result.labOnly}\nfull_suite=${result.fullSuite}\n`, | ||
| `spec_only=${result.specOnly}\ndocsite_only=${result.docsiteOnly}\ntooling_only=${result.toolingOnly}\nlab_only=${result.labOnly}\nfull_suite=${result.fullSuite}\nvitest_full_suite=${result.vitestFullSuite}\n`, |
There was a problem hiding this comment.
spec-owner-workflow.test.mjs:95-97 pins this exact output and now fails (test-node (4/4)); add vitest_full_suite=false\n to its expected string.
…t lanes The Node scripts stay shared for Vitest: package scripts run generators whose output component tests read (the i18n runtime), Vitest's --changed does not follow require, and the scoping tools themselves live there. The interaction harness and the lint plugin are ES modules whose Vitest reach is their importers plus the source-reading guards (every test that loads ESLint is one). The pinned classifier output in spec-owner-workflow.test.mjs now carries vitest_full_suite. AST-030 FR13 names the two groups.
cixzhang
changed the title
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec f8505e5 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Requesting changes for one fix in the description: the FR13 amendment carries an engineering owner's approval for this head, and the classifier, the action's fail-closed default and the two Vitest steps conform to it.
- Miss: The
CI impactline counts a scopedtest-uirun as about 2 runner-minutes, but each of its four legs still checks out, sets up and runs its share of the UI guard files, and the cheapest of 48 recent scopedtest-uiruns cost 11.9 runner-minutes (#9846, a single Changeset that reached no test) against a median 24.7 for full runs of the pull requests this change would narrow, so please restate the per-run line as about 24 to at least 12 runner-minutes and the pool line as at most about 450 runner-minutes a day and 0.3 concurrent jobs, with the Maintainer impact sentence to match.
Non-blocking: AST-030 DEC-6 still says shared infrastructure runs everything; one clause naming the new FR13 exception would keep the record consistent.
The test-node and time-to-green lines now hold.
[Automated review]
…ss and the lint plugin
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 12c291d |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the branch conflicts with main and pull-request CI starts only once that is resolved.
The FR13 and DEC-6 amendment carries an engineering owner's approval for this head, and the classifier, the action's fail-closed vitest_full_suite default and the two Vitest steps conform to it; the new classifier cases fail on main's classifier and pass here.
- The conflict is in
docs/specs/AST-030/spec.md, wheremainre-padded the Verification table whose FR13 row this pull request edits; after the rebase, the new head needs CI and an engineering owner's/approve-specagain.
[Automated review]
Maintainer impact
A pull request that changes the interaction harness or the repository's lint plugin runs every Vitest test, because the classifier calls those files shared infrastructure. In the UI lane that's a median of about 24 runner-minutes per full run on the pull requests this change narrows. A scoped run still costs at least about 12, because its four legs still check out, set up and run their share of the source-reading guards.
Intended invariant
A change whose only shared paths are the interaction harness or the lint plugin narrows only the Vitest owner: the UI and Node lanes run the tests whose module graph reaches the change, plus every test that reads repository sources. Every other owner runs as for shared infrastructure:
A change with any other shared path still runs every Vitest test.
Node scripts stay on every Vitest test:
--changeddoes not followrequire;Change
.github/scripts/change-scope.cjs:isVitestGraphInfraPathcoversinternal/interaction-harness/andinternal/eslint-plugin-astryx/.vitestFullSuiteis false when every shared path in the change is one of them;fullSuiteis unchanged. The CLI writesvitest_full_suite.vitest_full_suitethrough. Its "everything" default istrue, and a base classifier that predates it reports nothing, which runs every test.ci.yml: the two Vitest steps ("Run this shard of the UI tests", "… Node tests") scope byvitest_full_suite. Every otherSCOPED/full_suitereader is unchanged.Evidence
change-scope.test.mjs):fullSuiteand narrowsvitestFullSuite;scripts/generate-i18n-runtime.mjs,.github/scripts/run-labels.cjs,.github/scripts/fs-guard-tests.mjsand admitted Node tooling;vitest_full_suite.ci-test-routing.test.mjs): only the two Vitest steps readvitest_full_suite.spec-owner-workflow.test.mjs's pinned classifier output includes it.main's classifier, the new classifier cases fail.change-scope,ci-test-routing,test-scope-action,spec-owner-workflowandvisual-scopetests pass (256), andpnpm check:knowledgepasses.test-ui(standardubuntu-latestrunners) drops from a median of about 24 to at least about 12 runner-minutes (the cheapest of 48 recent scopedtest-uiruns cost 11.9).test-nodechanges little: its scoped run still runs every source-reading guard, which holds nearly all of the Node project's test time.main's hourly full unit run (FR21).Writing rubric results
AST-030 FR13 (one sentence added), its verification row, and DEC-6 (one clause added):
Scope