fix(e2e): replace false-passing assertions and hard-coded waits in test suite#28486
fix(e2e): replace false-passing assertions and hard-coded waits in test suite#28486dididy wants to merge 3 commits intocalcom:mainfrom
Conversation
There was a problem hiding this comment.
5 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/playwright/fixtures/workflows.ts">
<violation number="1" location="apps/web/playwright/fixtures/workflows.ts:130">
P2: Disabled-state checks use non-retrying snapshot assertions instead of Playwright’s auto-retrying locator assertions, which can cause flaky E2E failures.</violation>
</file>
<file name="apps/web/playwright/out-of-office.e2e.ts">
<violation number="1" location="apps/web/playwright/out-of-office.e2e.ts:203">
P2: `getByTestId("away-emoji")` can resolve to multiple elements (one per away day), and Playwright strict mode throws when assertions target a non-unique locator. This can make the test flaky/fail even when the UI is correct.</violation>
<violation number="2" location="apps/web/playwright/out-of-office.e2e.ts:370">
P1: Custom agent: **E2E Tests Best Practices**
Rule 1 violation: these updated assertions still rely on `page.locator('text=...')` instead of resilient `getByTestId`-based selectors.</violation>
</file>
<file name="apps/web/playwright/payment-apps.e2e.ts">
<violation number="1" location="apps/web/playwright/payment-apps.e2e.ts:127">
P1: Custom agent: **E2E Tests Best Practices**
Rule 1 forbids `page.locator('text=...')`, but this change adds that brittle selector pattern across several payment-app assertions. Replace these page-level text locators with stable test ids (or another scoped stable locator) instead.</violation>
<violation number="2" location="apps/web/playwright/payment-apps.e2e.ts:210">
P1: Custom agent: **E2E Tests Best Practices**
Use `expect(page).toHaveURL()` here instead of `waitForURL()`. Rule 1 requires a URL assertion so this test fails if the setup page immediately redirects somewhere unexpected.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -123,15 +123,15 @@ test.describe("Payment app", () => { | |||
|
|
|||
There was a problem hiding this comment.
P1: Custom agent: E2E Tests Best Practices
Rule 1 forbids page.locator('text=...'), but this change adds that brittle selector pattern across several payment-app assertions. Replace these page-level text locators with stable test ids (or another scoped stable locator) instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/playwright/payment-apps.e2e.ts, line 127:
<comment>Rule 1 forbids `page.locator('text=...')`, but this change adds that brittle selector pattern across several payment-app assertions. Replace these page-level text locators with stable test ids (or another scoped stable locator) instead.</comment>
<file context>
@@ -123,15 +123,15 @@ test.describe("Payment app", () => {
- // expect 200 sats to be displayed in page
- expect(await page.locator("text=350").first()).toBeTruthy();
+ // expect 350 USD to be displayed in page
+ await expect(page.locator("text=350").first()).toBeVisible();
await selectFirstAvailableTimeSlotNextMonth(page);
</file context>
There was a problem hiding this comment.
Two reasons these can't be replaced with data-testid selectors. AlbyPriceComponent, AppCard, and the paypal setup page have no data-testid attributes - that would need a separate PR on the production side. Also, some of these are checking the actual text on purpose: 200 sats and MX$150.00 exist to verify the price format is correct. A testid would only tell you the element is there, not what it says.
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/playwright/payment-apps.e2e.ts">
<violation number="1">
P2: Custom agent: **E2E Tests Best Practices**
Avoid page-level text locators in E2E tests per E2E Tests Best Practices; use a stable selector (e.g., data-testid) or verify navigation with expect(page).toHaveURL instead.</violation>
<violation number="2">
P2: The new assertion only checks that a Locator object exists, not that the Alby setup page content appears. Because locators are always truthy, this test can pass even if navigation fails or the text never appears.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
romitg2
left a comment
There was a problem hiding this comment.
makes sense to me, let's run ci
- Replace toBeTruthy() on Locator objects with toBeVisible() (always-passing)
- Replace toBeTruthy() on boolean expressions with toBe(true) for clarity
- Replace hardcoded waitForTimeout() with element-based waits (waitFor/toBeVisible)
- Fix strict mode violation: getByTestId("away-emoji").first()
- Fix error assertion: infinite redirect error shown as toast → getByTestId("toast-error")
- Fix isDisabled() no-op → await expect().toBeDisabled() in workflows fixture
- Fix alby setup: add waitForURL(), remove unverifiable "Connect with Alby" assertion
- Fix stripe/paypal: replace no-op isDisabled() with toBeChecked() on paypal switch
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- workflows.ts: replace non-retrying isDisabled() snapshot with toBeDisabled() - out-of-office.e2e.ts: add .first() for away-emoji strict mode, use toast-error testid, remove unused const t - payment-apps.e2e.ts: replace always-passing toBeTruthy() with toHaveURL() assertion
0df41f3 to
6d32905
Compare
|
Sorry about the noise — CI failed on the previous run, so I rebased and force-pushed. Could you re-trigger CI? The failures were in the API v2 roles/permissions tests (run link) — |
What does this PR do?
Fixes silent test failures caused by always-passing assertions and arbitrary
waitForTimeout()calls in the Playwright E2E test suite.This is a scoped subset of the changes from #28466, split to stay within the 10-file / 500-line PR guidelines. Files with outstanding review feedback have been excluded and will be addressed in follow-up PRs.
Problem 1:
toBeTruthy()on Locator objects (always passes)page.locator()andpage.getByTestId()return aLocatorobject, which is always truthy regardless of whether the element exists in the DOM.Before:
After:
Affected files:
fixtures/apps.ts,payment-apps.e2e.tsProblem 2:
isDisabled()result not asserted (no-op)isDisabled()returns aPromise<boolean>. Without asserting the result, the call is a complete no-op.Before:
After:
Affected files:
fixtures/workflows.tsProblem 3:
toBeTruthy()on booleans and nullable valuesBefore:
After:
Affected files:
booking-phone-autofill.e2e.ts,login.api.e2e.ts,event-types.e2e.ts,signup.e2e.tsProblem 4:
waitForTimeout()— brittle arbitrary waitseventType/limit-tab.e2e.tswaitForTimeout(10000)getByTestId("time").first().waitFor({ state: "visible" })fixtures/apps.tswaitForTimeout(1000)×2signup.e2e.tswaitForTimeout(500),waitForTimeout(1000)Problem 5: Strict mode violation and toast assertion in
out-of-office.e2e.tsgetByTestId("away-emoji")matched 6 elements → added.first()getByTestId("toast-error")Problem 6: Alby setup page navigation in
payment-apps.e2e.tsSetup page returns 500 without API keys configured, making "Connect with Alby" text unverifiable. Navigation to the setup page via
waitForURL()is sufficient verification.Also replaced no-op
isDisabled()on stripe switch (pre-existing UI bug where switch never becomes disabled) withtoBeChecked()on the paypal switch to verify the actual enabled state.Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist