Skip to content

Comments

fix: few more e2e flakes#23966

Merged
anikdhabal merged 1 commit intomainfrom
flx-flaki-e2e
Sep 20, 2025
Merged

fix: few more e2e flakes#23966
anikdhabal merged 1 commit intomainfrom
flx-flaki-e2e

Conversation

@anikdhabal
Copy link
Contributor

@anikdhabal anikdhabal commented Sep 20, 2025

  • In apps/web/playwright/fixtures/users.ts, the logout flow now locates the logout button by test ID, asserts its text is "Go back to the login page", reloads the page, and re-asserts the text.
  • In apps/web/playwright/insights.e2e.ts, chart title selection was changed from substring matching to exact matching using a RegExp with ^ and $ anchors.

@graphite-app graphite-app bot requested a review from a team September 20, 2025 11:06
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 20, 2025

Walkthrough

  • In apps/web/playwright/fixtures/users.ts, the logout flow now locates the logout button by test ID, asserts its text is "Go back to the login page", reloads the page, and re-asserts the text.
  • In apps/web/playwright/insights.e2e.ts, chart title selection was changed from substring matching to exact matching using a RegExp with ^ and $ anchors.
  • No changes to exported/public API signatures.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description Check ❓ Inconclusive No author-provided PR description is present, so there is insufficient information to determine whether the changeset is adequately described; the raw_summary indicates e2e test fixes but the absence of an explicit description makes this check inconclusive. Please add a short PR description that states the intent (e.g., reducing flaky e2e tests), lists the key files changed (apps/web/playwright/fixtures/users.ts and apps/web/playwright/insights.e2e.ts), and includes any verification steps or notes about what flakes were addressed.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix: few more e2e flakes" clearly communicates the PR's intent to reduce end-to-end test flakiness and maps to the changes in the fixtures and insights e2e files (logout UI assertion and exact chart title matching), so it is on-topic and concise; however it is somewhat informal and could be more specific.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch flx-flaki-e2e

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.

@keithwillcode keithwillcode added the core area: core, team members only label Sep 20, 2025
@dosubot dosubot bot added the automated-tests area: unit tests, e2e tests, playwright label Sep 20, 2025
Comment on lines +687 to +690
const logoutBtn = page.getByTestId("logout-btn");
await expect(logoutBtn).toHaveText("Go back to the login page");
await page.reload();
await expect(logoutBtn).toHaveText("Go back to the login page");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one ensures that the session is correctly removed after logout. Previously it was causing flakes, as session were there

@anikdhabal anikdhabal marked this pull request as draft September 20, 2025 11:07
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/insights.e2e.ts (1)

274-276: Escape dynamic regex input (future‑proof exact match).

new RegExp(\^${title}$`)` will misbehave if a future title contains regex metacharacters. It’s not a security risk here (constant array), but it can cause brittle tests and triggers linters.

Apply this diff and add a small helper:

-      const chartCard = page
-        .locator("[data-testid='panel-card'] h2")
-        .filter({ hasText: new RegExp(`^${title}$`) });
+      const chartCard = page
+        .locator("[data-testid='panel-card'] h2")
+        .filter({ hasText: new RegExp(`^${escapeRegex(title)}$`) });

Add once in this file (top-level is fine):

function escapeRegex(s: string) {
  return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
}
apps/web/playwright/fixtures/users.ts (1)

687-691: LGTM. Minor hardening + DRY opportunity.

Solid flake guard. Optionally: assert URL after navigation and reuse the same assertion in both logout helpers to avoid drift.

-    logout: async () => {
-      await page.goto("/auth/logout");
-      const logoutBtn = page.getByTestId("logout-btn");
-      await expect(logoutBtn).toHaveText("Go back to the login page");
-      await page.reload();
-      await expect(logoutBtn).toHaveText("Go back to the login page");
-    },
+    logout: async () => {
+      await page.goto("/auth/logout");
+      await expect(page).toHaveURL(/\/auth\/logout$/);
+      await assertLogoutScreen(page);
+      await page.reload();
+      await assertLogoutScreen(page);
+    },

Add once in this file and reuse from the other logout (lines 585–591) too:

async function assertLogoutScreen(page: Page) {
  const logoutBtn = page.getByTestId("logout-btn");
  await expect(logoutBtn).toHaveText("Go back to the login 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 d5d44fb and 980a5eb.

📒 Files selected for processing (2)
  • apps/web/playwright/fixtures/users.ts (1 hunks)
  • apps/web/playwright/insights.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/fixtures/users.ts
  • apps/web/playwright/insights.e2e.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
  • apps/web/playwright/insights.e2e.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
  • apps/web/playwright/insights.e2e.ts
🪛 ast-grep (0.39.5)
apps/web/playwright/insights.e2e.ts

[warning] 275-275: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${title}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html

(regexp-from-variable)

⏰ 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). (2)
  • GitHub Check: Install dependencies / Yarn install & cache
  • GitHub Check: Detect changes

@github-actions
Copy link
Contributor

E2E results are ready!

@anikdhabal anikdhabal marked this pull request as ready for review September 20, 2025 11:44
@anikdhabal anikdhabal enabled auto-merge (squash) September 20, 2025 11:44
Copy link
Contributor

@Devanshusharma2005 Devanshusharma2005 left a comment

Choose a reason for hiding this comment

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

LGTM!

@anikdhabal anikdhabal merged commit cf89046 into main Sep 20, 2025
105 of 113 checks passed
@anikdhabal anikdhabal deleted the flx-flaki-e2e branch September 20, 2025 11:47
saurabhraghuvanshii pushed a commit to saurabhraghuvanshii/cal.com that referenced this pull request Sep 24, 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 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