Conversation
…es and cache checks
There was a problem hiding this comment.
Pull request overview
This PR adds debugging instrumentation and a separate system dependencies installation step to the Playwright browser installation process in the E2E test workflow. The changes are identical across all three browser jobs (Chromium, Firefox, and WebKit), adding verbose logging to diagnose Playwright installation behavior.
Changes:
- Added separate "Install Playwright system dependencies" step before each browser installation
- Enhanced browser installation steps with diagnostic echo statements, cache directory listings, and file searches
- Added exit code verification for installation commands
| echo "✅ Install command completed (exit code: $?)" | ||
| echo "📁 Checking browser cache..." | ||
| ls -lR ~/.cache/ms-playwright/ 2>/dev/null || echo "Cache directory not found" | ||
| echo "🔍 Searching for chromium executable..." | ||
| find ~/.cache/ms-playwright -name "*chromium*" -o -name "*chrome*" 2>/dev/null | head -10 || echo "No chromium files found" |
There was a problem hiding this comment.
The verbose debugging output (lines 208-214) is useful for troubleshooting Playwright installation issues, but appears to be temporary diagnostic code.
Best practices for CI debugging instrumentation:
- Add context in the PR description explaining what issue is being investigated
- Remove once resolved: This verbose output adds noise to CI logs and should be removed after the issue is fixed
- Consider conditional debugging: Use a workflow input or environment variable to enable/disable verbose mode rather than having it always-on
If this is addressing a specific installation failure, please document the issue in the PR description and create a follow-up task to remove this debugging code once the root cause is identified and fixed.
| echo "✅ Install command completed (exit code: $?)" | |
| echo "📁 Checking browser cache..." | |
| ls -lR ~/.cache/ms-playwright/ 2>/dev/null || echo "Cache directory not found" | |
| echo "🔍 Searching for chromium executable..." | |
| find ~/.cache/ms-playwright -name "*chromium*" -o -name "*chrome*" 2>/dev/null | head -10 || echo "No chromium files found" |
| - name: Install Playwright system dependencies | ||
| run: npx playwright install-deps firefox | ||
|
|
There was a problem hiding this comment.
The separate "Install Playwright system dependencies" step is redundant. The npx playwright install --with-deps firefox command already installs both the browser binary AND all required system dependencies.
Running npx playwright install-deps firefox beforehand duplicates this work, adds unnecessary CI execution time, and provides no benefit. The --with-deps flag is specifically designed to be a single command that handles everything.
Recommendation: Remove the separate "Install Playwright system dependencies" step entirely.
| - name: Install Playwright system dependencies | |
| run: npx playwright install-deps firefox |
| run: | | ||
| echo "📦 Installing Firefox..." | ||
| npx playwright install --with-deps firefox | ||
| echo "✅ Install command completed (exit code: $?)" |
There was a problem hiding this comment.
The exit code check on line 376 is misleading. The $? variable contains the exit code of the LAST command, which is the echo statement itself (not the npx playwright install command). This will always show exit code 0 even if Playwright installation failed.
To correctly capture the installation exit code, use:
npx playwright install --with-deps firefox
EXIT_CODE=$?
echo "✅ Install command completed (exit code: $EXIT_CODE)"However, if the primary goal is debugging, consider that a failed npx playwright install command will already cause the step to fail and stop the workflow. The exit code check adds limited value unless you're implementing custom error handling.
| echo "✅ Install command completed (exit code: $?)" | |
| EXIT_CODE=$? | |
| echo "✅ Install command completed (exit code: $EXIT_CODE)" |
| - name: Install Playwright system dependencies | ||
| run: npx playwright install-deps webkit | ||
|
|
There was a problem hiding this comment.
The separate "Install Playwright system dependencies" step is redundant. The npx playwright install --with-deps webkit command already installs both the browser binary AND all required system dependencies.
Running npx playwright install-deps webkit beforehand duplicates this work, adds unnecessary CI execution time, and provides no benefit. The --with-deps flag is specifically designed to be a single command that handles everything.
Recommendation: Remove the separate "Install Playwright system dependencies" step entirely.
| - name: Install Playwright system dependencies | |
| run: npx playwright install-deps webkit |
| echo "✅ Install command completed (exit code: $?)" | ||
| echo "📁 Checking browser cache..." | ||
| ls -lR ~/.cache/ms-playwright/ 2>/dev/null || echo "Cache directory not found" | ||
| echo "🔍 Searching for webkit executable..." | ||
| find ~/.cache/ms-playwright -name "*webkit*" -o -name "*MiniBrowser*" 2>/dev/null | head -10 || echo "No webkit files found" | ||
|
|
There was a problem hiding this comment.
The exit code check on line 542 is misleading. The $? variable contains the exit code of the LAST command, which is the echo statement itself (not the npx playwright install command). This will always show exit code 0 even if Playwright installation failed.
To correctly capture the installation exit code, use:
npx playwright install --with-deps webkit
EXIT_CODE=$?
echo "✅ Install command completed (exit code: $EXIT_CODE)"However, if the primary goal is debugging, consider that a failed npx playwright install command will already cause the step to fail and stop the workflow. The exit code check adds limited value unless you're implementing custom error handling.
| echo "✅ Install command completed (exit code: $?)" | |
| echo "📁 Checking browser cache..." | |
| ls -lR ~/.cache/ms-playwright/ 2>/dev/null || echo "Cache directory not found" | |
| echo "🔍 Searching for webkit executable..." | |
| find ~/.cache/ms-playwright -name "*webkit*" -o -name "*MiniBrowser*" 2>/dev/null | head -10 || echo "No webkit files found" | |
| EXIT_CODE=$? | |
| echo "✅ Install command completed (exit code: $EXIT_CODE)" | |
| echo "📁 Checking browser cache..." | |
| ls -lR ~/.cache/ms-playwright/ 2>/dev/null || echo "Cache directory not found" | |
| echo "🔍 Searching for webkit executable..." | |
| find ~/.cache/ms-playwright -name "*webkit*" -o -name "*MiniBrowser*" 2>/dev/null | head -10 || echo "No webkit files found" | |
| exit "$EXIT_CODE" |
| - name: Install Playwright system dependencies | ||
| run: npx playwright install-deps chromium | ||
|
|
||
| - name: Install Playwright Chromium | ||
| run: npx playwright install --with-deps chromium | ||
| run: | | ||
| echo "📦 Installing Chromium..." | ||
| npx playwright install --with-deps chromium | ||
| echo "✅ Install command completed (exit code: $?)" | ||
| echo "📁 Checking browser cache..." | ||
| ls -lR ~/.cache/ms-playwright/ 2>/dev/null || echo "Cache directory not found" | ||
| echo "🔍 Searching for chromium executable..." | ||
| find ~/.cache/ms-playwright -name "*chromium*" -o -name "*chrome*" 2>/dev/null | head -10 || echo "No chromium files found" |
There was a problem hiding this comment.
The PR title mentions "enhance Playwright installation steps" and "cache checks", but there is no explanation in the PR description about:
- What problem is being solved: Is there a current failure in Playwright browser installation? If so, what are the symptoms?
- Why these changes are needed: What issue do the debugging outputs help diagnose?
- Expected outcomes: Is this a permanent change or temporary diagnostic instrumentation?
- When to remove: If this is debugging code, when will it be removed?
Without this context, reviewers cannot assess:
- Whether the approach is correct for the actual problem
- If the changes should be permanent or temporary
- What success criteria indicate the issue is resolved
Please update the PR description to document the issue being addressed and the expected outcome of these changes.
| - name: Install Playwright system dependencies | ||
| run: npx playwright install-deps chromium | ||
|
|
There was a problem hiding this comment.
The separate "Install Playwright system dependencies" step is redundant. The npx playwright install --with-deps chromium command already installs both the browser binary AND all required system dependencies.
Running npx playwright install-deps chromium beforehand duplicates this work, adds unnecessary CI execution time, and provides no benefit. The --with-deps flag is specifically designed to be a single command that handles everything.
Recommendation: Remove the separate "Install Playwright system dependencies" step entirely.
| - name: Install Playwright system dependencies | |
| run: npx playwright install-deps chromium |
| run: | | ||
| echo "📦 Installing Chromium..." | ||
| npx playwright install --with-deps chromium | ||
| echo "✅ Install command completed (exit code: $?)" |
There was a problem hiding this comment.
The exit code check on line 210 is misleading. The $? variable contains the exit code of the LAST command, which is the echo statement itself (not the npx playwright install command). This will always show exit code 0 even if Playwright installation failed.
To correctly capture the installation exit code, use:
npx playwright install --with-deps chromium
EXIT_CODE=$?
echo "✅ Install command completed (exit code: $EXIT_CODE)"However, if the primary goal is debugging, consider that a failed npx playwright install command will already cause the step to fail and stop the workflow. The exit code check adds limited value unless you're implementing custom error handling.
| echo "✅ Install command completed (exit code: $?)" | |
| EXIT_CODE=$? | |
| echo "✅ Install command completed (exit code: $EXIT_CODE)" |
❌ E2E Test Results: FAILED (Split Browser Jobs)Some browser tests failed. Each browser runs independently. Browser Results (Sequential Execution)
Phase 1 Hotfix Active: Each browser runs in a separate job. One browser failure does not block others. 📊 View workflow run & download reports 🤖 Phase 1 Emergency Hotfix - See docs/plans/browser_alignment_triage.md |
No description provided.