Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses IPv6/IPv4 resolution issues in the E2E test infrastructure by changing default URLs from localhost to 127.0.0.1 and adding a comprehensive CI/CD hang remediation plan. While the PR title indicates "update comments for clarity," the changes include both functional code modifications (URL defaults) and extensive documentation additions.
Changes:
- Changed all default URLs from
localhostto127.0.0.1in test configuration files to avoid IPv6/IPv4 resolution conflicts where Node.js/Playwright might prefer::1(IPv6) while Docker binds to0.0.0.0(IPv4) - Added explanatory comments in
playwright.config.jsdocumenting the IPv4 loopback address rationale - Added comprehensive 946-line CI/CD hanging issue remediation plan with 4-phase implementation strategy, rollback procedures, and verification steps
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/global-setup.ts | Changed 5 default URL fallbacks from localhost to 127.0.0.1 for getBaseURL(), checkCaddyAdminHealth(), waitForContainer(), checkEmergencyServerHealth(), and emergencySecurityReset() functions |
| playwright.config.js | Changed baseURL default from localhost:8080 to 127.0.0.1:8080 and added multi-line comment explaining IPv4 loopback rationale; added commented PHASE 1 stdout/stderr pipe configuration |
| docs/plans/ci_hang_remediation.md | Added comprehensive 946-line remediation plan detailing root causes (I/O buffer deadlock, resource starvation, signal handling gaps), 4-phase implementation strategy, verification procedures, rollback plans, and success criteria |
| * but the Docker container binds to 0.0.0.0 (IPv4). | ||
| */ | ||
| baseURL: process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8080', | ||
| baseURL: process.env.PLAYWRIGHT_BASE_URL || 'http://127.0.0.1:8080', |
There was a problem hiding this comment.
This is a functional code change (default URL changed from localhost to 127.0.0.1), not just a comment update as the PR title suggests. The change is valid for avoiding IPv6/IPv4 resolution issues, but the PR description should acknowledge these are functional changes.
| */ | ||
| function getBaseURL(): string { | ||
| return process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8080'; | ||
| return process.env.PLAYWRIGHT_BASE_URL || 'http://127.0.0.1:8080'; |
There was a problem hiding this comment.
The PR title describes this as "update comments for clarity" but this line contains a functional code change, not just a comment update. The default URL has changed from localhost to 127.0.0.1. This is an IPv4-specific loopback address that bypasses DNS resolution and potential IPv6 conflicts. While this is a valid fix for the IPv6/IPv4 resolution issues described in the new comments in playwright.config.js, the PR description should acknowledge this as a functional code change, not merely a comment clarification.
| */ | ||
| async function checkCaddyAdminHealth(): Promise<boolean> { | ||
| const caddyAdminHost = process.env.CADDY_ADMIN_HOST || 'http://localhost:2019'; | ||
| const caddyAdminHost = process.env.CADDY_ADMIN_HOST || 'http://127.0.0.1:2019'; |
There was a problem hiding this comment.
This is a functional code change (default URL changed from localhost to 127.0.0.1), not just a comment update as the PR title suggests. The change is valid for avoiding IPv6/IPv4 resolution issues, but the PR description should acknowledge these are functional changes.
| */ | ||
| async function waitForContainer(maxRetries = 15, delayMs = 2000): Promise<void> { | ||
| const baseURL = process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8080'; | ||
| const baseURL = process.env.PLAYWRIGHT_BASE_URL || 'http://127.0.0.1:8080'; |
There was a problem hiding this comment.
This is a functional code change (default URL changed from localhost to 127.0.0.1), not just a comment update as the PR title suggests. The change is valid for avoiding IPv6/IPv4 resolution issues, but the PR description should acknowledge these are functional changes.
| */ | ||
| async function checkEmergencyServerHealth(): Promise<boolean> { | ||
| const emergencyHost = process.env.EMERGENCY_SERVER_HOST || 'http://localhost:2020'; | ||
| const emergencyHost = process.env.EMERGENCY_SERVER_HOST || 'http://127.0.0.1:2020'; |
There was a problem hiding this comment.
This is a functional code change (default URL changed from localhost to 127.0.0.1), not just a comment update as the PR title suggests. The change is valid for avoiding IPv6/IPv4 resolution issues, but the PR description should acknowledge these are functional changes.
|
|
||
| const emergencyToken = process.env.CHARON_EMERGENCY_TOKEN; | ||
| const baseURL = process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8080'; | ||
| const baseURL = process.env.PLAYWRIGHT_BASE_URL || 'http://127.0.0.1:8080'; |
There was a problem hiding this comment.
This is a functional code change (default URL changed from localhost to 127.0.0.1), not just a comment update as the PR title suggests. The change is valid for avoiding IPv6/IPv4 resolution issues, but the PR description should acknowledge these are functional changes.
❌ 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 |
2 participants
No description provided.