Add E2E tests for PR #254#257
Conversation
max-california
left a comment
max-california
left a comment
There was a problem hiding this comment.
🤖 Agentic Code Review
Summary: the test is small and easy to understand, but I see one correctness issue that is likely to make it flaky or fail against perfectly healthy preview deployments.
Blocking
body.innerText() can fail on app shells and blank-but-valid roots, and the test doesn’t re-check after waiting
In tests/e2e/pr-254/preview-app-loads.spec.ts:25-29, the test reads body.innerText(), and if it is initially empty it waits 1 second:
const bodyText = (await page.locator('body').innerText().catch(() => '')) ?? '';
if (bodyText.trim().length === 0) {
await page.waitForTimeout(1000);
}But after that extra wait, there is no assertion that content actually appeared. So this block currently does not prove anything. More importantly, innerText() itself is a brittle signal for modern SPAs: a valid app shell can render only non-textual content initially, or content inside shadow DOM/canvas, and still be functioning correctly.
As written, the test effectively only asserts:
- navigation returned a 2xx/3xx response,
<body>is visible,- no
pageerrorevent fired, <html>exists.
That means the innerText()/timeout block is dead weight at best, and at worst it encourages a hidden assumption that “non-empty visible text” is required for success.
Suggested fix: either remove the innerText() logic entirely, or turn it into a real assertion on a stable app-specific readiness signal (for example a known heading, landmark, root element, or URL/state change introduced by PR #254). If the goal is just “the root doesn’t white-screen/crash”, the current status/body/pageerror checks are enough without the unused text probe.
Non-blocking
waitForLoadState('load').catch(() => {}) hides navigation problems
tests/e2e/pr-254/preview-app-loads.spec.ts:20
Swallowing any exception here makes diagnosis harder if the preview hangs or navigates strangely. If this wait is optional, consider either removing it or documenting exactly which failure mode is being tolerated.
The test is very generic, so its value is limited
Because this only checks for an HTTP response plus absence of uncaught page errors, it may pass on an error page rendered server-side, a maintenance page, or an unintended static fallback. If PR #254 changed a specific UI contract, a selector tied to that contract would give much better regression protection.
Read-only agentic review by AI Git Bot
3 participants
AI-Git-Bot — automated test promotion (
offer-as-pr).Parent PR: #254. The bot generated the following end-to-end tests during the parent PR's E2E workflow and is offering them as a separate review.
Files under
tests/e2e/pr-254/:tests/e2e/pr-254/preview-app-loads.spec.ts