-
-
Notifications
You must be signed in to change notification settings - Fork 7
Optimize E2E Testing Suite for CI Efficiency #462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
26b2854
e7b97c6
65c34d2
191fa0c
2438740
125cded
6122e79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,28 @@ jobs: | |
| test: | ||
| timeout-minutes: 60 | ||
| runs-on: ubuntu-latest | ||
|
|
||
|
|
||
| services: | ||
| postgres: | ||
| image: postgres:15 | ||
| env: | ||
| POSTGRES_PASSWORD: postgres | ||
| POSTGRES_DB: postgres | ||
| ports: | ||
| - 5432:5432 | ||
| # Wait for postgres to be ready | ||
| options: >- | ||
| --health-cmd pg_isready | ||
| --health-interval 10s | ||
| --health-timeout 5s | ||
| --health-retries 5 | ||
|
|
||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| shardIndex: [1, 2, 3, 4] | ||
| shardTotal: [4] | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
@@ -26,36 +47,56 @@ jobs: | |
| - name: Install Playwright Browsers | ||
| run: bunx playwright install --with-deps | ||
|
|
||
| - name: Build application | ||
| run: bun run build | ||
| - name: Run migrations | ||
| run: bun run db:migrate | ||
| env: | ||
| # Add any required environment variables for build | ||
| NODE_ENV: production | ||
| DATABASE_URL: postgresql://postgres:postgres@localhost:5432/postgres | ||
|
|
||
| - name: Start application | ||
| run: | | ||
| bun run start & | ||
| sleep 10 | ||
| # Wait for the server to be ready | ||
| npx wait-on http://localhost:3000 --timeout 60000 | ||
|
|
||
| - name: Run Playwright tests | ||
| run: bun run test:e2e | ||
| - name: Run Playwright tests (Shard ${{ matrix.shardIndex }}/${{ matrix.shardTotal }}) | ||
| run: bunx playwright test --shard=${{ matrix.shardIndex }}/${{ matrix.shardTotal }} | ||
| env: | ||
| CI: true | ||
| NODE_ENV: production | ||
| AUTH_DISABLED_FOR_DEV: true | ||
|
Comment on lines
59
to
60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: rg -n -C3 'AUTH_DISABLED_FOR_DEV' -t yaml -t js -t ts -t jsonRepository: QueueLab/QCX Length of output: 2661 Remove or clarify The flag is guarded in the code to only work when 🤖 Prompt for AI Agents |
||
| DATABASE_URL: postgresql://postgres:postgres@localhost:5432/postgres | ||
|
|
||
| - name: Upload test results | ||
| uses: actions/upload-artifact@v4 | ||
| if: always() | ||
| with: | ||
| name: playwright-report | ||
| path: playwright-report/ | ||
| retention-days: 30 | ||
| name: blob-report-${{ matrix.shardIndex }} | ||
| path: blob-report/ | ||
| retention-days: 1 | ||
|
|
||
| merge-reports: | ||
| if: always() | ||
| needs: [test] | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Upload test screenshots | ||
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: 1.3.5 | ||
|
|
||
| - name: Install dependencies | ||
| run: bun install --frozen-lockfile | ||
|
|
||
| - name: Download blobs | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| path: all-blobs | ||
| pattern: blob-report-* | ||
| merge-multiple: true | ||
|
|
||
| - name: Merge reports | ||
| run: bunx playwright merge-reports --reporter html ./all-blobs | ||
|
|
||
| - name: Upload merged report | ||
| uses: actions/upload-artifact@v4 | ||
| if: failure() | ||
| with: | ||
| name: playwright-screenshots | ||
| path: test-results/ | ||
| retention-days: 7 | ||
| name: playwright-report | ||
| path: playwright-report/ | ||
| retention-days: 30 | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,10 @@ | |||||
| "lint": "next lint", | ||||||
| "db:migrate": "cross-env EXECUTE_MIGRATIONS=true bun lib/db/migrate.ts", | ||||||
| "test:e2e": "playwright test", | ||||||
| "test:e2e:smoke": "playwright test --grep @smoke", | ||||||
| "test:e2e:parallel": "playwright test --workers=4", | ||||||
| "test:e2e:chromium": "playwright test --project=chromium", | ||||||
| "test:e2e:shard": "playwright test --shard=$SHARD_INDEX/$SHARD_TOTAL", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cross-platform compatibility concern with environment variable syntax. The Proposed fix for cross-platform support (if needed)- "test:e2e:shard": "playwright test --shard=$SHARD_INDEX/$SHARD_TOTAL",
+ "test:e2e:shard": "cross-env playwright test --shard=${SHARD_INDEX}/${SHARD_TOTAL}",Note: This still won't work on native Windows CMD. If Windows support is required, consider a small Node script wrapper. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| "test:e2e:ui": "playwright test --ui", | ||||||
| "test:e2e:headed": "playwright test --headed", | ||||||
| "test:e2e:debug": "playwright test --debug" | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,11 +2,12 @@ import { defineConfig, devices } from '@playwright/test'; | |||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| export default defineConfig({ | ||||||||||||||||||||||||||||||||||||
| testDir: './tests', | ||||||||||||||||||||||||||||||||||||
| globalSetup: require.resolve('./tests/global-setup'), | ||||||||||||||||||||||||||||||||||||
| fullyParallel: true, | ||||||||||||||||||||||||||||||||||||
| forbidOnly: !!process.env.CI, | ||||||||||||||||||||||||||||||||||||
| retries: process.env.CI ? 2 : 0, | ||||||||||||||||||||||||||||||||||||
| workers: process.env.CI ? 1 : undefined, | ||||||||||||||||||||||||||||||||||||
| reporter: 'html', | ||||||||||||||||||||||||||||||||||||
| workers: process.env.CI ? 2 : undefined, | ||||||||||||||||||||||||||||||||||||
| reporter: process.env.CI ? [['list'], ['github'], ['blob']] : 'html', | ||||||||||||||||||||||||||||||||||||
| use: { | ||||||||||||||||||||||||||||||||||||
| baseURL: 'http://localhost:3000', | ||||||||||||||||||||||||||||||||||||
| trace: 'on-first-retry', | ||||||||||||||||||||||||||||||||||||
|
|
@@ -15,28 +16,35 @@ export default defineConfig({ | |||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||
| name: 'chromium', | ||||||||||||||||||||||||||||||||||||
| use: { ...devices['Desktop Chrome'] }, | ||||||||||||||||||||||||||||||||||||
| testIgnore: /mobile\.spec\.ts/, | ||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||
| name: 'firefox', | ||||||||||||||||||||||||||||||||||||
| use: { ...devices['Desktop Firefox'] }, | ||||||||||||||||||||||||||||||||||||
| testIgnore: /mobile\.spec\.ts/, | ||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||
| name: 'webkit', | ||||||||||||||||||||||||||||||||||||
| use: { ...devices['Desktop Safari'] }, | ||||||||||||||||||||||||||||||||||||
| testIgnore: /mobile\.spec\.ts/, | ||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||
| name: 'Mobile Chrome', | ||||||||||||||||||||||||||||||||||||
| use: { ...devices['Pixel 5'] }, | ||||||||||||||||||||||||||||||||||||
| testMatch: /mobile\.spec\.ts/, | ||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||
| name: 'Mobile Safari', | ||||||||||||||||||||||||||||||||||||
| use: { ...devices['iPhone 12'] }, | ||||||||||||||||||||||||||||||||||||
| testMatch: /mobile\.spec\.ts/, | ||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||
| /* webServer: { | ||||||||||||||||||||||||||||||||||||
| command: process.env.CI ? 'npm run build && npm run start' : 'npm run dev', | ||||||||||||||||||||||||||||||||||||
| webServer: { | ||||||||||||||||||||||||||||||||||||
| command: process.env.CI ? 'bun run build && bun run start' : 'bun run dev', | ||||||||||||||||||||||||||||||||||||
| url: 'http://localhost:3000', | ||||||||||||||||||||||||||||||||||||
| reuseExistingServer: !process.env.CI, | ||||||||||||||||||||||||||||||||||||
| timeout: 600000, | ||||||||||||||||||||||||||||||||||||
| }, */ | ||||||||||||||||||||||||||||||||||||
| stdout: 'pipe', | ||||||||||||||||||||||||||||||||||||
| stderr: 'pipe', | ||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
3
to
49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You set This can overload CI resources and make tests less reliable (especially with WebGL/map tests and any shared external dependencies). If the goal is 4-way parallelization total, you typically want either sharding or increased workers, not both. SuggestionDecide on one primary scaling mechanism:
If you keep both, cap workers when sharding is enabled: const isSharded = !!process.env.SHARD_INDEX && !!process.env.SHARD_TOTAL;
workers: process.env.CI ? (isSharded ? 1 : 4) : undefined,Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
Comment on lines
42
to
49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right now Also, the workflow sets SuggestionParameterize
Comment on lines
+42
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider reducing the webServer timeout or documenting the rationale. The 10-minute (600000ms) timeout is quite generous. While build times can vary, this may mask issues where the server fails to start within a reasonable window. If typical build + start times are known, consider:
The ♻️ Suggested adjustment with documentation webServer: {
command: process.env.CI ? 'bun run build && bun run start' : 'bun run dev',
url: 'http://localhost:3000',
reuseExistingServer: !process.env.CI,
- timeout: 600000,
+ timeout: 180000, // 3 minutes - adjust based on typical build times
stdout: 'pipe',
stderr: 'pipe',
},📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: QueueLab/QCX
Length of output: 75
🏁 Script executed:
Repository: QueueLab/QCX
Length of output: 1732
🏁 Script executed:
Repository: QueueLab/QCX
Length of output: 3213
🏁 Script executed:
Repository: QueueLab/QCX
Length of output: 38
🏁 Script executed:
Repository: QueueLab/QCX
Length of output: 4904
Fix SSL configuration for localhost Postgres in CI migrations.
lib/db/migrate.tsforces SSL for all connections, but the GitHub Actionspostgres:15service doesn't support SSL by default. This causesbun run db:migrateto fail in CI with a protocol mismatch error. Make SSL conditional on non-localhost connections.🛠️ Suggested fix for
lib/db/migrate.ts🧰 Tools
🪛 Checkov (3.2.334)
[medium] 53-54: Basic Auth Credentials
(CKV_SECRET_4)
🤖 Prompt for AI Agents