-
Notifications
You must be signed in to change notification settings - Fork 1.4k
chore: new test workspace #6841
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
WalkthroughE2E test infra and test data updated: Maestro rerun/dedup logic changed, seed/test URLs switched to QA, new password generator added, multiple Maestro test flows made platform-specific or more deterministic, and small app E2E-related adjustments to notification timing and password masking. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (1)
.maestro/README.md (1)
42-42: Documentation updated correctly, with a minor style suggestion.The documentation now accurately reflects the QA server being used for testing.
Optional: Address markdown linting warning
The markdown linter flags this as a bare URL. Consider wrapping it in angle brackets or code formatting for consistency:
-- Currently we point to https://mobile.qa.rocket.chat as main server +- Currently we point to <https://mobile.qa.rocket.chat> as main serveror
-- Currently we point to https://mobile.qa.rocket.chat as main server +- Currently we point to `https://mobile.qa.rocket.chat` as main server
📜 Review details
Configuration used: Organization 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 (20)
.github/scripts/run-maestro.sh(2 hunks).github/workflows/maestro-android.yml(2 hunks).github/workflows/maestro-ios.yml(2 hunks).maestro/README.md(1 hunks).maestro/scripts/data.js(2 hunks).maestro/scripts/random.js(1 hunks).maestro/tests/assorted/accessibility-and-appearance.yaml(4 hunks).maestro/tests/assorted/changeserver.yaml(1 hunks).maestro/tests/assorted/delete-server.yaml(1 hunks).maestro/tests/assorted/user-preferences.yaml(1 hunks).maestro/tests/onboarding/server-history.yaml(2 hunks).maestro/tests/room/room.yaml(3 hunks).maestro/tests/room/unread-badge.yaml(9 hunks).maestro/tests/teams/utils/close-action-sheet.yaml(1 hunks)app/containers/InAppNotification/index.tsx(1 hunks)app/lib/methods/getQuoteAttachment.test.ts(4 hunks)app/lib/methods/helpers/getAvatarUrl.test.ts(1 hunks)app/reducers/server.test.ts(1 hunks)app/views/NewServerView/utils/completeUrl.test.ts(2 hunks)app/views/RegisterView/index.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T12:13:16.929Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6598
File: .maestro/tests/room/room.yaml:416-419
Timestamp: 2025-09-16T12:13:16.929Z
Learning: Maestro swipe command accepts uppercase direction values: UP, DOWN, LEFT, RIGHT (not lowercase as incorrectly suggested). The official documentation shows examples using uppercase direction values.
Applied to files:
.maestro/tests/teams/utils/close-action-sheet.yaml
🧬 Code graph analysis (1)
.maestro/scripts/data.js (1)
.maestro/scripts/data-setup.js (1)
output(7-7)
🪛 markdownlint-cli2 (0.18.1)
.maestro/README.md
42-42: Bare URL used
(MD034, no-bare-urls)
🪛 Shellcheck (0.11.0)
.github/scripts/run-maestro.sh
[warning] 56-56: name appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (19)
app/containers/InAppNotification/index.tsx (1)
62-62: Excellent improvement to E2E test performance!Reducing the notification duration from 60 seconds to 5 seconds for E2E builds is a significant improvement that will speed up test execution while still providing sufficient visibility. The fallback to 3 seconds for normal operation is also appropriate, and the logic correctly prioritizes
notification.customTimewhen provided..maestro/tests/assorted/accessibility-and-appearance.yaml (1)
90-90: LGTM! Improved test reliability with exact text matches.Replacing regex patterns with exact text matches makes these assertions more deterministic and performant while reducing false positives. The overly permissive
.*text.*patterns could have matched unintended substrings.Also applies to: 94-94, 129-129, 133-133, 168-168, 203-203
.maestro/tests/assorted/user-preferences.yaml (1)
84-90: LGTM - Test logic improved.The removal of the redundant
notVisiblecheck for':('improves the test correctness. The current flow properly validates:
- When ASCII-to-emoji is OFF:
':('is visible (line 84-85) and'😞'is NOT visible (lines 87-90) ✓- When ASCII-to-emoji is ON:
'😞'is visible (line 142) ✓Having both
visibleandnotVisiblechecks for the same text without intermediate actions would have been contradictory..maestro/tests/teams/utils/close-action-sheet.yaml (1)
9-9: LGTM! Direction value corrected to uppercase.The swipe direction is now uppercase (
DOWN), which aligns with Maestro's official convention for direction values.Based on learnings, Maestro accepts uppercase direction values (UP, DOWN, LEFT, RIGHT) as documented in the official examples.
app/reducers/server.test.ts (1)
52-52: LGTM! Test data updated to QA domain.The test data URL has been correctly updated to align with the broader QA domain migration across the test suite.
app/views/NewServerView/utils/completeUrl.test.ts (1)
17-17: LGTM! Test expectations updated to QA domain.The test expectations have been correctly updated to use the QA domain, maintaining the same test logic while aligning with the broader domain migration across the test suite.
Also applies to: 21-21, 34-34
.maestro/scripts/data.js (1)
2-2: LGTM! Seed data updated to QA domain.The server URL has been correctly updated to the QA domain, and the file now has a proper trailing newline, aligning with the broader test environment migration.
Also applies to: 18-18
.github/workflows/maestro-ios.yml (1)
39-39: LGTM! Maestro installation URL updated for reliability.The installation URL change is consistent with the Android workflow and addresses the reliability issue when the primary endpoint is unavailable. The verification requested in the Android workflow review applies here as well.
.maestro/tests/onboarding/server-history.yaml (1)
26-26: LGTM! Element IDs updated to QA domain.All server history element identifiers have been correctly updated to reference the QA domain. The test flow logic remains unchanged, and the updates are consistent with the server URL change in the seed data.
Also applies to: 30-30, 46-46, 52-52, 56-56
.maestro/tests/room/unread-badge.yaml (2)
7-7: Good practice to isolate flaky tests.Restricting this test to Android-only addresses the iOS flakiness mentioned in the PR objectives. This improves overall test reliability.
25-26: Animation synchronization improves test stability.The addition of
waitForAnimationToEndblocks throughout the test helps ensure UI state is settled before assertions, reducing race conditions.Also applies to: 38-39, 53-54, 66-67, 82-83, 95-96, 111-112, 124-125
.maestro/scripts/random.js (1)
10-32: Password generation meets requirements.The function correctly ensures passwords contain at least one lowercase, uppercase, and numeral, and prevents three consecutive identical characters. The initial three characters are guaranteed diverse since they come from separate character sets.
app/lib/methods/helpers/getAvatarUrl.test.ts (1)
7-7: Test data migration to QA domain.This change aligns with the broader migration to QA endpoints for test stability, as indicated in the PR objectives.
Also applies to: 10-10
.github/scripts/run-maestro.sh (2)
8-8: Increased retry limit improves resilience.Increasing
MAX_RERUN_ROUNDSfrom 2 to 3 addresses intermittent API failures as mentioned in the PR objectives, improving overall test stability.
54-60: String-based deduplication improves macOS compatibility.Replacing the associative array with string-based tracking using
grepensures compatibility with macOS's default bash 3.x, which doesn't support associative arrays. While less efficient, this approach is more portable.Note on static analysis warning: The
namevariable at line 56 is intentionally read but unused. It's part of the tab-separated parsing where onlypathis needed for deduplication. The shellcheck warning is a false positive.app/lib/methods/getQuoteAttachment.test.ts (1)
22-22: Test data migration to QA domain.Comprehensive update of test fixtures to use QA endpoints, consistent with the broader migration strategy across the PR.
Also applies to: 30-30, 50-50, 70-70, 91-91
.maestro/tests/room/room.yaml (3)
259-259: LGTM! Direction case now consistent.The uppercase
DOWNaligns with the existingUPdirection at line 420, establishing consistency across swipe actions.
268-272: LGTM! Improved selector determinism and tap reliability.The ID-based selector and
retryTapIfNoChangeflag enhance test stability by targeting elements deterministically and handling scenarios where the tap action may not produce immediate visible changes.
293-297: LGTM! Consistent application of improved selector pattern.The same ID-based selector approach and retry mechanism are correctly applied to the other user's profile interaction, maintaining consistency with the pattern established earlier in the file.
| - runFlow: | ||
| when: | ||
| platform: iOS | ||
| commands: | ||
| - tapOn: | ||
| point: 65%,55% | ||
| retryTapIfNoChange: true | ||
| - runFlow: | ||
| when: | ||
| platform: Android | ||
| commands: | ||
| - tapOn: | ||
| text: 'Delete' | ||
| retryTapIfNoChange: true |
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.
Use text or accessibility identifier-based selectors instead of coordinates for cross-device compatibility.
The iOS flow's hardcoded coordinates (65%, 55%) are device-dependent and may fail on different screen sizes or resolutions. The Android flow already uses the superior text: 'Delete' selector. Consider whether the Delete button has an accessibility identifier and update both flows consistently:
- runFlow:
when:
platform: iOS
commands:
- tapOn:
text: 'Delete'
retryTapIfNoChange: true
- runFlow:
when:
platform: Android
commands:
- tapOn:
text: 'Delete'
retryTapIfNoChange: trueNote: retryTapIfNoChange addresses timing issues with screen hierarchy changes, not coordinate fragility.
🤖 Prompt for AI Agents
.maestro/tests/assorted/changeserver.yaml around lines 183 to 196: the iOS
runFlow uses hardcoded screen coordinates (65%,55%) which are device-dependent;
replace the coordinate-based tap with a selector-based tap (use the button's
text or its accessibility identifier) and make the iOS flow consistent with the
Android flow by using the same tapOn selector (e.g., text: 'Delete' or
accessibilityId: '<identifier>') while keeping retryTapIfNoChange: true to
preserve retry behavior.
| - runFlow: | ||
| when: | ||
| platform: iOS | ||
| commands: | ||
| - tapOn: | ||
| point: 65%,55% | ||
| retryTapIfNoChange: true | ||
| - runFlow: | ||
| when: | ||
| platform: Android | ||
| commands: | ||
| - tapOn: | ||
| text: 'Delete' | ||
| retryTapIfNoChange: true |
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.
Coordinate-based taps are fragile across device configurations.
Same concern as in changeserver.yaml: the iOS flow uses hardcoded coordinates (65%, 55%) which may fail on devices with different screen configurations. Consider using ID-based selectors if available.
🤖 Prompt for AI Agents
In .maestro/tests/assorted/delete-server.yaml around lines 96 to 109, the iOS
step uses a hardcoded coordinate tap (65%,55%) which is fragile; replace the
coordinate-based tap with an element-based selector (accessibility id,
resource-id, label or visible text) that targets the Delete button, preserve
retryTapIfNoChange behavior, and add a safe fallback to the coordinate tap only
if the selector lookup fails so tests remain robust across screen sizes and
configurations.
| required | ||
| label={I18n.t('Password')} | ||
| secureTextEntry | ||
| secureTextEntry={process.env.RUNNING_E2E_TESTS !== 'true'} |
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.
Verify test data hygiene to prevent password exposure.
Disabling secureTextEntry during E2E tests makes passwords visible in plain text, which could appear in test artifacts (screenshots, recordings, logs). Ensure test passwords are synthetic and never reused.
Also applies to: 258-258
🤖 Prompt for AI Agents
In app/views/RegisterView/index.tsx around lines 243 and 258, tests currently
disable secureTextEntry for E2E which can expose passwords in artifacts; either
keep secureTextEntry enabled at all times or, if E2E requires visible input,
ensure the code pulls only synthetic, non-reused test passwords (generated by
the test harness or from a dedicated TEST_PASSWORD env var) and never real
credentials, and add a comment and/or assert to make it explicit these values
are ephemeral and not stored or reused.
…Rocket.Chat.ReactNative into new-mobile-workspace
Proposed changes
Working Action Link: https://github.com/RocketChat/Rocket.Chat.ReactNative/actions/runs/20311081572/job/58355918165
Issue(s)
https://rocketchat.atlassian.net/browse/COMM-104
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.