Repository navigation
Conversation
Adds the test-scope action: inside the job that owns the work, it reads the pull request's file list from GitHub and the base commit's trusted classifier, and puts everything in scope when it cannot decide (spec:AST-030 FR3, FR4). The interaction proofs and the native Swift build use it, so the interaction-scope and native-swift-scope jobs, the pr-scope-touches helper and its tests go away. Native Swift keeps one named runner-selection pre-job, native-swift-runner, because its macOS runner must be chosen before the job starts. AST-030's path list names the action instead of the helper.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
/approve-spec bc9ce0e |
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: moving the interaction and native-Swift scope decisions into the shared test-scope action conforms to the CI routing spec (AST-030), but two fail-closed edges no longer have a test, which AST-030 FR9 requires.
- Add a
test-scope-action.test.mjscase where therequires-head-filelookup fails with a non-404 error (a 500, say) and assert thatappliesstays'true'with no notice. - Restore a test that runs the "Require this pull request's Storybook build" step and expects exit 1, as the deleted "fails each guard step when it runs" case did.
Please also note in the description that every merge-queue entry now runs the macOS Swift build and that pr-interaction now always takes a 4-core runner.
[Automated review]
| try { | ||
| await github.rest.repos.getContent({...context.repo, path: headFile, ref: pr.head.sha}); | ||
| } catch (error) { | ||
| if (error.status === 404) { |
There was a problem hiding this comment.
Only a 404 should turn the proofs off here, and today that holds, but no test pins it. The decide() stub in test-scope-action.test.mjs only ever throws 404, so treating every error as "file absent" keeps all 15 cases green. AST-030 FR9 asks for a case where this lookup fails with, say, a 500 and applies stays 'true' with no notice.
| expect(step(proofs, 'Require a decided interaction scope').if).toBe( | ||
| "needs.interaction-scope.result != 'success'", | ||
| ); | ||
| it('fails closed when Storybook did not build for proofs that apply', () => { |
There was a problem hiding this comment.
This now checks only the guard's if. The deleted "fails each guard step when it runs" case ran the step and expected exit 1; without it, changing the guard to exit 0 keeps the suite green (AST-030 FR9). Running the step's run script and asserting status 1 restores that coverage.
A head-file lookup that fails for a reason other than 404 keeps the proofs in scope with no notice, and the Storybook guard fails the job when it runs (spec:AST-030 FR9).
|
/approve-spec 56ea52e |
There was a problem hiding this comment.
Requesting changes: the shared test-scope action conforms to the CI routing spec (AST-030), and the non-404 head lookup and the Storybook guard are now pinned, but two other fail-closed edges in the action still have no test, which AST-030 FR9 requires.
- Add a
test-scope-action.test.mjscase that renamespackages/core/src/Button/Button.tsxto a spec path and assertsspec_onlystays'false'andapplies'true'(theR100line inaction.yml). - Add a case where the pull request renames
playwright.interaction.config.tsaway and assertappliesstays'true'with no notice (theprevious_filenameclause of the head-file check).
It would also help to say in the description that every merge-queue entry now runs the macOS Swift build and that pr-interaction always takes a 4-core runner.
[Automated review]
| const STATUS = {added: 'A', removed: 'D', modified: 'M', changed: 'M', copied: 'C', unchanged: 'M'}; | ||
| const nameStatus = files | ||
| .map(f => f.status === 'renamed' | ||
| ? `R100\t${f.previous_filename}\t${f.filename}` |
There was a problem hiding this comment.
This R100 line is what keeps a rename's old path in front of the classifier, but no test depends on it: sending renames as M\t<new> keeps all 16 cases green, because "matches the old name of a rename" only checks touches. AST-030 FR9 asks for a case where, say, packages/core/src/Button/Button.tsx is renamed to a spec path and spec_only stays 'false' with applies 'true'.
| const specialized = ['spec_only', 'docsite_only', 'tooling_only'].some(key => classified[key] === 'true'); | ||
| let applies = String(!specialized && touches === 'true'); | ||
| const headFile = process.env.SCOPE_HEAD_FILE; | ||
| if (applies === 'true' && headFile && !files.some(f => f.filename === headFile || f.previous_filename === headFile)) { |
There was a problem hiding this comment.
The f.previous_filename === headFile clause keeps the proofs running (and failing) when a pull request renames playwright.interaction.config.ts away, but nothing pins it: without it the suite stays green and such a pull request would skip with the "merge main" notice instead. AST-030 FR9 asks for a rename-away case that asserts applies stays 'true' with no notice.
A rename into spec records is classified by both names, so it is not spec-only; a pull request that renames the required head file away still applies, with no notice (spec:AST-030 FR9).
|
/approve-spec c4a1ead |
There was a problem hiding this comment.
Requesting changes: the shared test-scope action conforms to the CI routing spec (AST-030), and the rename-away case now pins its clause, but two fail-closed edges still have no failing test, which AST-030 FR9 requires.
- In
test-scope-action.test.mjs, rename intodocs/specs/AST-030/spec.mdinstead ofdocs/specs/AST-030/Button.md:Button.mdis not a spec record, so the case passes even when renames reach the classifier without their old name. - In
ci-native-swift.test.mjs, assert thatnative-swift-runner's effectiveoutside-pull-request(its input, else the action's default) isrun; today switching off the merge-queue Swift build keeps every test green.
[Automated review]
| const {outputs} = await decide({ | ||
| files: [ | ||
| { | ||
| filename: 'docs/specs/AST-030/Button.md', |
There was a problem hiding this comment.
docs/specs/AST-030/Button.md is not a spec record to the base classifier (it matches only docs/specs/<id>/spec.md or plan.md), so this case passes even when action.yml:152 sends renames as M\t<new>. Renaming into docs/specs/AST-030/spec.md makes it fail under that change, which is what AST-030 FR9 asks for.
| fi | ||
| uses: ./.test-scope/.github/actions/test-scope | ||
| with: | ||
| paths: '^packages/native/.*(\.swift|/Package\.swift)$|^\.github/scripts/native-swift\.sh$|^\.github/actions/test-scope/' |
There was a problem hiding this comment.
This job relies on the action's default outside-pull-request: run to build Swift on every merge-group entry (AST-030 FR4, FR13), but nothing pins it: flipping the default at action.yml:39 to skip, or adding outside-pull-request: skip here, keeps the whole .github/scripts suite green. An assertion in ci-native-swift.test.mjs that scope.with['outside-pull-request'] ?? action.inputs['outside-pull-request'].default is 'run' closes it.
…ift run The rename-into-specs case now targets docs/specs/AST-030/spec.md, so it fails if a rename reaches the classifier without its old name, and the native Swift runner job is pinned to run its build outside a pull request (spec:AST-030 FR9).
|
/approve-spec 45ae187 |
There was a problem hiding this comment.
Recommending approval: the shared test-scope action conforms to the CI routing spec (AST-030), and with the rename case now using a real spec record and ci-native-swift.test.mjs pinning the merge-queue Swift run, every fail-closed edge AST-030 FR9 covers fails a test when removed. No further changes are needed for this review. When the stack is rebased onto main, pr-interaction's fixed 4-core-ubuntu runner will need reconciling with main's move to ubuntu-latest (#8885).
[Automated review]
Who this helps
Contributors whose PRs change components or native Swift: the interaction proofs and the Swift build no longer wait about a minute for a separate scope job before they start.
What changes
New
test-scopeaction (.github/actions/test-scope). It runs inside the job that owns the work.pr-interactionandtest-native-swiftuse it.interaction-scopeandnative-swift-scopejobs are removed, along with thepr-scope-toucheshelper and its tests.native-swift-runner, because its macOS runner has to be chosen before the job starts.AST-030's path list names the action instead of the removed helper. That is the one-line record change already approved for the consolidation.
Two runner effects:
pr-interactionnow always takes its 4-core runner. It skips its work inside the job when out of scope, rather than switching to a slim runner (no runner-selection exception for it).Required contexts are unchanged (FR7).
Part 2 of 4 of the CI scope consolidation. Stacked on #8916.
Line count
+629 / −936, net −307.
Verification
vitest run --project node .github/scripts: all pass.test-scope-action.test.mjsruns the action's real script against a stubbed GitHub API that serves the repository's real classifier files.ci-interactionandci-native-swiftrouting tests cover the new jobs.