-
Notifications
You must be signed in to change notification settings - Fork 14
Optimize CI: separate fast tests from live infrastructure tests #315
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?
Conversation
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run edge-worker:test:live |
❌ Failed | 4m 16s | View ↗ |
nx run cli:test:live |
❌ Failed | 3s | View ↗ |
nx run client:test:live |
✅ Succeeded | 2m 44s | View ↗ |
nx affected -t build --configuration=production... |
✅ Succeeded | 22s | View ↗ |
nx affected -t lint typecheck test --parallel -... |
✅ Succeeded | 1m 11s | View ↗ |
nx run core:test:live |
✅ Succeeded | 1s | View ↗ |
nx run core:db:verify |
✅ Succeeded | 3m 11s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-11-08 13:18:20 UTC
.github/workflows/ci.yml
Outdated
| edge-worker-e2e: | ||
| runs-on: ubuntu-latest | ||
| env: |
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.
The edge-worker-e2e job should include a dependency on the db-verification job to maintain consistency with the other test jobs. Adding needs: [db-verification] would ensure that database verification completes successfully before running the E2E tests, preventing potential issues from unverified database changes.
| edge-worker-e2e: | |
| runs-on: ubuntu-latest | |
| env: | |
| edge-worker-e2e: | |
| runs-on: ubuntu-latest | |
| needs: [db-verification] | |
| env: |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
b3f3430 to
67d3641
Compare
67d3641 to
14e5260
Compare
14e5260 to
0a2a37d
Compare
| - name: Run core live tests | ||
| run: pnpm nx run core:test:live |
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.
The core-live-tests job unconditionally runs pnpm nx run core:test:live without checking if core is affected by the changes. This is inconsistent with client-live-tests (line 119-121) and edge-worker-live-tests (line 153-155), which both check if they are affected before running.
This means core's live tests will run on every CI run, even when core is not affected by any changes, wasting CI resources and time.
Fix: Add an affected check similar to the other packages:
- name: Check if core is affected
id: check-affected
run: |
if pnpm nx show projects --affected --base="$NX_BASE" --head="$NX_HEAD" | grep -q "^core$"; then
echo "affected=true" >> $GITHUB_OUTPUT
echo "Core is affected by changes"
else
echo "affected=false" >> $GITHUB_OUTPUT
echo "Core is not affected by changes - skipping"
fi
- name: Run core live tests
if: steps.check-affected.outputs.affected == 'true'
run: pnpm nx run core:test:live| - name: Run core live tests | |
| run: pnpm nx run core:test:live | |
| - name: Check if core is affected | |
| id: check-affected | |
| run: | | |
| if pnpm nx show projects --affected --base="$NX_BASE" --head="$NX_HEAD" | grep -q "^core$"; then | |
| echo "affected=true" >> $GITHUB_OUTPUT | |
| echo "Core is affected by changes" | |
| else | |
| echo "affected=false" >> $GITHUB_OUTPUT | |
| echo "Core is not affected by changes - skipping" | |
| fi | |
| - name: Run core live tests | |
| if: steps.check-affected.outputs.affected == 'true' | |
| run: pnpm nx run core:test:live |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
🔍 Preview Deployment: Website✅ Deployment successful! 🔗 Preview URL: https://pr-315.pgflow.pages.dev 📝 Details:
_Last updated: _ |
0a2a37d to
cc056b9
Compare
dd77727 to
c42ad3d
Compare
| # ─────────────────────────────────────── 2. EDGE-WORKER E2E ────────────────────────────────────── | ||
| edge-worker-e2e: | ||
| # ─────────────────────────────────────── 3. CLI LIVE TESTS ────────────────────────────────────── | ||
| cli-live-tests: |
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.
Consider running live tests sequentially instead of in parallel by making cli-live-tests depend on db-verification, core-live-tests depend on cli-live-tests, client-live-tests depend on core-live-tests, and edge-worker-live-tests depend on client-live-tests. This would prevent resource conflicts at the cost of longer CI run times.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| - name: Run edge-worker live tests | ||
| if: steps.check-affected.outputs.affected == 'true' | ||
| run: pnpm nx affected -t test:e2e --parallel --base="$NX_BASE" --head="$NX_HEAD" | ||
| run: pnpm nx run edge-worker:test:live |
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.
Add a timeout to prevent the edge-worker live tests from being terminated prematurely. Change to: run: pnpm nx run edge-worker:test:live --timeout=10m
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| core-live-tests: | ||
| needs: [db-verification] |
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.
The core-live-tests job should depend on fast-tests instead of db-verification to ensure it runs after fast-tests completes, creating a sequential chain of jobs.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| cd "$SUPABASE_DIR" | ||
|
|
||
| if pnpm supabase status &>/dev/null; then | ||
| echo "✓ Supabase already running in $SUPABASE_DIR" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "Starting Supabase in $SUPABASE_DIR..." | ||
| pnpm supabase start |
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.
Add a locking mechanism to prevent multiple processes from starting Supabase simultaneously. This could be implemented using flock or a simple file-based lock to ensure only one process can start Supabase at a time.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
c42ad3d to
3a5298f
Compare
pkgs/cli/scripts/test-install
Outdated
| # Initialize a fresh Supabase project | ||
| echo "🏗️ Creating new Supabase project" | ||
| npx -y supabase@latest init --force --with-vscode-settings --with-intellij-settings | ||
| supabase init --force --with-vscode-settings --with-intellij-settings |
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.
This line is missing the '--force' flag which is causing the test to fail with an EEXIST error when trying to create a directory that already exists. The command should be 'supabase init --force --with-vscode-settings --with-intellij-settings' to match the other similar scripts in the codebase.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| "test:e2e": { | ||
| "executor": "nx:run-commands", | ||
| "dependsOn": ["supabase:reset", "serve:functions:e2e"], | ||
| "dependsOn": ["supabase:reset", "sync-e2e-deps"], | ||
| "local": true, | ||
| "inputs": ["default", "^production"], | ||
| "options": { | ||
| "cwd": "pkgs/edge-worker", | ||
| "commands": [ | ||
| "../../scripts/ensure-supabase.sh .", | ||
| "deno test --config deno.test.json --allow-all --env=supabase/functions/.env tests/e2e/" | ||
| ], | ||
| "parallel": false |
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.
Add retry mechanism for the supabase db reset command to handle potential interruptions. The current implementation fails with SIGINT (code 130) which suggests the command is being interrupted, possibly due to race conditions with other jobs.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| #!/usr/bin/env bash | ||
| set -e | ||
|
|
||
| # Usage: ensure-supabase.sh <path-to-supabase-parent-dir> | ||
| if [ -z "$1" ]; then | ||
| echo "ERROR: Supabase directory path required" | ||
| echo "Usage: ensure-supabase.sh <path-to-supabase-parent-dir>" | ||
| exit 1 | ||
| fi | ||
|
|
||
| SUPABASE_DIR="$1" | ||
|
|
||
| if [ ! -d "$SUPABASE_DIR/supabase" ]; then | ||
| echo "ERROR: No supabase/ directory found in $SUPABASE_DIR" | ||
| exit 1 | ||
| fi | ||
|
|
||
| cd "$SUPABASE_DIR" | ||
|
|
||
| if pnpm supabase status &>/dev/null; then | ||
| echo "✓ Supabase already running in $SUPABASE_DIR" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "Starting Supabase in $SUPABASE_DIR..." | ||
| pnpm supabase start |
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.
Enhance script to handle potential race conditions by adding retries and better error handling. The script should be more resilient when multiple CI jobs call it simultaneously.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
3a5298f to
f0eb6fa
Compare
| - name: Install sqruff | ||
| uses: quarylabs/install-sqruff-cli-action@main |
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.
This step is failing because the 'quarylabs/install-sqruff-cli-action@main' action cannot successfully download and install the sqruff-linux-x86_64-musl.tar.gz file. Either pin this action to a specific stable version (e.g., 'quarylabs/install-sqruff-cli-action@v1.0.0') instead of using the potentially unstable '@main' reference, or temporarily remove this step until the upstream action is fixed.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| "commands": [ | ||
| "../../scripts/ensure-supabase.sh .", | ||
| "mkdir -p supabase/migrations/", | ||
| "rm -f supabase/migrations/*.sql", | ||
| "cp ../core/supabase/migrations/*.sql supabase/migrations/", |
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.
The 'supabase db reset' command is likely hanging or timing out. We should add a timeout and potentially retry logic. Replace the last command with 'timeout 60s supabase db reset || (echo "Retrying database reset..." && timeout 60s supabase db reset)' to give the command a 60-second timeout and one retry attempt if it fails.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
f0eb6fa to
dc614a9
Compare
| if pnpm supabase status &>/dev/null; then | ||
| echo "✓ Supabase already running in $SUPABASE_DIR" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "Starting Supabase in $SUPABASE_DIR..." | ||
| pnpm supabase start |
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.
The script doesn't handle timeouts or potential failures when starting Supabase. It should include a timeout mechanism and retry logic to prevent hanging indefinitely, which is likely causing the SIGINT termination in CI.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
dc614a9 to
5ba8b03
Compare
Split test targets into two categories: - test: Fast tests without infrastructure (vitest, types) - test:live: Tests requiring live infrastructure (pgtap, integration) Enable Nx Cloud caching for verification tasks: - Remove local: true from verify-* targets - Add db:verify meta-target for verification pipeline - Verification results now cached across CI jobs Restructure CI workflow: - Job 1: db-verification (runs once, caches to Nx Cloud) - Job 2: fast-tests (restores cache, runs all packages in parallel) - Job 3-5: *-live-tests (separate jobs per package infrastructure) - Job 6: edge-worker-e2e (unchanged) Benefits: - verify-migrations runs once instead of multiple times - Fast tests complete in ~2-3 min with full parallelization - Simple commands: nx affected -t test (fast), nx affected -t test:live (infra) - Nx handles dependency resolution and caching automatically 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
5ba8b03 to
ec56e39
Compare

Split test targets into two categories:
Enable Nx Cloud caching for verification tasks:
Restructure CI workflow:
Benefits:
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com