ci(e2e): run the E2E specs a PR changes (#10518)

.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<<SPECS_EOF'
+            cat changed-specs.txt
+            echo 'SPECS_EOF'
+          } >> "$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

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/')
+  })
+})

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` : '')
+}

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",