ci: select desktop e2e specs through source reachability - #4583
Conversation
|
|
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The selection is sound as a filter, but a spec's static import closure is not the reachability relation an Electron e2e spec has with the app it drives, so this turns the Desktop e2e gate off for most desktop changes rather than narrowing it.
P1: Every spec imports ./fixtures and then launches a built Electron app. The app is loaded at runtime, so it is invisible to a static import closure. I computed the 36 spec closures at this head and compared them against the files that isE2eProductPath sets e2e=true for: apps/desktop/src/main 1 of 190 covered, src/renderer 29 of 493, src/preload 1 of 16, src/shared 1 of 8. (packages/ui/src is 125 of 128, but only because accessibility-coverage.spec.ts happens to import src/renderer/settings/settings-nav and drags the component tree in, which is accident rather than design.) Replaying the last 60 first-parent commits on main with this PR's own selector: of the 39 that trigger e2e, 11 now select zero specs and the step exits 0. One of them is e1e108a79 "move Settings to Usage into a renderer feature slice", which rewrote src/renderer/settings/settings-surface.tsx and settings/usage-settings-page.tsx, the exact surface settings.spec.ts, settings-row-focus-ring.spec.ts and accessibility-coverage.spec.ts drive in a real window. Others are 6cfb832e5 (shared Session Guest controls), 61cbda2fc (manual context compaction in Side Conversations), ea2457ae6 (connection snapshot during refresh). A concrete instance of the same gap: apps/desktop/package.json is not in FULL_SUITE_FILES, so an Electron version or main entry change sets e2e=true and selects zero specs. The 57 zero-spec changes in the PR's own histogram are this, not savings. The nightly full run turns each of these into a post-merge detection up to 24 hours later, on main.
I do not think there is a small patch here. Putting the desktop build entry points into every spec's closure makes the selection correct and collapses the reduction back to roughly zero, which is the honest reading: per-spec static reachability cannot select e2e specs. If selective e2e is worth it, coverage has to be declared by each spec (the feature slices it drives) or sampled from an instrumented full run.
P3: The gate now carries two selection authorities that systematically disagree. isE2eProductPath says any apps/desktop change needs e2e; the selector says it does not, and the selector silently wins. Whichever survives should be the only one.
P3: Generalizing collectWorkspaceSourceClosure dropped the Windows-specific completeness argument (why static imports alone were sound there: the forked root-authority.test.ts fixture and the bundled worker-entry.ts are both free of win32). workspace-source-closure.mjs now only says callers need their own escape coverage, and the Windows call site no longer states its. Worth restating above windowsPackageSourceEntrypoints.
P3: reporter: 'list' in playwright.config.ts also changes local runs and is separable from selection. The nightly schedule is not separable, it is the escape hatch and belongs with the mechanism.
Verified as fine: playwright.config.ts and the two selector scripts force a full run; storybook catalog paths stay out of e2e; root package.json / package-lock.json / ci.yml / ci-test-plan.mjs reach plan.full; a selector failure exits non-zero under set -euo pipefail rather than failing open; the new test step sits after npm ci, so the prepare codegen the closure needs has already run; removing the packages/ filter changes nothing for the Windows lane today (its closure has no non-packages/ input and its 3 tests pass).
CI has not run at this head: only label reported, the CI and Release Windows suites are still awaiting approval.
| if (plan.full || files.some((path) => SELECTION_AUTHORITY_FILES.has(path))) return specs; | ||
|
|
||
| const closures = options.closures ?? (await collectDesktopE2eSpecClosures(specs, repoRoot)); | ||
| return specs.filter((spec) => files.some((path) => closures.get(spec)?.has(path))); |
There was a problem hiding this comment.
P1: This is the whole selection, and the relation it uses is wrong for e2e. A spec imports ./fixtures and then launches a built Electron app, so the app under test never appears in the spec's static closure. Measured at this head against the files that make the planner set e2e=true: apps/desktop/src/main 1 of 190 files reach any spec, src/renderer 29 of 493, src/preload 1 of 16, src/shared 1 of 8. Replaying the last 60 first-parent commits, 11 of the 39 e2e-triggering merges select zero specs, including e1e108a79, which rewrote src/renderer/settings/settings-surface.tsx while settings.spec.ts and settings-row-focus-ring.spec.ts drive that exact surface. The gate goes green with nothing run and the nightly catches it after merge. The smallest correct fix (adding the desktop main/preload/renderer entry points to every spec's closure) also removes essentially all of the reduction, which is the signal that static import reachability cannot select e2e specs. Selection needs coverage declared per spec or sampled from an instrumented run.
| assert.ok(currentSpecs.length > 0); | ||
| for (const spec of currentSpecs) { | ||
| assert.ok(currentClosures.get(spec)?.has(spec), spec); | ||
| assert.ok(currentClosures.get(spec)?.has('apps/desktop/e2e/fixtures.ts'), spec); |
There was a problem hiding this comment.
P2: The suite does not test the scenario the PR is for. Three of the four tests use the hand-written closures fixture and only prove the filter; this one uses real closures but asserts only scripts/ reachability and that every spec closure contains fixtures.ts. Nothing asserts anything about apps/desktop/src/**, which is where the selection actually fails. Add an assertion that every product source file under apps/desktop/src selects at least one spec. It fails today, and that failure is the evidence the design needs.
| const files = [...new Set(changedFiles.map(normalizePath).filter(Boolean))]; | ||
| const specs = options.specs ?? listDesktopE2eSpecs(repoRoot); | ||
| const plan = planTests(files, { repoRoot, forceFull: options.forceFull }); | ||
| if (!plan.e2e) return []; |
There was a problem hiding this comment.
P3: Redundant with the caller. The Desktop e2e step is already gated on steps.plan.outputs.e2e and already branches on steps.plan.outputs.full, so plan.e2e, plan.full and the second changedFilesBetween here recompute a decision the same job made one step earlier. Let the selector take changed files and nothing else.
| * the source consumes. | ||
| * | ||
| * This closure covers static imports. Spawned processes and generated or | ||
| * separately bundled entry points need caller-owned escape coverage. |
There was a problem hiding this comment.
P3: The original comment on this function argued why static imports alone were complete for the Windows filter: root-authority.test.ts forks a fixture, worker-entry.ts is bundled rather than imported, and both are free of win32. Generalizing replaced that with "caller-owned escape coverage", and the Windows call site does not restate it, so the argument is now nowhere. Move it back above windowsPackageSourceEntrypoints.
Summary
The workflow exits before Xvfb allocation when the changed files reach zero specs. Selection-authority files and full plans run all 36 spec files.
Fixes #4478
Measured effect
A replay over the latest 300 first-parent commits found 169 E2E-triggering changes. Test invocations fell from 17,914 to 7,583, a 57.67% reduction. The selection histogram was 57 zero-spec changes, 30 single-spec changes, 10 two-spec changes, one three-spec change, one seven-spec change, and 70 full plans. The scheduled escape run adds one 106-test execution per day.
Verification
e2e/sidebar-geometry.spec.ts: 2/2 passednpm run format:checknpm run lintnpm run buildnpm run typechecknpx knip --workspace apps/desktopnpx knip --workspace packages/uinpm run check:asf-headersnpm run check:release: 191/191 passedactionlint .github/workflows/ci.ymlgit diff --checkThe repository-wide
npm testrun reached macOS environment failures in unmodified Runtime sandbox, Runtime Host fixture, and Desktop shell-env suites.