-
Notifications
You must be signed in to change notification settings - Fork 1.4k
test: fix unread badge test #6752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReplaces a text-regex wait with an element-based wait in an existing Maestro test, adds a new Maestro test for unread/mention badge scenarios, and assigns testID attributes to the UnreadBadge component via a new helper. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/containers/UnreadBadge/index.tsx (1)
111-111: Parameter alignment with function signature.This line passes the formatted
textstring togetTestId, which expects a numericunreadparameter per its signature. This mismatch is the root cause of the issues identified in thegetTestIdfunction review.
📜 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 ignored due to path filters (3)
app/containers/Header/components/HeaderButton/__snapshots__/HeaderButtons.test.tsx.snapis excluded by!**/*.snapapp/containers/RoomItem/__snapshots__/RoomItem.test.tsx.snapis excluded by!**/*.snapapp/containers/UnreadBadge/__snapshots__/UnreadBadge.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
.maestro/tests/room/mark-as-unread.yaml(1 hunks).maestro/tests/room/unread-badge.yaml(1 hunks)app/containers/UnreadBadge/index.tsx(2 hunks)
⏰ 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). (1)
- GitHub Check: format
🔇 Additional comments (3)
app/containers/UnreadBadge/index.tsx (1)
119-120: LGTM: testID application is correct.The testID is properly applied to the View component using standard React Native conventions.
.maestro/tests/room/mark-as-unread.yaml (1)
54-59: LGTM: Element-based wait improves test reliability.Switching from text-based regex matching to element ID targeting significantly reduces flakiness. The
childOfconstraint properly scopes the wait to the specific room list item, making the assertion more precise..maestro/tests/room/unread-badge.yaml (1)
18-110: Test logic is sound, but color verification is missing.The test comprehensively covers the four scenarios described in the PR objectives and properly verifies badge counts and types (unread vs. mention). The test structure is well-organized with clear comments for each scenario.
However, the PR description states the test verifies "badge color and count behavior," but there are no color assertions in the test. Maestro's
extendedWaitUntilwithvisibleonly confirms element presence, not visual properties like background color.Consider whether color verification is necessary. If it is, you may need to:
- Use Maestro's assertion capabilities to check styling/color properties (if supported), or
- Update the PR description to accurately reflect that only count and badge type are verified, not colors.
| appId: chat.rocket.reactnative | ||
| name: Unread badge | ||
| onFlowStart: | ||
| - runFlow: '../../helpers/setup.yaml' | ||
| tags: | ||
| - test-13 | ||
|
|
||
| --- | ||
| - evalScript: ${output.user = output.utils.createUser()} | ||
| - evalScript: ${output.otherUser = output.utils.createUser()} | ||
|
|
||
| - runFlow: | ||
| file: '../../helpers/login-with-deeplink.yaml' | ||
| env: | ||
| USERNAME: ${output.user.username} | ||
| PASSWORD: ${output.user.password} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add cleanup step for created users.
The test creates users but doesn't clean them up. The mark-as-unread.yaml test includes an onFlowComplete hook to delete created users, which should be added here as well.
Add this after line 4:
onFlowStart:
- runFlow: '../../helpers/setup.yaml'
+onFlowComplete:
+ - evalScript: ${output.utils.deleteCreatedUsers()}
tags:
- test-13🤖 Prompt for AI Agents
In .maestro/tests/room/unread-badge.yaml around lines 1 to 16, add an
onFlowComplete hook immediately after line 4 that runs the same cleanup flow
used in mark-as-unread.yaml to delete the created users; specifically invoke the
helper cleanup flow and pass the created users (output.user and
output.otherUser) via env (usernames/passwords or identifiers) so both users are
removed when the test finishes.
| function getTestId(userMentions: number | undefined, groupMentions: number | undefined, unread: number | undefined) { | ||
| if (userMentions) { | ||
| return `mention-badge-${unread}`; | ||
| } | ||
| if (groupMentions) { | ||
| return `group-mention-badge-${unread}`; | ||
| } | ||
| if (unread) { | ||
| return `unread-badge-${unread}`; | ||
| } | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify parameter naming and handle edge cases.
The third parameter is named unread but receives the text variable (line 111), which is a formatted string that can be "+99", "+999", or a number string. This creates several issues:
- Misleading parameter name: The parameter should be named
textorcountto match what it actually receives. - Unexpected testID formats: When large counts are formatted, testIDs become
unread-badge-+99ormention-badge-+999, which may complicate test maintenance. - Missing validation: If
textis undefined or empty, testIDs likemention-badge-undefinedwill be generated.
Consider this refactor for clarity and robustness:
-function getTestId(userMentions: number | undefined, groupMentions: number | undefined, unread: number | undefined) {
+function getTestId(userMentions: number | undefined, groupMentions: number | undefined, count: string) {
+ if (!count) {
+ return '';
+ }
if (userMentions) {
- return `mention-badge-${unread}`;
+ return `mention-badge-${count}`;
}
if (groupMentions) {
- return `group-mention-badge-${unread}`;
+ return `group-mention-badge-${count}`;
}
- if (unread) {
- return `unread-badge-${unread}`;
- }
- return '';
+ return `unread-badge-${count}`;
}🤖 Prompt for AI Agents
In app/containers/UnreadBadge/index.tsx around lines 42 to 53, the third
parameter is misleadingly named `unread` while callers pass the formatted `text`
(e.g. "+99"), which can produce invalid or brittle testIDs and even "undefined"
values; rename the parameter to `text` (or `displayText`), validate/normalize it
before building IDs (fallback to an empty string or "0" when undefined), strip
any leading "+" or other formatting characters used for display, and prefer
using the raw numeric count (if available) or a consistent token like "overflow"
for large counts so produced testIDs are deterministic and safe (e.g.
`mention-badge-99` or `unread-badge-overflow`) instead of `+99` or `undefined`.
b42a1d9 to
8e3a778
Compare
Proposed changes
Right now, we use text matching to check the unread count, which sometimes makes the iOS tests flaky. In this PR, I added a test ID for the badge to make it more reliable.
Along with that, I also added a test to verify that we display the normal badge for regular messages and the red badge for mentions.
Added Testcase
Working Action Link: https://github.com/RocketChat/Rocket.Chat.ReactNative/actions/runs/18985162911
Issue(s)
https://rocketchat.atlassian.net/browse/COMM-74
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.