Skip to content

fix: e2e flakes visible in ci#23897

Merged
anikdhabal merged 5 commits intomainfrom
fix-current-e2e-flake
Sep 17, 2025
Merged

fix: e2e flakes visible in ci#23897
anikdhabal merged 5 commits intomainfrom
fix-current-e2e-flake

Conversation

@anikdhabal
Copy link
Contributor

@anikdhabal anikdhabal commented Sep 17, 2025

No description provided.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 17, 2025

Walkthrough

  • apps/web/playwright/fixtures/users.ts: the logout flow now verifies UI after navigating to /auth/logout by selecting the element with test id logout-btn, asserting its text is Go back to the login page, reloading the page, and asserting the same text again; function signature unchanged.
  • apps/web/playwright/fixtures/apps.ts: goToEventType now waits for the event title element and asserts the vertical tab basics has aria-current="page" after navigation.
  • apps/web/playwright/workflow.e2e.ts: the createWorkflow call in the "Creating a new workflow" test now passes "test workflow" instead of an empty string; assertions unchanged.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request has no author-provided description; because it is empty it does not meet the expectation that the description relate to the changeset and therefore fails this check. Add a brief description summarizing what was changed and why—mention the modified Playwright files (apps/web/playwright/fixtures/users.ts, apps/web/playwright/fixtures/apps.ts, apps/web/playwright/workflow.e2e.ts), the CI flake observed, and how the added waits/assertions address the race conditions.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix: one e2e flake visible in ci" accurately and concisely describes the primary intent of the changeset—fixing a flaky end-to-end test observed in CI—and aligns with the Playwright test and fixture edits in the diff. It is clear and directly related to the main change.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-current-e2e-flake

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@graphite-app graphite-app bot requested a review from a team September 17, 2025 13:28
@keithwillcode keithwillcode added the core area: core, team members only label Sep 17, 2025
@vercel
Copy link

vercel bot commented Sep 17, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
cal Ignored Ignored Sep 17, 2025 3:29pm
cal-eu Ignored Ignored Sep 17, 2025 3:29pm

@dosubot dosubot bot added automated-tests area: unit tests, e2e tests, playwright 🐛 bug Something isn't working labels Sep 17, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
apps/web/playwright/fixtures/users.ts (1)

685-687: Keep both logout helpers consistent (DRY).

createUserFixture.logout still uses the old flow. Reuse the same verified helper to avoid reintroducing flakes elsewhere.

Apply this diff and add a small helper:

@@
     logout: async () => {
-      await page.goto("/auth/logout");
+      await visitAndVerifyLogout(page);
     },

Add once near the other helpers:

async function visitAndVerifyLogout(page: Page) {
  await page.goto("/auth/logout");
  await page.waitForURL(/\/auth\/logout(?:\?.*)?$/);
  const logoutBtn = page.getByTestId("logout-btn");
  await expect(logoutBtn).toBeVisible();
  await expect(logoutBtn).toHaveText(/go back to (the )?login page/i);
}
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 39917eb and 33ce9d1.

📒 Files selected for processing (1)
  • apps/web/playwright/fixtures/users.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • apps/web/playwright/fixtures/users.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js .utc() in hot paths like loops

Files:

  • apps/web/playwright/fixtures/users.ts
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • apps/web/playwright/fixtures/users.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Type check / check-types
  • GitHub Check: Linters / lint
  • GitHub Check: Tests / Unit
🔇 Additional comments (1)
apps/web/playwright/fixtures/users.ts (1)

587-590: ```shell
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- ls fixture dir ----"
ls -la apps/web/playwright/fixtures || true
echo "---- search for logout CTA text / testid ----"
rg -n -C2 "logout-btn|Go back to the login page" apps/web -g '/*' || true
echo "---- search for getByTestId / data-testid usage ----"
rg -n -C2 "getByTestId\(|data-testid=" apps/web -g '
/*' || true
echo "---- show users.ts around the reported lines if present ----"
if [ -f "apps/web/playwright/fixtures/users.ts" ]; then
sed -n '560,620p' apps/web/playwright/fixtures/users.ts || true
else
echo "MISSING: apps/web/playwright/fixtures/users.ts"
fi


</blockquote></details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

kart1ka
kart1ka previously approved these changes Sep 17, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Sep 17, 2025

E2E results are ready!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
apps/web/playwright/workflow.e2e.ts (1)

18-20: Make workflow name unique to prevent parallel-run collisions.

Hardcoded "test workflow" can clash across retries/shards and re-select the wrong row. Generate a unique name.

-        await createWorkflow({ name: "test workflow" });
+        const wfName = `test workflow ${test.info().parallelIndex}-${Date.now()}`;
+        await createWorkflow({ name: wfName });
apps/web/playwright/fixtures/apps.ts (1)

114-116: Prefer locator assertions over raw waitForSelector for stability.

Use Playwright’s auto-waiting visibility assertion; it’s more robust than DOM‑attachment checks.

-      // fix the race condition
-      await page.waitForSelector('[data-testid="event-title"]');
+      // fix the race condition
+      await expect(page.getByTestId("event-title")).toBeVisible();
       await expect(page.getByTestId("vertical-tab-basics")).toHaveAttribute("aria-current", "page");
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 33ce9d1 and a1b96af.

📒 Files selected for processing (2)
  • apps/web/playwright/fixtures/apps.ts (1 hunks)
  • apps/web/playwright/workflow.e2e.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

**/*.ts: For Prisma queries, only select data you need; never use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • apps/web/playwright/workflow.e2e.ts
  • apps/web/playwright/fixtures/apps.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js .utc() in hot paths like loops

Files:

  • apps/web/playwright/workflow.e2e.ts
  • apps/web/playwright/fixtures/apps.ts
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • apps/web/playwright/workflow.e2e.ts
  • apps/web/playwright/fixtures/apps.ts
🧠 Learnings (1)
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.

Applied to files:

  • apps/web/playwright/workflow.e2e.ts
  • apps/web/playwright/fixtures/apps.ts
🧬 Code graph analysis (1)
apps/web/playwright/workflow.e2e.ts (1)
apps/api/v2/src/modules/organizations/teams/workflows/controllers/org-team-workflows.controller.ts (1)
  • createWorkflow (85-92)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: Tests / Unit
  • GitHub Check: Production builds / Build Docs
  • GitHub Check: Production builds / Build API v1
  • GitHub Check: Production builds / Build Web App
  • GitHub Check: Production builds / Build API v2
  • GitHub Check: Production builds / Build Atoms
  • GitHub Check: Type check / check-types
  • GitHub Check: Linters / lint

@anikdhabal anikdhabal merged commit 3b471f0 into main Sep 17, 2025
38 checks passed
@anikdhabal anikdhabal deleted the fix-current-e2e-flake branch September 17, 2025 15:55
@anikdhabal anikdhabal changed the title fix: one e2e flake visible in ci fix: e2e flakes visible in ci Sep 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated-tests area: unit tests, e2e tests, playwright 🐛 bug Something isn't working core area: core, team members only ready-for-e2e size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants