diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 99205390e..9662ed24e 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -2,6 +2,10 @@ name: E2E run-name: E2E ${{ inputs.ref || github.ref }} +# Why: checkout + artifact upload only; callers can only further restrict. +permissions: + contents: read + on: workflow_call: inputs: diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index aa65296d3..f5497280d 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -265,6 +265,55 @@ jobs: - name: Smoke packaged CLI run: node config/scripts/smoke-packaged-cli.mjs --app-dir=dist/linux-unpacked + # Why: regression specs under tests/e2e/** used to merge green without ever + # running — e2e.yml only fired on schedule/release (#10518). Path-filter so + # ordinary PRs stay light; any E2E suite change still gets a full shard run. + e2e-paths: + name: detect e2e path changes + runs-on: ubuntu-latest + if: github.event.pull_request.draft != true + # Why: detector only needs to read the checkout; do not inherit repo defaults. + permissions: + contents: read + outputs: + should_run: ${{ steps.filter.outputs.should_run }} + steps: + - name: Checkout + uses: actions/checkout@v6 + with: + fetch-depth: 0 + persist-credentials: false + + - name: Filter E2E-relevant paths + id: filter + run: | + set -euo pipefail + BASE="${{ github.event.pull_request.base.sha }}" + HEAD="${{ github.event.pull_request.head.sha }}" + # Why: capture first so a failed git diff does not look like "no matches" + # (pipeline status in `if` is not aborted by set -e). Merge-base limits the + # list to files this PR introduced, not base-branch drift. + CHANGED="$(git diff --name-only --merge-base "$BASE" "$HEAD")" + # Why: tests/playwright.config.ts sits beside tests/e2e/, not inside it, so + # it needs its own pattern — a bare `playwright.` prefix matches no tracked + # file and would silently skip E2E when the runner config changes. + if printf '%s\n' "$CHANGED" | grep -E '^(tests/e2e/|tests/playwright\.|\.github/workflows/e2e\.yml$)' >/dev/null; then + echo "should_run=true" >> "$GITHUB_OUTPUT" + echo "E2E path changes detected" + else + echo "should_run=false" >> "$GITHUB_OUTPUT" + echo "No E2E path changes" + fi + + e2e: + name: e2e + needs: e2e-paths + if: needs.e2e-paths.outputs.should_run == 'true' + # Why: reusable e2e.yml only checkouts, builds, and uploads artifacts. + permissions: + contents: read + uses: ./.github/workflows/e2e.yml + verify: if: always() needs: @@ -277,6 +326,14 @@ jobs: runs-on: ubuntu-latest steps: + # Why: e2e is deliberately absent from needs. The suite is currently red on + # main (every scheduled run), so gating merges on it would block any PR that + # touches tests/e2e/** — including the ones fixing the suite. Until it is + # green the job runs and reports for E2E-path PRs without blocking. To flip + # it on: add `e2e` to needs, add E2E to the env below, and require + # `"$E2E" = success || skipped` after the loop — skipped is the normal + # result for a path-filtered job and must keep passing, so it has to be + # checked outside the loop or it would excuse the jobs above. - name: Require successful checks env: STATIC_ANALYSIS: ${{ needs.static_analysis.result }} diff --git a/config/scripts/pr-e2e-gate-contract.test.mjs b/config/scripts/pr-e2e-gate-contract.test.mjs new file mode 100644 index 000000000..bcedb1b5a --- /dev/null +++ b/config/scripts/pr-e2e-gate-contract.test.mjs @@ -0,0 +1,64 @@ +import { readFileSync } from 'node:fs' +import { join, resolve } from 'node:path' +import { describe, expect, it } from 'vitest' +import { parse } from 'yaml' + +const projectDir = resolve(import.meta.dirname, '../..') +const prWorkflow = parse(readFileSync(join(projectDir, '.github/workflows/pr.yml'), 'utf8')) + +const filterStep = prWorkflow.jobs['e2e-paths'].steps.find( + (step) => step.name === 'Filter E2E-relevant paths' +) +const verifyStep = prWorkflow.jobs.verify.steps.find( + (step) => step.name === 'Require successful checks' +) + +describe('PR E2E gate contract', () => { + it('keeps E2E advisory while the suite is red on main', () => { + // Why: pin the deliberate choice so it reads as intentional rather than as + // the "forgot to wire the gate" bug this file originally caught. Gating on a + // suite that fails every scheduled run would block the PRs that fix it. + // Flipping to blocking means updating this expectation too — see the comment + // on verify's Require-successful-checks step for the exact wiring. + expect(prWorkflow.jobs.verify.needs).not.toContain('e2e') + expect(verifyStep.env.E2E).toBeUndefined() + expect(verifyStep.run).not.toContain('$E2E') + }) + + it('runs E2E only when the detector says the PR touches E2E paths', () => { + // Why: without this the job could lose its filter and run on every PR — the + // cost the path filter exists to avoid — while the gate assertions above + // stay green. + expect(prWorkflow.jobs.e2e.needs).toBe('e2e-paths') + expect(prWorkflow.jobs.e2e.if).toBe("needs.e2e-paths.outputs.should_run == 'true'") + expect(prWorkflow.jobs['e2e-paths'].outputs.should_run).toBe( + '${{ steps.filter.outputs.should_run }}' + ) + }) + + it('enforces every job verify depends on', () => { + // Why: derive from verify.needs rather than hardcoding, so adding a required + // job without adding it to the strict loop fails here instead of silently + // leaving that job unenforced. This is what caught GIT_COMPATIBILITY and + // SHELL_CONTRACTS being absent from an earlier hardcoded list. + const strictLoop = verifyStep.run.slice(0, verifyStep.run.indexOf('done')) + for (const job of prWorkflow.jobs.verify.needs) { + const envVar = job.toUpperCase() + expect(verifyStep.env[envVar]).toBe(`\${{ needs.${job}.result }}`) + expect(strictLoop).toContain(`"$${envVar}"`) + } + }) + + it('matches the Playwright config where it actually lives', () => { + // Why: the config is tests/playwright.config.ts, beside tests/e2e/ rather + // than inside it. A bare `playwright.` prefix matches no tracked file, so + // editing the runner config would silently skip E2E. + expect(filterStep.run).toContain('tests/playwright\\.') + expect(filterStep.run).not.toMatch(/\(\^?\|\|]tests\/e2e\/\|playwright\\\./) + }) + + it('scopes detection to the PR range so base drift cannot false-trigger', () => { + expect(filterStep.run).toContain('git diff --name-only --merge-base "$BASE" "$HEAD"') + expect(filterStep.run).toContain('set -euo pipefail') + }) +})