Repository navigation
Conversation
The canonical setup compositions and the guest fixture contrast need a production browser render, and test-build already installs Playwright on the same runner under the same scope condition. Running them there drops a job from every pull request without changing what runs or when.
|
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.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This looks ready to merge once a fresh CI run passes: moving the two vibe-tests browser checks into test-build keeps the same checks under the same scope condition and permissions, every required check still reports, and the folded job should take roughly 6–7 of its 15 minutes. Consider noting one consequence in the description: a fixture failure now fails the required test check (before, it only turned the non-required fixture-contrast job red), and a pnpm test:build failure now skips both fixture steps for that run. The only CI run on this head was cancelled before the moved steps finished, so it needs a new run.
[Automated review]
|
|
||
| # The vibe-tests fixtures share this Playwright-equipped runner: the | ||
| # canonical setup compositions and the guest fixture contrast need a | ||
| # production browser render (registry scope test-theme-e2e). |
There was a problem hiding this comment.
Nothing in the repository defines a registry scope named test-theme-e2e; this comment is its only occurrence, so the parenthetical points readers at nothing. Consider dropping it, or naming the file that defines the scope once one does.
| # The vibe-tests fixtures share this Playwright-equipped runner: the | ||
| # canonical setup compositions and the guest fixture contrast need a | ||
| # production browser render (registry scope test-theme-e2e). | ||
| - name: Verify canonical setup compositions |
There was a problem hiding this comment.
Optional: these two steps use the default success condition, so when pnpm test:build fails they are skipped and the run reports nothing about the fixtures, where the separate job still did. If you want that signal back, if: ${{ !cancelled() }} on both steps restores it; test-build, and with it test, still fails either way.
There was a problem hiding this comment.
Thanks — folding fixture-contrast's two checks into test-build is a clean consolidation: both verification steps keep the same runner and scope condition, and the required test check still projects the result.
One change before this is ready: the new comment's (registry scope test-theme-e2e) names a scope that exists nowhere in the repository, so it points readers at nothing — please drop it or name the real scope.
Optionally, because the two steps use the default success condition, a pnpm test:build failure now skips them for that run; adding if: ${{ !cancelled() }} to both restores the independent fixture signal if you want it (the test check still fails either way).
[Automated review]
|
|
||
| # The vibe-tests fixtures share this Playwright-equipped runner: the | ||
| # canonical setup compositions and the guest fixture contrast need a | ||
| # production browser render (registry scope test-theme-e2e). |
There was a problem hiding this comment.
(registry scope test-theme-e2e) names a scope that exists nowhere in the repository — test-theme-e2e appears only on this line, so it points readers at nothing. Drop the parenthetical, or name the real scope/surface these checks belong to.
There was a problem hiding this comment.
This looks ready to merge: both vibe-tests browser checks now run at the end of test-build under the same runner, scope condition and permissions, every required check still reports, and the comment above them no longer names a scope that does not exist. Consider noting in the description that a fixture failure now fails the required test check and that a pnpm test:build failure skips both fixture steps for that run; if: ${{ !cancelled() }} on both steps would keep their signal. No CI run has completed the folded job on this pull request yet.
[Automated review]
There was a problem hiding this comment.
This looks ready to merge: after the merge with main, both vibe-tests browser checks run at the end of test-build behind the same scope condition, permissions and per-author heavy-suite caps, and the heavy-job contract now expects only pr-interaction and test-build. Consider updating the description: its +13/−20 count predates the merge, and it leaves out that a fixture failure now fails the required test check. If you want the fixture result back when pnpm test:build fails, condition both steps on the Playwright install's outcome (if: ${{ !cancelled() && steps.<id>.outcome == 'success' }}) rather than !cancelled() alone, which would now also start them after a heavy-suite hold. No CI run has completed on this revision yet.
[Automated review]
| # The vibe-tests fixtures share this Playwright-equipped runner: the | ||
| # canonical setup compositions and the guest fixture contrast need a | ||
| # production browser render. | ||
| - name: Verify canonical setup compositions |
There was a problem hiding this comment.
Optional: to keep the fixture result when pnpm test:build fails, give Install Playwright an id and put if: ${{ !cancelled() && steps.<id>.outcome == 'success' }} on both steps. Now that test-build starts with the heavy-suite caps, !cancelled() alone would also start these steps after a hold or a failed setup, where they fail for unrelated reasons beside the real error.
Who this helps
Everyone opening a pull request: one fewer job per CI run, with no change to what is tested or when.
What changes
fixture-contrastran two browser checks, the canonical setup compositions and the guest fixture contrast, on its own runner.test-buildalready installs Playwright on the same runner type, under the same scope condition.test-build, andfixture-contrastis removed. Required contexts are unchanged:teststill projectstest-build's result.Part 1 of 4 of the CI scope consolidation. Each part lands on its own, in order:
Line count
+13 / −20, net −7.
Verification
vitest run --project node .github/scripts: all pass.ci-tooling-routing.test.mjschecks that the two steps live intest-buildand inherit its Node-tooling skip.