test: Add waitForLogout in logout action
#36947
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughCentralized logout/login wait behavior across e2e tests and page objects: removed redundant Login-button assertions in specs, added LoginPage.waitForIt() after logout and after E2EE password reset, introduced an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Spec as E2E Spec
participant Sidenav as HomeSidenav
participant Login as LoginPage
Tester->>Spec: run test
Spec->>Sidenav: logout()
Sidenav->>Sidenav: click accountLogoutOption
Sidenav->>Login: waitForIt()
Note right of Login #DDEEFF: Wait until login screen is ready
Login-->>Spec: ready
Spec-->>Tester: continue
sequenceDiagram
autonumber
actor Tester
participant Spec as E2E Spec
participant E2EE as ResetE2EEPasswordModal
participant Login as LoginPage
Spec->>E2EE: confirmReset()
E2EE->>E2EE: close reset dialog
E2EE->>Login: waitForIt()
Note right of Login #DDEEFF: Ensure post-reset login screen is ready
Login-->>Spec: ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/meteor/tests/e2e/page-objects/login.ts (1)
26-28: Harden logout wait to reduce flakinessAlso assert the app content is gone (or the URL changed) to avoid false positives where the Login button briefly flashes.
async waitForLogout() { - await expect(this.loginButton).toBeVisible(); + await expect(this.loginButton).toBeVisible(); + // Ensure main app content is not visible anymore + await expect(this.page.getByRole('main')).not.toBeVisible(); + // Optionally, assert we are on a public route + // await expect(this.page).toHaveURL(/\/login|\/home/); }apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts (2)
107-109: Confirm ARIA role for Logout item or use a more permissive roleIf Logout is not exposed as a checkbox item, this locator will fail. Consider falling back to plain menuitem.
-get accountLogoutOption(): Locator { - return this.userProfileMenu.getByRole('menuitemcheckbox', { name: 'Logout' }); -} +get accountLogoutOption(): Locator { + // Works whether it's menuitem or menuitemcheckbox + return this.userProfileMenu.getByRole('menuitem', { name: 'Logout' }).or( + this.userProfileMenu.getByRole('menuitemcheckbox', { name: 'Logout' }), + ); +}
173-177: Wait for the menu to render before clicking LogoutClicking the option immediately after toggling the menu can be flaky. Wait for the menu visibility first.
async logout(): Promise<void> { await this.btnUserProfileMenu.click(); - await this.accountLogoutOption.click(); + await expect(this.userProfileMenu).toBeVisible(); + await this.accountLogoutOption.click(); await this.login.waitForLogout(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/meteor/tests/e2e/e2e-encryption.spec.ts(2 hunks)apps/meteor/tests/e2e/page-objects/fragments/e2ee.ts(3 hunks)apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts(3 hunks)apps/meteor/tests/e2e/page-objects/login.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/tests/e2e/page-objects/fragments/e2ee.ts (1)
apps/meteor/tests/e2e/page-objects/login.ts (1)
LoginPage(9-55)
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts (1)
apps/meteor/tests/e2e/page-objects/login.ts (1)
LoginPage(9-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
apps/meteor/tests/e2e/page-objects/fragments/e2ee.ts (3)
6-6: Import looks goodBrings LoginPage into the fragment where it’s needed.
100-105: Good ownership of LoginPage within modalInstantiating and encapsulating the login page object here makes the reset flow self‑contained.
114-115: Deterministic post‑reset flowWaiting for logout after confirming reset avoids races in subsequent steps.
apps/meteor/tests/e2e/e2e-encryption.spec.ts (3)
101-106: Comment clarifies flow; no behavior changeLGTM.
107-114: Comment clarifies flow; no behavior changeLGTM.
202-207: Using the centralized logout flow is the right callAligns tests on a single wait strategy via page objects.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36947 +/- ##
===========================================
- Coverage 66.60% 66.56% -0.04%
===========================================
Files 3346 3346
Lines 114661 114661
Branches 21099 21098 -1
===========================================
- Hits 76365 76327 -38
- Misses 35601 35644 +43
+ Partials 2695 2690 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts (1)
3-3: Resolved: switched to value import so LoginPage exists at runtimeThis addresses the prior crash risk when constructing
LoginPage. Good fix.
🧹 Nitpick comments (3)
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts (3)
10-15: InitializeLoginPageeagerly; consider DI for reuse/testabilityCurrent init works. Optionally accept a
LoginPagevia constructor to ease reuse/mocking in specs.- constructor(page: Page) { - this.page = page; - this.login = new LoginPage(page); - } + constructor(page: Page, login = new LoginPage(page)) { + this.page = page; + this.login = login; + }
108-111: Tighten locator to reduce flakinessAdd
exact: trueto avoid matching similarly named items.- get accountLogoutOption(): Locator { - return this.userProfileMenu.getByRole('menuitemcheckbox', { name: 'Logout' }); - } + get accountLogoutOption(): Locator { + return this.userProfileMenu.getByRole('menuitemcheckbox', { name: 'Logout', exact: true }); + }Please confirm that the ARIA role for “Logout” is indeed
menuitemcheckboxin current UI; if it’smenuitem, update accordingly.
176-178: Stabilize the logout flow; verify intent vs PR title
- Add an explicit wait for the menu visibility before clicking the item to avoid race conditions (ties to FLAKY-1542).
- Title mentions “waitForLogout” but code calls
waitForIt()(deprecated). If awaitForLogout()exists elsewhere in this PR, prefer it here.async logout(): Promise<void> { await this.btnUserProfileMenu.click(); + await expect(this.userProfileMenu).toBeVisible(); await this.accountLogoutOption.click(); - await this.login.waitForIt(); + // Prefer waitForLogout() if available; fallback is waitForIt() + await this.login.waitForIt(); }If
LoginPage.waitForLogout()was added, replace the last line withawait this.login.waitForLogout();.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/tests/e2e/page-objects/fragments/e2ee.ts(3 hunks)apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/tests/e2e/page-objects/fragments/e2ee.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts (1)
apps/meteor/tests/e2e/page-objects/login.ts (1)
LoginPage(9-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
tassoevan
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
FLAKY-1542
Summary by CodeRabbit