test: Flaky admin search users #37501
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughReplaces direct test interactions with the users search input by introducing and using a new AdminUsers.page-object method Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant PO as AdminUsers (page-object)
participant UI as Browser / DOM
Note over Test,PO: New flow uses page-object abstraction
Test->>PO: searchUser(username)
activate PO
PO->>UI: inputSearchUsers.fill(username)
UI-->>PO: input updated
PO->>UI: expect(getUserRowByUsername(username).first()).toHaveCount(1)
UI-->>PO: row visible
PO-->>Test: resolved
deactivate PO
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-23T19:22:59.217ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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 |
dougfabris
force-pushed
the
test/flaky-search-users
branch
from
1dce961 to
9c9b09a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/meteor/tests/e2e/admin-users-custom-fields.spec.ts(3 hunks)apps/meteor/tests/e2e/admin-users-role-management.spec.ts(2 hunks)apps/meteor/tests/e2e/admin-users-status-management.spec.ts(1 hunks)apps/meteor/tests/e2e/admin-users.spec.ts(1 hunks)apps/meteor/tests/e2e/administration.spec.ts(2 hunks)apps/meteor/tests/e2e/page-objects/admin-users.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/tests/e2e/admin-users-role-management.spec.ts
⏰ 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 (7)
apps/meteor/tests/e2e/page-objects/admin-users.ts (3)
31-33: Good encapsulation by making the search input private.Making
inputSearchUsersprivate and exposingsearchUser()as the public API is a solid design choice that centralizes search logic and prevents direct input manipulation in tests.
63-66: Excellent solution for addressing flaky user search tests.The
searchUser()method addresses flakiness by:
- Filling the search input
- Explicitly waiting for exactly 1 matching result via
toHaveCount(1)This prevents race conditions where tests might proceed before search results load.
Note:
toHaveCount(1)checks that exactly one element exists in the DOM, but doesn't verify visibility (CSS display/visibility properties). If visibility is a requirement for your tests, consider adding an additionaltoBeVisible()check:async searchUser(username: string): Promise<void> { await this.inputSearchUsers.fill(username); const row = this.getUserRowByUsername(username); await expect(row).toHaveCount(1); await expect(row).toBeVisible(); // Optional: if visibility matters }
39-41: Adding.first()is safe—all callers usesearchUser()first.Verification confirms that every usage of
getUserRowByUsername()across all test files is preceded bysearchUser(), which assertstoHaveCount(1). The concern about silent ambiguous selection does not apply to the current codebase. The.first()provides defensive programming but is not strictly necessary given the existing usage patterns.apps/meteor/tests/e2e/administration.spec.ts (2)
68-70: Clean refactor to use the centralized search method.The test now properly uses
searchUser()to find the user, improving reliability and consistency with other tests.
104-104: Proper usage pattern: search before interacting with user row.apps/meteor/tests/e2e/admin-users-custom-fields.spec.ts (1)
64-64: Consistent adoption of the centralized search method across all test cases.All three test cases properly use
searchUser()before interacting with user rows, following the established pattern and improving test reliability.Also applies to: 94-94, 143-143
apps/meteor/tests/e2e/admin-users-role-management.spec.ts (1)
33-33: Proper usage of the centralized search abstraction.Both tests correctly use
searchUser()to locate users before performing role management actions, improving reliability and consistency.Also applies to: 51-51
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37501 +/- ##
===========================================
- Coverage 68.98% 68.96% -0.02%
===========================================
Files 3358 3358
Lines 114240 114240
Branches 20537 20537
===========================================
- Hits 78803 78790 -13
- Misses 33345 33362 +17
+ Partials 2092 2088 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit