[Test] Web acceptance stability#4506
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors Playwright acceptance tests to reduce network-response dependencies and adds comprehensive tests for the "Use API" documentation drawer. Changes include member-invite modal helpers, prompt-registry UI navigation, new Use API snippet tests for variants and deployments, and a slug validation assertion in evaluator creation. ChangesUse API Snippet Tests
Test Refactoring to UI-Based Waiting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
Bot
added
size:XS
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
web/oss/tests/playwright/acceptance/use-api/index.ts (1)
111-143: 💤 Low valueConsolidate the two drawer-open helpers.
openVariantUseApiDrawerandopenDeploymentUseApiDrawerare identical except for the button locator. Consider parameterizing to remove the duplicated drawer-resolution block.♻️ Proposed consolidation
-const openVariantUseApiDrawer = async (page: any) => { - await page.waitForLoadState("networkidle") - const useApiButton = page.locator('[data-tour="api-code-button"]') - await expect(useApiButton).toBeVisible({timeout: 15000}) - await expect(useApiButton).toBeEnabled({timeout: 5000}) - await useApiButton.click() - - const drawer = page.locator(".ant-drawer-content-wrapper").filter({ - hasText: "How to use API", - }) - await expect(drawer).toBeVisible({timeout: 20000}) - return drawer -} - -const openDeploymentUseApiDrawer = async (page: any) => { - await page.waitForLoadState("networkidle") - const useApiButton = page.getByRole("button", {name: "Use API"}).first() - await expect(useApiButton).toBeVisible({timeout: 15000}) - await expect(useApiButton).toBeEnabled({timeout: 5000}) - await useApiButton.click() - - const drawer = page.locator(".ant-drawer-content-wrapper").filter({ - hasText: "How to use API", - }) - await expect(drawer).toBeVisible({timeout: 20000}) - return drawer -} +const openUseApiDrawer = async (page: any, useApiButton: any) => { + await expect(useApiButton).toBeVisible({timeout: 15000}) + await expect(useApiButton).toBeEnabled({timeout: 5000}) + await useApiButton.click() + + const drawer = page.locator(".ant-drawer-content-wrapper").filter({ + hasText: "How to use API", + }) + await expect(drawer).toBeVisible({timeout: 20000}) + return drawer +} + +const openVariantUseApiDrawer = (page: any) => + openUseApiDrawer(page, page.locator('[data-tour="api-code-button"]')) + +const openDeploymentUseApiDrawer = (page: any) => + openUseApiDrawer(page, page.getByRole("button", {name: "Use API"}).first())web/oss/tests/playwright/acceptance/evaluators/tests.ts (1)
354-355: ⚡ Quick winUpdate slug assertion to account for real slug transformation (UI uses
slugify)The slug field in
CreateEvaluatoris set from the (debounced) evaluator name via aslugifyhelper (toLowerCase()andreplace(/[^a-z0-9_\-]+/g, "-"), plus trimming/collapsing dashes). So the assertion should not assume a no-op transformation in general.That said, the current tests pass
evaluatorNamevalues likee2e-human-eval-${Date.now()}(already lowercase and hyphen-safe), so it matches the slug output today. For robustness, assert againstslugify(evaluatorName)instead ofevaluatorName.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 685200ae-6c11-485f-8a7f-e0a67aa8fa47
📒 Files selected for processing (8)
web/ee/tests/playwright/acceptance/members/index.tsweb/ee/tests/playwright/acceptance/use-api/use-api.spec.tsweb/oss/tests/playwright/10-use-api.tsweb/oss/tests/playwright/acceptance/evaluators/tests.tsweb/oss/tests/playwright/acceptance/features/use-api.featureweb/oss/tests/playwright/acceptance/prompt-registry/index.tsweb/oss/tests/playwright/acceptance/use-api/index.tsweb/oss/tests/playwright/acceptance/use-api/use-api.spec.ts
| const openInviteMembersModal = async (page: any) => { | ||
| const inviteButton = page.getByRole("button", {name: "Invite Members"}).first() | ||
| await expect(inviteButton).toBeVisible({timeout: 20000}) | ||
| await expect(inviteButton).toBeEnabled() | ||
|
|
||
| const inviteModal = page.getByRole("dialog", {name: "Invite Members"}) | ||
| const emailInput = inviteModal.getByPlaceholder("member@organization.com") | ||
|
|
||
| for (let attempt = 0; attempt < 2; attempt++) { | ||
| await inviteButton.click() | ||
|
|
||
| if ( | ||
| await inviteModal | ||
| .waitFor({state: "visible", timeout: 5000}) | ||
| .then(() => true) | ||
| .catch(() => false) | ||
| ) { | ||
| await expect(emailInput).toBeVisible({timeout: 20000}) | ||
| return {inviteModal, emailInput} | ||
| } | ||
| } | ||
|
|
||
| await expect(emailInput).toBeVisible({timeout: 10000}) | ||
| return {inviteModal, emailInput} | ||
| } |
There was a problem hiding this comment.
Make the email input timeout consistent in both code paths.
The email input visibility check uses a 20-second timeout inside the retry loop (line 70) but only a 10-second timeout in the fallback path (line 75). This inconsistency could cause the fallback to fail prematurely if the modal opens on the second attempt but the email input takes 11-19 seconds to render.
🔧 Proposed fix to unify timeout
}
- await expect(emailInput).toBeVisible({timeout: 10000})
+ await expect(emailInput).toBeVisible({timeout: 20000})
return {inviteModal, emailInput}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const openInviteMembersModal = async (page: any) => { | |
| const inviteButton = page.getByRole("button", {name: "Invite Members"}).first() | |
| await expect(inviteButton).toBeVisible({timeout: 20000}) | |
| await expect(inviteButton).toBeEnabled() | |
| const inviteModal = page.getByRole("dialog", {name: "Invite Members"}) | |
| const emailInput = inviteModal.getByPlaceholder("member@organization.com") | |
| for (let attempt = 0; attempt < 2; attempt++) { | |
| await inviteButton.click() | |
| if ( | |
| await inviteModal | |
| .waitFor({state: "visible", timeout: 5000}) | |
| .then(() => true) | |
| .catch(() => false) | |
| ) { | |
| await expect(emailInput).toBeVisible({timeout: 20000}) | |
| return {inviteModal, emailInput} | |
| } | |
| } | |
| await expect(emailInput).toBeVisible({timeout: 10000}) | |
| return {inviteModal, emailInput} | |
| } | |
| const openInviteMembersModal = async (page: any) => { | |
| const inviteButton = page.getByRole("button", {name: "Invite Members"}).first() | |
| await expect(inviteButton).toBeVisible({timeout: 20000}) | |
| await expect(inviteButton).toBeEnabled() | |
| const inviteModal = page.getByRole("dialog", {name: "Invite Members"}) | |
| const emailInput = inviteModal.getByPlaceholder("member@organization.com") | |
| for (let attempt = 0; attempt < 2; attempt++) { | |
| await inviteButton.click() | |
| if ( | |
| await inviteModal | |
| .waitFor({state: "visible", timeout: 5000}) | |
| .then(() => true) | |
| .catch(() => false) | |
| ) { | |
| await expect(emailInput).toBeVisible({timeout: 20000}) | |
| return {inviteModal, emailInput} | |
| } | |
| } | |
| await expect(emailInput).toBeVisible({timeout: 20000}) | |
| return {inviteModal, emailInput} | |
| } |
| const revisionsRadio = page.getByRole("radio", {name: "Revisions"}) | ||
| const revisionsControl = page | ||
| .locator(".ant-radio-button-wrapper") | ||
| .filter({hasText: "Revisions"}) | ||
| .first() | ||
| await expect(revisionsControl).toBeVisible({timeout: 15000}) | ||
| if (!(await revisionsRadio.isChecked())) { | ||
| await revisionsControl.click() | ||
| } |
There was a problem hiding this comment.
Guard revisionsRadio against strict-mode violations.
revisionsControl is scoped with .first(), but revisionsRadio is not. getByRole's name option matches by substring (case-insensitive) by default, so if more than one element resolves, revisionsRadio.isChecked() will throw a strict-mode error. Add .first() for consistency with revisionsControl.
🛡️ Proposed fix
- const revisionsRadio = page.getByRole("radio", {name: "Revisions"})
+ const revisionsRadio = page.getByRole("radio", {name: "Revisions"}).first()| const openVariantUseApiDrawer = async (page: any) => { | ||
| await page.waitForLoadState("networkidle") | ||
| const useApiButton = page.locator('[data-tour="api-code-button"]') | ||
| await expect(useApiButton).toBeVisible({timeout: 15000}) | ||
| await expect(useApiButton).toBeEnabled({timeout: 5000}) | ||
| await useApiButton.click() | ||
|
|
||
| const drawer = page.locator(".ant-drawer-content-wrapper").filter({ | ||
| hasText: "How to use API", | ||
| }) | ||
| await expect(drawer).toBeVisible({timeout: 20000}) | ||
| return drawer | ||
| } | ||
|
|
||
| /** | ||
| * Opens the "How to use API" drawer from the Deployments tab. | ||
| * Uses the generic primary "Use API" button (no data-tour on the deployments one). | ||
| * Waits for networkidle first — DeploymentsDashboard is a dynamic() import whose | ||
| * onClick setter may not be wired yet if clicked too early. | ||
| */ | ||
| const openDeploymentUseApiDrawer = async (page: any) => { | ||
| await page.waitForLoadState("networkidle") | ||
| const useApiButton = page.getByRole("button", {name: "Use API"}).first() | ||
| await expect(useApiButton).toBeVisible({timeout: 15000}) | ||
| await expect(useApiButton).toBeEnabled({timeout: 5000}) | ||
| await useApiButton.click() | ||
|
|
||
| const drawer = page.locator(".ant-drawer-content-wrapper").filter({ | ||
| hasText: "How to use API", | ||
| }) | ||
| await expect(drawer).toBeVisible({timeout: 20000}) | ||
| return drawer | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is page.waitForLoadState("networkidle") discouraged in current Playwright documentation, and what is the recommended alternative?
💡 Result:
Yes. In the current Playwright documentation, waiting for the page to reach the networkidle load state is explicitly discouraged for testing/readiness checks. Playwright’s waitUntil documentation for page.goto()/load-state waiting states that waitUntil: "networkidle" / waitForLoadState("networkidle") is “DISCOURAGED” and that it “Don’t use this method for testing, rely on web assertions to assess readiness instead.” [1] Recommended alternative: use assertions (web-first assertions) or wait for specific user-visible/UI conditions (e.g., expect locators like toBeVisible()) rather than waiting for the network to become idle. Playwright’s best-practices guidance emphasizes using auto-waiting, web-first assertions for resilient readiness checks. [2][3]
Citations:
- 1: https://playwright.dev/docs/api/class-page
- 2: https://playwright.dev/docs/best-practices
- 3: https://github.com/microsoft/playwright/blob/main/docs/src/best-practices-js.md
Avoid waitForLoadState("networkidle") in Playwright drawer helpers (web/oss/tests/playwright/acceptance/use-api/index.ts, Lines 112 & 132). Playwright best-practices explicitly discourage using the networkidle load state for test/readiness synchronization because it can be unreliable (e.g., long-polling/streaming can prevent “idle”). Since the helpers already rely on auto-waiting expect(...).toBeVisible() / toBeEnabled() plus a concrete drawer visibility assertion, remove the explicit await page.waitForLoadState("networkidle") and synchronize on the existing UI signals instead.
Railway Preview Environment
|
…iness Without --max-time/--connect-timeout, curl could hang indefinitely when a Railway preview accepted the TCP connection but stalled before sending an HTTP response. This caused the wait-for-readiness job to block for hours instead of cycling through its 30-attempt loop. - Add --max-time 10 --connect-timeout 5 to each curl attempt so the loop is always bounded (~10 min max across both URL checks). - Add timeout-minutes: 25 to the job as a defence-in-depth backstop. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n reinstall playwright install --with-deps chromium was downloading 170MB and running apt-get for hundreds of X11/font packages on every run, taking ~17 minutes. This left almost no time for the auth bootstrap within the job timeout. Cache ~/.cache/ms-playwright keyed on the tests package.json hash so the browser binary is restored on cache hits (subsequent runs). playwright install still runs after the cache restore — it detects the binary is present and skips the download, but still verifies/installs any missing system deps via apt which is fast when packages are already cached by the runner. Also bumps the job timeout from 25 to 30 minutes to give the first (cold) run enough headroom. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
Testing
Verified locally
Added or updated tests
QA follow-up
Demo
Checklist
Contributor Resources