feat: enable parallel Playwright workers for E2E tests#3509
Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
|
|
||
| - name: 🧪 Run E2E tests | ||
| run: pnpm exec playwright test --workers=1 | ||
| run: pnpm exec playwright test |
There was a problem hiding this comment.
we want to have the workers number have a single source of truth: the playwright config
frandiox
left a comment
There was a problem hiding this comment.
Funny because I actually tried spinning up multiple servers with different ports (that's why capturedUrl already existed) and it failed miserably 😂 (blaming Opus 4.5)
e2e/fixtures/server.ts
Outdated
| this.id = options.id; | ||
| this.storeKey = options.storeKey; | ||
| this.port = options.port ?? 3100; | ||
| this.port = options.port ?? 0; |
There was a problem hiding this comment.
is zero really a good fallback? undefined is easier to check for later
e2e/fixtures/server.ts
Outdated
| getUrl() { | ||
| return this.capturedUrl || `http://localhost:${this.port}`; | ||
| if (this.capturedUrl) return this.capturedUrl; | ||
| if (this.port === 0) { |
There was a problem hiding this comment.
like here, if we had undefined as a possible value, this would be clearer to check, zero feels a lil magic numbery
There was a problem hiding this comment.
Using port 0 is a POSIX convention, but yeah definitely magic numbery, thanks for catching that :) I've updated it here to not be a magic number
fredericoo
left a comment
There was a problem hiding this comment.
left some non blocking comments but looks great! lfg!!!
849a85e to
2502880
Compare
E2E tests were limited to 1 worker because every DevServer defaulted to port 3100, causing bind failures under parallel execution. The skeleton's npm script also includes --codegen which causes file write conflicts when multiple servers share the same directory. Key changes: - Default port changed from 3100 to 0 (OS-assigned) so port collision is structurally impossible - Spawn shopify hydrogen dev directly (not via npm run dev) to avoid --codegen file write conflicts - Process group kill via detached spawn + negative PID ensures child processes (vite, workerd) are terminated, not just the npm parent - stop() hardened: handles already-dead processes, captures PID upfront to avoid races, SIGKILL timeout always resolves the promise - getUrl() throws clear error instead of returning nonsensical http://localhost:0 - Server startup timeout increased from 60s to 120s for CPU contention during parallel startup - Workers: 3 in CI (ubuntu-latest: 2 vCPUs, 7GB RAM), 4 locally - CI job timeout increased from 15 to 20 minutes
2502880 to
4a724dc
Compare
…T constant Addresses PR feedback from Freddie: port 0 was doing double duty as both 'no port specified' (application concept) and 'let the OS pick' (POSIX convention). Now undefined means 'not specified' at the TypeScript level, and OS_ASSIGNED_PORT = 0 is only used at the spawn boundary where it has actual POSIX semantics. This eliminates the magic number and makes getUrl() checks more idiomatic TypeScript (=== undefined vs === 0).
4a724dc to
1aaced3
Compare
|
These findings are in files not modified by this PR and cannot be posted as inline comments.
The server is considered “started” only when the CLI output contains the substring "success":
When started, code sets |
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - No issues 📋 History❌ Failed → ✅ No issues |
Summary
DevServerdefaulted to port 3100, causing bind failures under parallel executionChanges
e2e/fixtures/server.ts(main changes):3100to0(OS-assigned) — port collision is now structurally impossibleshopify hydrogen devdirectly instead ofnpm run devto avoid--codegenfile write conflicts in sharedtemplates/skeleton/directorydetached: true+ negative PID ensures child processes (vite, workerd) are terminated, not just the npm parentstop()hardened: handles already-dead processes, captures PID upfront to avoid races, SIGKILL timeout always resolves the promisegetUrl()throws clear error instead of returning nonsensicalhttp://localhost:0playwright.config.ts:workers: process.env.CI ? 3 : 4(was1).github/workflows/ci.yml:--workers=1override (config is now single source of truth)Test plan