[RUM-15522] Electron OS & Version testing - #204
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
🔗 Commit SHA: ac16222 | Docs | View more details | Give us feedback! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review — Score: 4.7 / 5
This PR adds a well-structured cross-OS, cross-Electron-version compatibility matrix with config-driven fixture materialization, generated GitLab child pipelines, and solid script-level test coverage. The refactor consolidates integration Playwright projects and packager-copy variants under e2e/compatibility/config.json instead of ad-hoc per-app wiring, and the SDK git-ref worktree flow is thoughtfully isolated. I would approve.
Why 4.7: Clear architecture (config.json as source of truth, generated apps per target/variant, prefetch + version assertion, CI generator with filters), thorough unit tests for the new scripts, and documentation updates in e2e/compatibility/README.md, docs/TESTING.md, and docs/DEVELOPMENT.md. Prior Codex review found no major issues; independent review of the current head agrees.
Why not 5: A few workflow/documentation gaps remain — new script entry points are not reflected in scripts/README.md, and packager-copy plus Windows payload regression coverage now depends on scheduled compatibility runs rather than every-PR integration CI.
Findings
- [Minor] scripts/README.md not updated — New compatibility script entry points are absent from the scripts reference despite the docs policy for workflow changes.
- [Minor] packager-copy coverage is scheduled-only — Non-default variants (packager-copy, Windows payload extraction) no longer run in every-PR integration CI.
- [Nit] Playwright config skips config validation —
playwright.config.tsimportsconfig.jsondirectly instead of reusingvalidateCompatibilityConfig().
Architectural flow
sequenceDiagram
participant Dev as Developer or schedule
participant Parent as GitLab parent pipeline
participant Gen as generate-compatibility-pipeline
participant Child as Generated matrix job
participant Init as test:compatibility:init
participant PW as Playwright suite
Dev->>Parent: COMPATIBILITY_TESTS=true
Parent->>Gen: yarn test:compatibility:ci:generate
Gen-->>Parent: generated.gitlab-ci.yml artifact
Parent->>Child: trigger child pipeline
Child->>Child: yarn install with ELECTRON_SKIP_BINARY_DOWNLOAD
Child->>Init: materialize apps and pack SDK
Init->>Init: prefetch Electron artifact
Child->>PW: yarn test:compatibility target
Note over PW: DD_ELECTRON_COMPATIBILITY_ROOT selects generated fixtures
PW->>PW: assertExpectedElectronVersion on each launch
Before: Every-PR CI exercised E2E and integration apps at the repository's current Electron version, with packager-copy and Windows payload cases wired ad hoc into Playwright projects and fixture apps.
After: Every-PR CI still runs the default variant only. Scheduled compatibility pipelines generate an environment × target child matrix that materializes pinned-Electron fixture copies (including packager-copy variants), optionally packages the SDK from an independent git ref, prefetches Electron artifacts, and runs the same Playwright scenarios with runtime version assertions.
Sent by Cursor Automation: electron-sdk reviews
| })) | ||
| ...integrationAppTemplates.flatMap((app) => | ||
| app.variants | ||
| .filter((variant) => compatibilityRun || variant.id === 'default') |
There was a problem hiding this comment.
[Minor] packager-copy coverage is scheduled-only
This filter keeps every-PR integration CI on the default variant only. That is reasonable for CI cost, but it means regressions in packager-copy fixtures (including the Windows unsigned payload extraction scenario) will surface only in scheduled compatibility jobs, not on ordinary PRs. Worth calling out explicitly in rollout docs so reviewers know where that signal moved.
| @@ -1,20 +1,24 @@ | |||
| import { defineConfig } from '@playwright/test'; | |||
| import type { IntegrationFixtures } from './integration/lib/integrationFixture'; | |||
| import compatibilityConfig from './compatibility/config.json'; | |||
There was a problem hiding this comment.
[Nit] Playwright config bypasses config validation
scripts/lib/compatibility.ts validates config.json thoroughly, but Playwright imports the JSON directly here. A malformed template entry (for example an unsupported modes value) will fail at Playwright startup with a generic error rather than the clearer validation message from loadCompatibilityConfig(). Low risk while config changes stay paired with the generator tests, but sharing validation would tighten the feedback loop.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac16222728
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const artifactPath = await downloadArtifact({ | ||
| arch: process.arch, | ||
| artifactName: 'electron', | ||
| downloader, | ||
| platform: process.platform, | ||
| version: electron.version, |
There was a problem hiding this comment.
When preparing electron-45-nightly, whose dependency is electron-nightly, this call supplies only the version and uses @electron/get's default Electron release location. Nightly binaries are published from the separate Electron nightlies release feed, so the prefetch receives a 404 and aborts initialization before any generated application is installed or tested on every configured OS. Select the mirror based on electron.dependency (or otherwise use the nightly package's download configuration).
Useful? React with 👍 / 👎.
| printLog('Packing the SDK from the current checkout'); | ||
| command`yarn pack --out ${destination}`.withCurrentWorkingDirectory(getRepositoryRoot()).withLogs().run(); |
There was a problem hiding this comment.
In every Windows compatibility job, preparing the SDK reaches this call with commandName === "yarn", but command() uses spawnSync without a shell and does not resolve the Windows yarn.cmd Corepack shim. This fails before the generated apps can be installed or tested; the new runLoggedCommand() wrapper already handles the same constraint by appending .cmd on Windows, but nested calls through command() do not. Apply equivalent executable resolution in the shared command runner.
Useful? React with 👍 / 👎.
1 participant
Motivation
This PR pipeline validates the SDK against the repository’s current Electron version, primarily on Linux. That gives us good regression coverage for day-to-day development, but it does not tell us whether the SDK continues to work across supported Electron versions or on macOS and Windows.
This became particularly relevant with the Windows packaging issue addressed in #182, where loose runtime dependencies caused an MSIX extraction failure because of legacy Windows path limits.
This PR adds a scheduled compatibility suite that reuses our existing E2E and integration scenarios across explicitly maintained Electron versions and all three supported operating systems, without multiplying the matrix on every PR.
Electron release schedule: https://releases.electronjs.org/schedule
Changes
This PR introduces a configuration-driven Electron compatibility test harness.
The main additions are:
e2e/compatibility/config.json, with one job per environment and Electron target.COMPATIBILITY_TESTS=true.DD_ELECTRON_COMPATIBILITY_ENVIRONMENTSDD_ELECTRON_COMPATIBILITY_TARGETSDD_ELECTRON_SDK_GIT_REFpackages the SDK from an isolated Git worktree without changing the harness checkout.The compatibility initializer now materializes disposable copies of the existing minimal E2E app and integration apps for each target. Each generated app:
Integration app behavior is modeled using templates, variants, and modes:
defaultvariant tests plugin-owned runtime dependency copying in development and packaged modes.packager-copyvariant setscopyRuntimeDependencies: falseand tests packaged applications where the packager owns dependency staging.This PR also adds a Windows-specific regression scenario for #182. It verifies that the
electron-builder-vitepackager-copy output does not contain loosenode_modules, then uses Windows PowerShell 5.1 to archive and extract the unsigned payload under a long temporary path.To make the matrix more reliable and easier to investigate:
@electron/getwith a custom native-fetch downloader that supports retries and partial-download resumption.@electron/getstill performs checksum verification and places artifacts in its standard cache.logs/.Documentation and unit coverage were added for the configuration, selectors, CI generation, overrides, SDK source selection, command logging, retries, and custom downloader.
Test instructions
Install the repository dependencies:
Run the unit suite:
Generate and run the minimal E2E compatibility app:
Generate Forge Webpack and test both runtime dependency strategies:
The initialization command creates both
defaultandpackager-copyapp directories.To test an SDK from another branch, tag, or commit while keeping the harness on the current checkout:
Preview a small generated child pipeline:
Inspect the generated file at:
To exercise the CI workflow, manually start a GitLab pipeline on this branch with:
Leave the environment and target filters empty to generate the complete matrix.
On Windows, the payload extraction regression can be isolated with:
yarn test:compatibility:init electron-41 --template electron-builder-vite yarn test:compatibility electron-41 \ --template=electron-builder-vite \ --variant=packager-copy \ --mode=packaged \ --grep="@windows"