diff --git a/.github/workflows/e2e-changed-specs.yml b/.github/workflows/e2e-changed-specs.yml new file mode 100644 index 000000000000..363546ed483d --- /dev/null +++ b/.github/workflows/e2e-changed-specs.yml @@ -0,0 +1,120 @@ +name: E2E (changed specs) + +# Why: e2e.yml is reachable only from schedule and release-cut's workflow_call, +# so a PR that adds an E2E regression test never runs that test — it runs for +# the first time on main, where nobody is gating on it (#10518). This gate runs +# exactly the specs the PR touched: it closes the "test lands red" hole without +# putting the full 22-minute sharded suite on every pull request. +on: + pull_request: + types: + - opened + - synchronize + - reopened + - ready_for_review + +# Why: a force-push should not leave the previous Electron run burning a runner. +concurrency: + group: e2e-changed-specs-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + changed-specs: + name: e2e changed specs + runs-on: ubuntu-latest + timeout-minutes: 30 + permissions: + contents: read + pull-requests: read + + steps: + - name: Checkout + uses: actions/checkout@v6 + with: + persist-credentials: false + + # Why: the PR's file list comes from the API rather than a git diff — the + # default PR checkout is shallow, and its merge ref would need extra + # fetches to produce the same answer. + - name: Select changed E2E specs + id: select + env: + GH_TOKEN: ${{ github.token }} + run: | + set -euo pipefail + gh api --paginate \ + "repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/files" \ + -q '.[] | select(.status != "removed") | .filename' > changed-files.txt + node config/scripts/select-changed-e2e-specs.mjs < changed-files.txt > changed-specs.txt + { + echo 'specs<> "$GITHUB_OUTPUT" + if [ -s changed-specs.txt ]; then + { + echo '### E2E specs this PR changed' + echo + sed 's/^/- `/; s/$/`/' changed-specs.txt + } >> "$GITHUB_STEP_SUMMARY" + else + echo 'No E2E spec changed — nothing to run.' >> "$GITHUB_STEP_SUMMARY" + fi + + - name: Install native build and headless UI tools + if: steps.select.outputs.specs != '' + run: sudo apt-get update && sudo apt-get install -y build-essential python3 xvfb + + - name: Setup Node.js + if: steps.select.outputs.specs != '' + uses: actions/setup-node@v6 + with: + node-version-file: package.json + + - name: Setup pnpm + if: steps.select.outputs.specs != '' + uses: pnpm/action-setup@v6 + with: + run_install: false + + # Why: pnpm's bundled node-gyp ships gyp_main.py without execute + # permission and breaks node-pty. Use the lockfile copy instead of + # installing one outside the lock. + - name: Use lockfile node-gyp to avoid pnpm's bundled copy (Linux only) + if: steps.select.outputs.specs != '' && runner.os == 'Linux' + run: echo "npm_config_node_gyp=$(pwd)/node_modules/node-gyp/bin/node-gyp.js" >> "$GITHUB_ENV" + + - name: Install dependencies + if: steps.select.outputs.specs != '' + run: pnpm install --frozen-lockfile + + # Why: mirrors e2e.yml's build job so Playwright's globalSetup reuses the + # output (SKIP_BUILD=1) instead of building inside the test run. + - name: Build Electron app for E2E + if: steps.select.outputs.specs != '' + run: npx electron-vite build --mode e2e + + # Why: the selected paths are PR-controlled, so they are read into an array + # from the file the selector wrote and expanded as separate argv entries — + # never interpolated into the command string. + - name: Run changed E2E specs + id: run + if: steps.select.outputs.specs != '' + shell: bash + run: | + set -euo pipefail + SPECS=() + while IFS= read -r spec; do SPECS+=("$spec"); done < changed-specs.txt + printf 'Running %d changed spec(s):\n' "${#SPECS[@]}" + printf ' %s\n' "${SPECS[@]}" + xvfb-run --auto-servernum env SKIP_BUILD=1 ORCA_E2E_FORWARD_APP_LOGS=1 \ + pnpm run test:e2e --pass-with-no-tests "${SPECS[@]}" + + - name: Upload Playwright traces + if: failure() + uses: actions/upload-artifact@v7 + with: + name: playwright-traces-changed-specs + path: test-results/ + retention-days: 7 + if-no-files-found: ignore diff --git a/config/scripts/e2e-changed-specs-workflow.test.mjs b/config/scripts/e2e-changed-specs-workflow.test.mjs new file mode 100644 index 000000000000..f382e64c7082 --- /dev/null +++ b/config/scripts/e2e-changed-specs-workflow.test.mjs @@ -0,0 +1,172 @@ +import { execFileSync } from 'node:child_process' +import { readFileSync } from 'node:fs' +import { join, resolve } from 'node:path' +import { describe, expect, it } from 'vitest' +import { parse } from 'yaml' +import { selectChangedE2eSpecs } from './select-changed-e2e-specs.mjs' + +const projectDir = resolve(import.meta.dirname, '../..') +const workflowPath = join(projectDir, '.github/workflows/e2e-changed-specs.yml') +const selectorPath = join(projectDir, 'config/scripts/select-changed-e2e-specs.mjs') + +function readWorkflow() { + return parse(readFileSync(workflowPath, 'utf8')) +} + +function runSelector(stdin) { + return execFileSync('node', [selectorPath], { input: stdin, encoding: 'utf8' }) +} + +describe('select-changed-e2e-specs', () => { + it('keeps changed E2E specs and drops everything else', () => { + expect( + selectChangedE2eSpecs([ + 'src/main/ipc/preflight.ts', + 'tests/e2e/onboarding.spec.ts', + 'tests/e2e/helpers/orca-app.ts', + 'tests/unit/foo.spec.ts', + 'tests/e2e/computer-mac.e2e.ts', + 'docs/e2e.md' + ]) + ).toEqual(['tests/e2e/onboarding.spec.ts']) + }) + + it('accepts specs in nested E2E directories', () => { + expect(selectChangedE2eSpecs(['tests/e2e/regression/tab-close.spec.ts'])).toEqual([ + 'tests/e2e/regression/tab-close.spec.ts' + ]) + }) + + it('deduplicates and sorts so the run order does not depend on the API page order', () => { + expect( + selectChangedE2eSpecs([ + 'tests/e2e/zeta.spec.ts', + 'tests/e2e/alpha.spec.ts', + 'tests/e2e/zeta.spec.ts' + ]) + ).toEqual(['tests/e2e/alpha.spec.ts', 'tests/e2e/zeta.spec.ts']) + }) + + it('drops paths a fork PR could weaponize into the test command', () => { + // Why: these become argv for the shell-invoked `pnpm run test:e2e`, and a + // fork PR picks its own filenames. + expect( + selectChangedE2eSpecs([ + 'tests/e2e/a.spec.ts; rm -rf /', + 'tests/e2e/$(id).spec.ts', + 'tests/e2e/`id`.spec.ts', + 'tests/e2e/a b.spec.ts', + 'tests/e2e/--shard=1/2.spec.ts', + 'tests/e2e/../../etc/passwd.spec.ts', + '../tests/e2e/outside.spec.ts', + '/tests/e2e/absolute.spec.ts' + ]) + ).toEqual([]) + }) + + it('rejects leading and trailing whitespace in PR-controlled filenames', () => { + expect( + selectChangedE2eSpecs([ + ' tests/e2e/leading.spec.ts', + 'tests/e2e/trailing.spec.ts ', + 'tests/e2e/valid.spec.ts' + ]) + ).toEqual(['tests/e2e/valid.spec.ts']) + }) + + it('emits nothing at all when no spec changed, so the workflow can test for empty', () => { + expect(runSelector('src/main/index.ts\nREADME.md\n')).toBe('') + }) + + it('emits one newline-terminated path per selected spec', () => { + expect(runSelector('tests/e2e/b.spec.ts\nsrc/main/index.ts\ntests/e2e/a.spec.ts\n')).toBe( + 'tests/e2e/a.spec.ts\ntests/e2e/b.spec.ts\n' + ) + }) +}) + +describe('e2e-changed-specs workflow', () => { + it('runs on every pull request so it can be made a required check', () => { + const workflow = readWorkflow() + + // Why: a `paths:` filter would make the check simply not appear on PRs that + // touch no spec, and a required check that never reports blocks the merge + // box forever. Selection happens inside the job instead. + expect(workflow.on.pull_request.paths).toBeUndefined() + expect(workflow.on.pull_request['paths-ignore']).toBeUndefined() + expect(workflow.on.pull_request.types).toEqual([ + 'opened', + 'synchronize', + 'reopened', + 'ready_for_review' + ]) + }) + + it('asks the API for the changed files instead of diffing a shallow checkout', () => { + const workflow = readWorkflow() + const steps = workflow.jobs['changed-specs'].steps + const select = steps.find((step) => step.id === 'select') + + expect(select.run).toContain('--paginate') + expect(select.run).toContain('/pulls/${{ github.event.pull_request.number }}/files') + // Why: a spec deleted by the PR must not be handed to Playwright. + expect(select.run).toContain('select(.status != "removed")') + expect(select.run).toContain('config/scripts/select-changed-e2e-specs.mjs') + expect(workflow.jobs['changed-specs'].permissions).toEqual({ + contents: 'read', + 'pull-requests': 'read' + }) + }) + + it('skips the build when no spec changed', () => { + const workflow = readWorkflow() + const steps = workflow.jobs['changed-specs'].steps + const afterSelect = steps.slice(steps.findIndex((step) => step.id === 'select') + 1) + + // Why: the cheap path is the common one — no toolchain, no pnpm install, no + // Electron build for a PR that changed no spec. Only the failure-gated + // trace upload is exempt, and it has nothing to collect on that path. + const workSteps = afterSelect.filter( + (step) => !step.uses?.startsWith('actions/upload-artifact') + ) + expect(workSteps.length).toBeGreaterThan(0) + for (const step of workSteps) { + expect(step.if).toContain("steps.select.outputs.specs != ''") + } + }) + + it('runs the selected specs the same way the scheduled suite runs its shards', () => { + const workflow = readWorkflow() + const run = workflow.jobs['changed-specs'].steps.find((step) => step.id === 'run') + const scheduled = parse(readFileSync(join(projectDir, '.github/workflows/e2e.yml'), 'utf8')) + const scheduledRun = scheduled.jobs.e2e.steps.find((step) => + step.run?.includes('pnpm run test:e2e') + ).run + + for (const token of [ + 'xvfb-run --auto-servernum', + 'SKIP_BUILD=1', + 'ORCA_E2E_FORWARD_APP_LOGS=1', + 'pnpm run test:e2e' + ]) { + expect(scheduledRun).toContain(token) + expect(run.run).toContain(token) + } + // Why: a spec whose tests are all @headful is filtered out by the + // electron-headless project, and "no tests ran" must not fail the gate. + expect(run.run).toContain('--pass-with-no-tests') + // Why: paths reach the command as an array, never spliced into the string. + expect(run.run).toContain('"${SPECS[@]}"') + expect(run.shell).toBe('bash') + }) + + it('uploads traces when the gate fails', () => { + const workflow = readWorkflow() + const upload = workflow.jobs['changed-specs'].steps.find((step) => + step.uses?.startsWith('actions/upload-artifact') + ) + + expect(upload.if).toContain('failure()') + expect(upload.with.path).toBe('test-results/') + }) +}) diff --git a/config/scripts/select-changed-e2e-specs.mjs b/config/scripts/select-changed-e2e-specs.mjs new file mode 100644 index 000000000000..fea783cef9cd --- /dev/null +++ b/config/scripts/select-changed-e2e-specs.mjs @@ -0,0 +1,43 @@ +#!/usr/bin/env node +/** + * Select the E2E spec files a pull request changed. + * + * Reads the PR's changed-file list (one path per line) on stdin and writes the + * subset that the headless Playwright suite can run, one path per line, on + * stdout. The e2e-changed-specs workflow feeds the result straight to + * `pnpm run test:e2e`, so this is the boundary where PR-controlled filenames + * stop being arbitrary text. + * + * Why a strict allowlist rather than a suffix check: a fork PR chooses its own + * filenames, and the selected paths become argv for a shell-invoked command. + * Anything outside `tests/e2e/`, containing a shell metacharacter, or walking + * upward is dropped rather than escaped. + */ + +/** Only plain paths directly under tests/e2e/ (nested dirs allowed). */ +const SPEC_PATH = /^tests\/e2e\/[A-Za-z0-9._-]+(?:\/[A-Za-z0-9._-]+)*\.spec\.ts$/ + +export function selectChangedE2eSpecs(changedFiles) { + const selected = changedFiles + // Why: do not normalize PR-controlled filenames before the allowlist test. + .filter((path) => SPEC_PATH.test(path)) + // Why: `.` and `..` are legal filename characters for the pattern above, + // so reject traversal explicitly instead of trusting the character class. + .filter((path) => !path.split('/').includes('..')) + return [...new Set(selected)].sort() +} + +async function readStdin() { + const chunks = [] + for await (const chunk of process.stdin) { + chunks.push(chunk) + } + return Buffer.concat(chunks).toString('utf8') +} + +// Why: importing this module from the unit test must not consume stdin. +if (process.argv[1] && import.meta.url === `file://${process.argv[1]}`) { + const input = await readStdin() + const specs = selectChangedE2eSpecs(input.split('\n')) + process.stdout.write(specs.length ? `${specs.join('\n')}\n` : '') +} diff --git a/package.json b/package.json index af69111a1fc1..3c4127867b5b 100644 --- a/package.json +++ b/package.json @@ -193,6 +193,7 @@ "marked": "^17.0.1", "mermaid": "^11.15.0", "monaco-editor": "^0.55.1", + "node-gyp": "12.3.0", "oxfmt": "^0.52.0", "oxlint": "^1.71.0", "oxlint-plugin-react-doctor": "0.2.10", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ea1672e8937e..4894ef3f1c9b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -282,6 +282,9 @@ importers: monaco-editor: specifier: ^0.55.1 version: 0.55.1 + node-gyp: + specifier: 12.3.0 + version: 12.3.0 oxfmt: specifier: ^0.52.0 version: 0.52.0