Repository navigation
ci(pr-a11y): skip contract specs an older PR head does not have - #8240
Conversation
A pull request's run takes its workflow from the test merge with main, but pr-a11y checks out the exact PR head. When a pull request that adds a contract step merges, every open pull request that branched before it fails that step with "No tests found" until it rebases. pr-a11y now loads .github/scripts/skip-absent-specs.sh from the base and exports it as BASH_ENV. Its pnpm function drops spec files absent from the checkout from `pnpm exec playwright test`, with a notice naming each, and passes a step left with none. Other pnpm commands and Playwright's exit status are untouched, and none of the contract steps change. A routing test keeps every spec a pr-a11y step names present on main, so only an older head skips anything.
|
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.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Visual Regression46 of 498 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 26 more. Generated by PR Enrichment workflow | Storybook | Sandbox | 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.
Recommended for approval: loading the wrapper from the base and filtering only the spec arguments of pnpm exec playwright test fixes the No tests found failure on older heads, the new tests keep main from naming a missing spec, and release checks still run every contract. Two optional follow-ups: the heading-links step writes should_run=true before its Playwright call, so an older head with Markdown in scope still fails at that step's if-no-files-found: error upload. The notice in skip-absent-specs.sh also says a missing spec "was added to main" even when the pull request deleted it, so wording it as "not in this pull request's head" would stay accurate.
[Automated review]
| -*) ;; | ||
| *.spec.ts | *.spec.tsx | *.spec.js | *.spec.mjs) | ||
| if [ ! -e "$arg" ]; then | ||
| echo "::notice title=Spec not in this PR head::${arg} was added to main after this pull request branched; rebase to run it." |
There was a problem hiding this comment.
This text is printed for every absent spec, including one this pull request deleted while its step still names it. The test join catches that case through "names only spec files that exist", but this notice would point the author at main. Reporting what was observed stays accurate either way, for example: ${arg} is not in this pull request's head; if it was added to main after this branch, rebase to run it.
…the head An older head with Markdown in scope set should_run=true, the wrapper then skipped the absent spec, and the upload's if-no-files-found: error failed the step. The notice now says the spec is not in this pull request's head, which also holds when the pull request deleted it.
There was a problem hiding this comment.
Recommended for approval: loading the wrapper from the base and filtering only the spec arguments of pnpm exec playwright test fixes the No tests found failure on older heads, the heading-links step now promises its upload only when its spec is in the head, and release checks still run every contract. One optional follow-up: the wrapper is read from pull_request.base.sha, which can lag main (on this pull request's own run it was five commits behind the workflow being executed), so a pull request opened before this lands may still fail unwrapped; reading it from github.sha or the base branch's current tip would reach those too. This branch's own pr-a11y failure at the Lab reorder-grip pinch-zoom contract is the failure being fixed and clears with a rebase onto main.
[Automated review]
| - name: Skip contract specs the PR head predates | ||
| if: github.event_name == 'pull_request' | ||
| env: | ||
| BASE_SHA: ${{ github.event.pull_request.base.sha }} |
There was a problem hiding this comment.
This reads the wrapper from pull_request.base.sha, but the steps it serves come from the test merge with the current main. On this pull request's own run the two differed: BASE_SHA was the main commit the pull request was opened on, five commits behind the ci.yml the job executed, and the API still reports that base after the later push. So after this lands, an older pull request whose recorded base predates it can still get the warning below and fail with No tests found until it rebases. Reading the wrapper from github.sha (the test merge) or from the base branch's current tip would reach those pull requests too; the BASE_SHA assertion in ci-test-routing.test.mjs would move with it.
…the PR's base pull_request.base.sha can lag main, so a pull request opened before this lands would read a base without the wrapper and fail unwrapped. github.sha is the merge commit the steps themselves come from, so it always has the wrapper those steps rely on.
There was a problem hiding this comment.
Recommended for approval: the wrapper is now read from the test merge commit, so it reaches open pull requests whose head and recorded base both predate it, and this pull request's own pr-a11y run skipped the reorder-grip spec its head lacks with a notice and passed. One optional follow-up: the description still says the step loads the wrapper from BASE_SHA and quotes the old notice wording, and its "Not covered" note could add that a contract main renames or moves after a branch point is skipped the same way as a new one.
[Automated review]
Maintainer impact
When a pull request that adds a pr-a11y contract step merges, pr-a11y breaks on every open pull request that branched before it. #8130 added "Run picker wheel pinch-zoom contract", and #8231's next run failed that step with
Error: No tests found.Each open pull request hits it on its next push until it rebases, and again on the next such merge.Intended invariant
A pr-a11y contract runs whenever its spec file exists in the checked-out PR head. Main's steps name only existing spec files. pr-a11y keeps checking out the exact PR head.
Change
Why: a pull request's run takes its workflow from the test merge with main, so it has main's newest steps, but pr-a11y checks out
pull_request.head.sha(pinned by the provenance test). A contract step runspnpm exec playwright test <spec> …, and a spec added on main after the branch point is not in that checkout.pr-scope-touches.shis loaded from the base for the same reason..github/scripts/skip-absent-specs.shdefines a bashpnpmfunction. Forpnpm exec playwright test, it drops spec files absent from the checkout and emits a::noticenaming each one ("…is not in this pull request's head; rebase onto main to run it"). A step left with no spec passes without running Playwright. Every other pnpm command runs unchanged, and Playwright's exit status still decides the step.github.sha, the test merge commit the steps themselves come from (an older head lacks it, andpull_request.base.shacan lag main), and exports it asBASH_ENV, so every later bash step gets the wrapper.Not covered: a stale head that skips a new contract can merge without running it, as before the contract existed. A contract that main renames or moves after the branch point is skipped the same way.
Evidence
Error: No tests found., exit 1.BASH_ENV, prints one notice and "nothing to run", and exits 0.Scope
Testing
pnpm exec vitest run --project node .github/scripts: 69 files, 1220 tests passed. New or changed:skip-absent-specs.test.mjs(new) runs step scripts withbash -eand the wrapper inBASH_ENV, against a stand-in pnpm. It covers five cases:ci-test-routing.test.mjs:github.sha(not the PR base) and setsBASH_ENV;release-check-workflow.test.mjs: the wrapper step runs on pull requests, not on release checks.bash -n,check-executable-bits.Maintainer note: two commits on top (c3a9cb4, cff15ae) fix the review's findings: the heading-links upload no longer expects evidence from a skipped spec, and the wrapper loads from the merge commit.