Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new Playwright E2E test case to container-logs.spec.ts that verifies the error UI is displayed when the observability service returns a 500 error. It also fixes the closing brace that was missing from the preceding "can filter logs" test, which would have caused the suite to be malformed.
Changes:
- Adds the
});closing brace that was missing from the "can filter logs" test - Adds a new E2E test "displays error card when observability service is unreachable" that intercepts the logs API with a mocked 500 response and asserts the error card UI (network issue message and "Try Again" button) is visible
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Wait for fetch to complete | ||
| await page.waitForTimeout(2000); | ||
|
|
||
| // Assert error card is visible | ||
| await expect(page.getByText(/network issue/i)).toBeVisible(); |
There was a problem hiding this comment.
The page.waitForTimeout(2000) on line 301 is a fixed-duration sleep used to wait for the mocked 500 response to propagate and the error UI to render. This is a flaky testing pattern — if the network or UI is slower than 2 seconds, the subsequent assertions will fail, and if it's much faster, the wait is wasteful.
A more deterministic approach is to use page.waitForResponse to wait for the intercepted request to be fulfilled before asserting on the UI. For example, you could capture the response promise before clicking "View Logs" and await it before proceeding to assertions. Alternatively, since the error UI is ultimately what you're checking, await expect(page.getByText(/network issue/i)).toBeVisible({ timeout: 10_000 }) without the fixed sleep is more reliable. The existing test on line 137 uses the same pattern and has the same concern.
| // Wait for fetch to complete | |
| await page.waitForTimeout(2000); | |
| // Assert error card is visible | |
| await expect(page.getByText(/network issue/i)).toBeVisible(); | |
| // Assert error card is visible (Playwright will auto-wait for this state) | |
| await expect(page.getByText(/network issue/i)).toBeVisible({ timeout: 10_000 }); |
🚀 Pull Request Overview
📌 Summary
🔗 Related Issues
Closes #
🧪 Changes
✅ What’s Included
❌ What’s Not Included
🧪 Testing
🔍 How Was This Tested?
🧪 Test Coverage
🧩 Breaking Changes
Does this PR introduce breaking changes?
If yes, describe the impact and migration steps:
🔐 Security Considerations
If checked, explain:
📝 Documentation
Does this PR require updates to documentation?
If yes, update the relevant locations:
/docsREADME.md🎨 UI/UX Considerations (If Applicable)
Screenshots / recordings (if applicable):
📦 Checklist Before Merge
👤 Contributor Notes (Optional)