chore: iOS Maestro test fix#6700
Conversation
…eactNative into 3rd-set-of-room-test
…eactNative into 3rd-set-of-room-test
…Rocket.Chat.ReactNative into 3rd-set-of-room-test
…Rocket.Chat.ReactNative into 3rd-set-of-room-test
…Rocket.Chat.ReactNative into 3rd-set-of-room-test
…Rocket.Chat.ReactNative into maestro-ios-test-fix
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.maestro/tests/assorted/profile.yaml (1)
117-118: Add testIDs to label elements or use ID-based keyboard dismissal mechanism.The review comment is valid. ProfileView components expose
testID='profile-view-nickname'andtestID='profile-view-bio'on input fields, confirming PR changes align with the stated goal. However, the YAML test (lines 117–118, 125–126) uses text-based selectors (text: '.*Nickname.*'andtext: '.*Bio.*') to target the label elements for keyboard dismissal, which contradicts moving toward stable element IDs.To align with the PR's refactoring goal:
- Add testIDs to label elements rendered by FormTextInput (line 145 in
app/containers/TextInput/FormTextInput.tsx), or- Use an ID-based approach to dismiss the keyboard (e.g., tapping a dismiss button with a testID if available in Maestro, or a Maestro keyboard dismiss command).
🧹 Nitpick comments (5)
.maestro/tests/assorted/profile.yaml (2)
12-12: Clarify the purpose ofstartRecordingandstopRecordingcommands.The file adds
startRecording: 'user'(line 12) andstopRecording(line 216) to bracket the profile editing test flow. While these appear intentional for capturing test execution data, the purpose and expected behavior are not documented in the code.Please add a comment explaining what data is being captured and why—is this for observability, debugging, or something else?
Also applies to: 216-216
101-101: Consider ID-based selector for keyboard hiding instead of text regex.Line 101 uses a text-based selector
'.*Username.*'to hide the keyboard on iOS. While the inline comment explains the intent, this is inconsistent with the PR's goal of migrating to stable element IDs instead of text-based selectors.Check if an ID-based selector exists for this element (e.g.,
id: 'profile-view-username') or if a platform-specific helper could replace this tap operation..maestro/helpers/navigate-to-room.yaml (1)
7-7: Double waitForAnimationToEnd may add unnecessary latency; consider consolidation.Lines 7 and 11 both call
waitForAnimationToEnd, creating two sequential animation waits:
- Before
scrollUntilVisible(line 7)- Before
tapOn(line 11)If animations are only triggered by scrolling and complete before the tap, the second wait adds latency without benefit. If animations occur independently between scroll and tap, both waits are necessary.
Recommendation: Document why both waits are needed, or run tests to confirm both are essential. If the scroll itself completes all animations, the second wait can be removed.
Also applies to: 11-11
.github/workflows/maestro-ios.yml (1)
92-107: Consider making retry configuration more granular.The retry action uses hard-coded
max_attempts: 3andtimeout_minutes: 60. While retry logic is excellent for handling flakiness, the current configuration could mask real issues or waste CI resources.Consider:
- Reducing
timeout_minutesto 30-45 minutes per attempt (60 minutes per retry seems excessive for mobile tests)- Making
max_attemptsconfigurable via workflow inputs to allow different retry strategies per environment- Adding failure categorization to distinguish between infrastructure failures (worth retrying) and actual test failures
Apply this diff to make timeout more reasonable:
with: max_attempts: 3 - timeout_minutes: 60 + timeout_minutes: 30 command: |.maestro/tests/room/discussion.yaml (1)
129-130: Minor formatting inconsistency in selector syntax.Line 130 uses
id: message-composer-actionswithout quotes, while other id references in the file use quotes (e.g.,id: 'multi-select-search'). For consistency, wrap this in quotes:- tapOn: - id: message-composer-actions + tapOn: + id: 'message-composer-actions'
📜 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 (25)
.github/workflows/build-pr.yml(1 hunks).github/workflows/maestro-android.yml(1 hunks).github/workflows/maestro-ios.yml(2 hunks).maestro/helpers/erase-text.yaml(1 hunks).maestro/helpers/hide-keyboard.yaml(1 hunks).maestro/helpers/launch-app.yaml(1 hunks).maestro/helpers/login-with-deeplink.yaml(2 hunks).maestro/helpers/logout.yaml(2 hunks).maestro/helpers/navigate-to-room.yaml(1 hunks).maestro/helpers/open-deeplink.yaml(1 hunks).maestro/tests/assorted/accessibility-and-appearance.yaml(1 hunks).maestro/tests/assorted/broadcast.yaml(1 hunks).maestro/tests/assorted/profile.yaml(5 hunks).maestro/tests/room/create-dm-group.yaml(1 hunks).maestro/tests/room/discussion.yaml(3 hunks).maestro/tests/room/ignoreuser.yaml(3 hunks).maestro/tests/room/jump-to-message.yaml(8 hunks).maestro/tests/room/room-actions.yaml(2 hunks).maestro/tests/room/room-info.yaml(8 hunks).maestro/tests/room/room.yaml(9 hunks).maestro/tests/room/share-message.yaml(4 hunks).maestro/tests/room/threads.yaml(1 hunks).maestro/tests/teams/team.yaml(1 hunks)app/views/CreateDiscussionView/SelectChannel.tsx(1 hunks)app/views/ForwardMessageView/SelectPersonOrChannel.tsx(1 hunks)
🔇 Additional comments (31)
.maestro/tests/room/threads.yaml (1)
166-169: Verify: This change contradicts the PR's stated objective to use stable element IDs.The change replaces an element-id-based selector (
id: 'message-preview-sendToChannel') with a text regex selector (text: '.*sendToChannel.*'). However, the PR summary indicates the broader effort is to "use stable element IDs instead of text-based selectors." Text-based regex matching is inherently less stable and more prone to flakiness than element IDs.A few questions to clarify:
- Is the element
message-preview-sendToChannelunavailable or unreliable on iOS specifically? (This is an iOS-focused fix per the PR title.)- Should the element ID be exposed/fixed on the React component instead of regressing to text-based selection?
- Is this change intentional as a workaround, and if so, should it be documented or marked as a known limitation?
.maestro/tests/teams/team.yaml (1)
320-329: Consider timeout strategy consistency and duration appropriateness.The change adds a 60-second timeout to an
extendedWaitUntilfor user selection, but the file shows inconsistent timeout patterns:
- Early waits (lines 45, 65, 70, 75):
5000ms timeout- Later waits (most blocks after line 88):
60000ms timeoutA 60-second timeout is unusually long. While this may be necessary if the test environment is consistently slow or if tests are frequently timing out in CI, this should be verified. Consider whether shorter timeouts (10–30 seconds) would be sufficient, or if there are environment-specific performance issues that warrant such long waits.
To understand the timeout strategy, could you clarify:
- Why 60 seconds? Is this a known issue with test flakiness or CI environment performance?
- Consistency: Why do earlier waits use 5 seconds while later ones use 60 seconds? Should they be standardized?
- Test environment: Does this timeout differ between local development and CI (iOS/Android)?
.maestro/tests/assorted/accessibility-and-appearance.yaml (1)
7-9: Verify that theandroid-onlytag restriction is intentional.The addition of the
android-onlytag restricts this accessibility and appearance test to Android only. All assertions in the test use testID-based selectors, which should theoretically work on both platforms if the underlying component testIDs are exposed.Please confirm that:
- This test is intentionally platform-specific (e.g., due to iOS-specific behavior or missing testID exposure).
- Equivalent iOS coverage exists elsewhere or is deferred to a separate PR.
If the testIDs are exposed on iOS components but the test has not yet been validated on iOS, consider removing the
android-onlytag to enable broader coverage..github/workflows/maestro-android.yml (1)
124-130: Retention-days configuration is correct.The addition of
retention-days: 7to the screen recording upload step ensures consistency with the test report artifact retention (line 122). Screen recordings are valuable for debugging failed tests, and the 7-day retention window aligns with the test report retention..maestro/tests/assorted/profile.yaml (1)
106-106: Good:centerElementadditions improve UI stability.The addition of
centerElement: trueto thescrollUntilVisiblesteps (lines 106, 131, 154) is appropriate for improving test reliability on iOS by ensuring elements are properly centered during scroll operations.Also applies to: 131-131, 154-154
.maestro/helpers/launch-app.yaml (1)
16-21: Platform-specific condition addition is good; note summary inconsistency.The addition of
platform: Androidto line 19 correctly restricts the Pixel Launcher close tap to Android devices only. This is a sound platform-specific guard.However, the AI summary claims the retry block was "removed," but the code still shows the retry wrapper intact (lines 7–15). The retry logic remains unchanged and functional.
.maestro/helpers/erase-text.yaml (1)
7-31: Robust conditional text-erasing logic with multi-path support.The refactored erase-text helper now handles three scenarios based on input state:
- ID-based erasing (lines 7–12): if
idis provided, long-press on the id.- Text-based erasing (lines 13–18): if
textis provided, double-tap on the text pattern.- Select All visibility (lines 19–31): if 'Select All' is visible, tap and erase 1; otherwise erase 100.
This multi-path approach is more resilient than a single erase strategy and aligns well with the PR's shift toward conditional, state-aware flows.
.github/workflows/build-pr.yml (1)
103-108: Verification complete: maestro-ios.yml correctly accepts and uses the shard parameter.The maestro-ios.yml workflow:
- Declares
shardas a required input parameter (lines 5–7)- Uses the shard value to filter tests via
--include-tags=test-$SHARD(Run Tests step)- Partitions test execution across 13 shards by maestro tag filtering, preventing duplicate or overlapping test runs
- Follows the same pattern as maestro-android.yml for consistency
The build-pr.yml configuration correctly passes the matrix shard value to the workflow.
.maestro/tests/assorted/broadcast.yaml (1)
134-135: The review comment is based on an incorrect assumption.The test selector
'message-content-message'is not a static testID that should exist in the React component code. Instead, it's dynamically generated at runtime.The Content component (line 83 of
app/containers/message/Content.tsx) creates the testID using the pattern:testID={message-content-${props.msg || ''}}In this test, the message text sent is the literal string
'message'(line 101–102 of broadcast.yaml), so when the component renders,props.msgequals'message', resulting in the testID'message-content-message'.The test selector is correct and will work as expected.
Likely an incorrect or invalid review comment.
.maestro/helpers/logout.yaml (1)
77-77: Verify the aggressive timeout reduction.The timeout was reduced from 60000ms to 5000ms (a 92% reduction). This could introduce flakiness if navigation to the new-server-view takes longer under certain conditions (slow devices, network delays, etc.).
Please verify that this reduced timeout is sufficient across all test environments, or consider using a more conservative value like 15000-30000ms.
.github/workflows/maestro-ios.yml (1)
117-131: Good addition of shard-specific artifacts.The shard-specific naming for screenshots and videos will significantly aid debugging parallel test runs, and the 28-day retention provides a reasonable balance between storage and investigation needs.
app/views/ForwardMessageView/SelectPersonOrChannel.tsx (1)
71-71: Good addition of testID for test automation.Adding the
testIDprop enables stable element targeting in Maestro tests, aligning with the broader PR goal of replacing text-based selectors with id-based ones.app/views/CreateDiscussionView/SelectChannel.tsx (1)
75-75: Good addition of testID for test automation.Adding the
testIDprop enables stable element targeting in Maestro tests, consistent with the test improvements across this PR..maestro/tests/room/create-dm-group.yaml (1)
43-46: Excellent migration to id-based selectors.Replacing the regex text selector
'.*Direct message.*'with the explicit id'new-message-view-create-direct-message'significantly improves test stability and reduces the chance of false positives from matching unintended text..maestro/tests/room/room-actions.yaml (3)
130-133: Good addition of explicit wait for toast dismissal.Waiting for the "Message Starred" toast to disappear before proceeding ensures the UI is in a stable state, reducing potential race conditions in subsequent steps.
141-142: Excellent improvements for stability.Replacing the text selector with
id: 'room-actions-starred'and addingretryTapIfNoChange: trueaddresses two common sources of test flakiness:
- Text-based selectors can break with localization or copy changes
- The retry flag handles cases where the first tap doesn't register
180-192: Consistent application of stability improvements.The same improvements applied to the Unstar flow (wait for toast dismissal, id-based selector, retry flag) ensure consistency across related test scenarios.
.maestro/tests/room/room.yaml (3)
320-358: Good migration to id-based message targeting.The changes from text-based to id-based selectors for edit and quote actions improve test reliability, assuming the testIDs are properly set in the UI components.
110-110: Pattern verified as implemented.The testID generation pattern is correctly implemented in the component. In
app/containers/message/Content.tsx(line 83), the View component sets:testID={`message-content-${props.msg || ''}`}This confirms that the UI components generate testIDs by concatenating
'message-content-'with the message text, exactly as the YAML test assumes. The test snapshots show this pattern in action across multiple message types (e.g.,testID="message-content-Hey!",testID="message-content-Lorem Ipsum...").
85-98: Remove this review comment – the underlying issue is test code, not test data availability.The maestro test setup creates a random room via
output.utils.createRandomRoom(), but the test then searches for a hardcoded#translation-testroom that doesn't exist. This is a code bug, not a test environment issue. The room autocomplete test should use${output.room.name}to match the pattern used for emoji (hardcodedvalues) and user autocomplete (${output.user.username}).Likely an incorrect or invalid review comment.
.maestro/tests/room/jump-to-message.yaml (4)
104-105: Unusual input splitting - verify necessity.The input was changed from a single
inputText: '30'to two sequential inputs ('3'then'0'). This splitting pattern often indicates a workaround for timing issues or autocomplete interference.Please confirm:
- Why was this split necessary?
- Is there an underlying issue with the search input that should be addressed in the application code rather than worked around in tests?
- Does this pattern need to be applied to other search operations?
If this is a workaround for autocomplete or debouncing behavior, consider documenting it with a comment in the test.
158-181: Good addition of retry flags for potentially flaky operations.Adding
retryTapIfNoChange: trueto the "Load newer" taps addresses a common source of flakiness where scroll position or lazy loading can cause the first tap to be missed or ineffective.
243-243: Improved precision with exact text matching.Changing from the regex pattern
'.*quoted.*'to the exact match'quoted'reduces the chance of matching unintended elements and makes the test more deterministic.
28-32: Regex pattern confirmed as intentional—no issues found.The pattern
id: '.*Quote first message.*'is verified to use regex intentionally. No hardcoded testID matching this exact string exists in the codebase; this is a test-time fixture with dynamic matching. The.*wildcards are deliberate regex syntax, and the selector works correctly withretryTapIfNoChange: truefor reliable E2E testing..maestro/tests/room/share-message.yaml (1)
41-110: Solid migration from text-based to id-based selectors throughout the test.The consistent shift to stable element IDs (
message-content-Hello room,message-content-Hello user,select-person-or-channel,action-sheet-handle) improves test reliability and maintainability. The pattern is clean and follows Maestro best practices.Verify that all referenced testID props are properly exposed in the React components:
message-content-Hello roomandmessage-content-Hello user→ MessageContent or related componentselect-person-or-channel→ ForwardMessageView or SelectPersonOrChannel componentaction-sheet-handle→ ActionSheet or BottomSheet componentCan you confirm these testIDs are present in the corresponding React component implementations?
.maestro/tests/room/ignoreuser.yaml (2)
122-137: Platform-specific message content waits may indicate a platform-specific rendering or ordering issue.Lines 123–137 introduce a pattern where Android waits for
message-content-message-01while iOS waits formessage-content-message-02. This suggests that the two messages (created at lines 85–86) appear in different order or with different visibility on each platform.This conditional may be a workaround rather than a root cause fix. Before merging:
- Clarify why the message order or visibility differs between platforms. Is this expected behavior or a bug in the app?
- If this is expected (e.g., reverse chronological ordering on iOS), document this in a comment above the conditional.
- If this is a bug, consider fixing it at the component level rather than working around it in tests.
Can you provide context on what is driving this platform difference?
87-144: Good use of id-based selectors andretryTapIfNoChangefor reliability.The migration to id-based selectors (
username-header-${output.otherUser.username}) and consistent use ofretryTapIfNoChange: trueimproves test robustness. These patterns reduce flakiness from timing issues and stale element references..maestro/tests/room/room-info.yaml (4)
23-23: Verify recording scope and necessity.Lines 23 and 451 introduce test recording for this file only. This is useful for debugging, but clarify:
- Should recording be added to other critical tests or is this specific to room-info?
- Is this recording intended for CI artifact collection or just local debugging?
- Does the Maestro configuration have storage/cleanup for these recordings?
Document the intent and scope of recording if it's being systematized across tests.
Also applies to: 451-451
157-157: Good: Explicit keyboard hiding improves test reliability across platforms.Adding
runFlow: '../../helpers/hide-keyboard.yaml'after text inputs is a solid reliability pattern. This ensures keyboard dismissal doesn't interfere with subsequent taps or scrolls. Verify that the hide-keyboard helper is portable across iOS and Android.Confirm that
.maestro/helpers/hide-keyboard.yamlcorrectly dismisses keyboards on both platforms without side effects.Also applies to: 165-165, 173-173, 181-181, 189-189, 332-332, 340-340, 348-348
194-194:centerElement: trueimproves targeting precision for small or partially-visible elements.The addition of
centerElement: trueto scrollUntilVisible calls helps prevent tap misses on elements at screen edges. This is a good reliability improvement.Also applies to: 201-201
301-309: Platform-specific success message waits are Android-only; clarify intent and scope.Lines 301–309, 355–362, and 395–402 add platform-specific conditional flows that wait for success messages (
.*Settings succesfully changed.*) only on Android. This raises questions:
- Why Android-only? Does iOS not display this toast, handle it differently, or is this a temporary workaround?
- Is this a bug or feature? Should the app show consistent feedback across platforms?
- Is this complete? Will iOS need equivalent success message waits in the future?
Clarify whether these conditionals are:
- Temporary workarounds pending iOS feature parity
- By design (iOS uses different feedback mechanism)
- Technical debt to be addressed later
Additionally, note the typo "succesfully" → "successfully" (likely in the app's message, but worth fixing).
Also applies to: 355-362, 395-402
| - runFlow: | ||
| when: | ||
| platform: iOS | ||
| commands: | ||
| - tapOn: | ||
| point: 1%,20% |
There was a problem hiding this comment.
Hardcoded iOS coordinate is fragile and may not work across device sizes or orientations.
The iOS keyboard dismissal uses a hardcoded coordinate point: 1%,20% instead of an element selector. This approach is brittle because:
- It assumes a fixed screen layout and will break on landscape orientation or different device sizes
- If the app structure changes, the tap location becomes invalid
- No clear documentation of what UI element this coordinate targets
Consider using an id-based selector or a visibility condition if an appropriate element is available, or document why the coordinate is stable (e.g., "taps outside all interactive elements").
| - runFlow: | ||
| when: | ||
| visible: '.*Would like to send you notifications.*' | ||
| platform: iOS | ||
| commands: | ||
| - tapOn: | ||
| point: 65%,60% |
There was a problem hiding this comment.
Hardcoded iOS coordinate for notification permission is fragile.
Similar to the keyboard helper, this iOS notification flow uses a hardcoded coordinate point: 65%,60% instead of an element selector. This approach is brittle and will break if:
- The notification alert layout changes
- The app runs in landscape orientation
- Device screen size varies
Android uses text matching ('.*Allow.*') for similar permission flows, which is more robust. Consider whether iOS can also use text matching or an id-based selector.
🤖 Prompt for AI Agents
In .maestro/helpers/login-with-deeplink.yaml around lines 34 to 40, the iOS
notification permission uses a hardcoded coordinate (point: 65%,60%), which is
fragile; replace the tapOn coordinate with a selector-based action (prefer an
accessibility id or regex text match for the alert button, e.g. match button
label like 'Allow' or use the alert button's accessibility identifier) so the
test targets the actual button text/ID rather than screen coordinates; if a
selector cannot be determined for all devices, implement a prioritized strategy:
1) try element selector (id/accessibilityLabel), 2) try regex text match for
common labels, 3) only as a last-resort keep a coordinate fallback and document
it.
| - tapOn: | ||
| text: Logout | ||
| index: 1 | ||
| point: '65%,56%' |
There was a problem hiding this comment.
Replace coordinate-based tap with id-based selector.
The coordinate-based tap (point: '65%,56%') is brittle and will break if the UI layout changes. This creates an inconsistency with the Android implementation (lines 64-65) which uses a text-based selector.
Consider exposing a testID for the iOS logout button in the confirmation dialog and using it here, similar to how Android targets the button by text. This would make the test more maintainable and aligned across platforms.
🤖 Prompt for AI Agents
In .maestro/helpers/logout.yaml around line 72, replace the brittle
coordinate-based tap ("point: '65%,56%'") with an id/accessibility selector: add
or use an iOS testID (accessibilityIdentifier) on the logout confirmation button
in the app code, then change this step to tap by that id (e.g., selector: { id:
'logoutConfirmButton' } or equivalent Maestro selector syntax) so the test
matches the Android text-based approach and is resilient to layout changes.
| - tapOn: | ||
| text: Open | ||
| index: 0 | ||
| optional: true | ||
| - tapOn: | ||
| text: Open | ||
| index: 1 | ||
| optional: true |
There was a problem hiding this comment.
Index-based multi-tap sequencing is fragile; clarify the intent and add documentation.
Lines 14–21 replace a single tap with two sequential taps at different indices. This suggests handling multiple "Open" buttons in a share sheet, but the intent is unclear:
- Why two taps? Is the second tap a fallback, or are both always required?
- Brittle index assumptions: If the iOS sheet UI changes and the number/order of "Open" buttons shifts, this will break.
- optional: true: Both taps are marked optional, which mitigates outright failure but may mask underlying UI instability.
Recommendation: Add an inline comment explaining the scenario (e.g., "iOS 15+ share sheet can show multiple 'Open' buttons; we tap both as fallback") and consider whether a text pattern or visibility condition would be more resilient.
🤖 Prompt for AI Agents
In .maestro/helpers/open-deeplink.yaml around lines 14 to 21, the two sequential
taps using hard-coded indices for "Open" are fragile and unclear; add an inline
comment explaining the exact intent (e.g., "tap index 0, fallback to index 1 on
iOS X share-sheet behavior") and then replace the brittle index-only approach
with a more robust sequence: attempt to tap the visible/first matching "Open"
element by text or visibility predicate and only fall back to the second index
if the first is not present, keeping optional: true for the fallback; also add a
brief note in the file header describing the iOS versions/conditions that
require this behavior so future reviewers understand the rationale.
| - extendedWaitUntil: | ||
| visible: | ||
| text: ${output.room.name} | ||
| text: .*${output.room.name}.* | ||
| timeout: 60000 |
There was a problem hiding this comment.
🧩 Analysis chain
Inconsistent selector strategy: line 145 retains text-based regex while rest of file uses id-based selectors.
Line 145 uses text: .*${output.room.name}.*, which breaks the otherwise consistent migration to stable id-based selectors. Consider migrating this to an id-based selector (e.g., id: 'discussions-view-header-${output.room.name}' or similar) to improve reliability and align with the broader test refactoring strategy.
Is there a reason this element cannot be targeted by a stable testID, or is this an oversight during refactoring?
🏁 Script executed:
#!/bin/bash
# View the discussion.yaml file with line numbers to verify selector patterns
cat -n .maestro/tests/room/discussion.yaml | sed -n '40,55p; 125,150p; 310,325p'Length of output: 1748
🏁 Script executed:
#!/bin/bash
# Get overall statistics on selector usage in the file
echo "=== Text-based selectors (text:) ==="
rg -n "^\s+text:" .maestro/tests/room/discussion.yaml | head -20
echo ""
echo "=== ID-based selectors (id:) ==="
rg -n "^\s+id:" .maestro/tests/room/discussion.yaml | head -20
echo ""
echo "=== Total line count ==="
wc -l .maestro/tests/room/discussion.yamlLength of output: 1154
Fix two selector inconsistencies to complete migration to id-based selectors:
-
Line 145: Replace
text: .*${output.room.name}.*with an id-based selector (e.g.,id: 'discussions-view-header-${output.room.name}'or appropriate testID). This regex pattern is fragile and inconsistent with the file's migration pattern (see lines 54, 317, 323 for working examples of interpolated id selectors). -
Line 130: Add quotes around the id value: change
id: message-composer-actionstoid: 'message-composer-actions'to match the formatting at line 127 and maintain YAML consistency throughout the file.
🤖 Prompt for AI Agents
In .maestro/tests/room/discussion.yaml around lines 143-146 and line 130,
replace the fragile regex text selector at line 145 with an id-based selector
that interpolates the room name (for example use id:
'discussions-view-header-${output.room.name}' or the appropriate testID used
elsewhere), and also add quotes around the id value at line 130 so it reads id:
'message-composer-actions' to match the file’s id-based selector formatting and
YAML consistency.
| - scrollUntilVisible: | ||
| element: | ||
| id: 'room-info-edit-view-name' | ||
| id: 'avatar-edit-button' | ||
| timeout: 60000 | ||
| direction: UP | ||
| - scrollUntilVisible: | ||
| element: | ||
| text: '.*${output.room.name}.*' | ||
| timeout: 60000 |
There was a problem hiding this comment.
Inconsistent selector strategy: line 224 retains text-based regex while most of file uses id-based selectors.
Line 224 uses text: '.*${output.room.name}.*', which breaks the pattern of id-based selectors seen elsewhere in this file and across the test suite. Consider migrating to an id-based selector:
- scrollUntilVisible:
- element:
- text: '.*${output.room.name}.*'
+ scrollUntilVisible:
+ element:
+ id: 'room-info-view-name'(Or use the appropriate stable testID that matches the room name.)
Is there a reason to retain text-based matching here, or should this be refactored to match the broader selector strategy?
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.maestro/tests/room/room-info.yaml around lines 217 to 225: the
scrollUntilVisible call at line 224 uses a text-based regex selector ('text:
.*${output.room.name}.*') which is inconsistent with the id/testID-based
selector strategy used elsewhere; replace this text regex with the stable
id/testID selector that corresponds to the room name (e.g., compute or
interpolate the appropriate element id/testID that includes the room identifier
or add a dedicated testID in the app if missing), then update the
scrollUntilVisible element to use that id-based selector and remove the regex
text selector so the file follows the consistent selector strategy.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.maestro/tests/room/room-info.yaml (1)
221-223: Text-based regex selector inconsistent with id-based strategy (previously flagged).This is the same selector consistency issue flagged in the prior review. Line 221–223 retains the text-based regex
text: '.*${output.room.name}.*'while the rest of the file uses stable id-based selectors. This reduces test robustness and maintainability.Please migrate this to an id-based selector matching the broader test strategy. If no stable testID exists for the room name display in the reset verification context, consider adding one to the app (e.g.,
room-info-view-nameor similar):- element: - text: '.*${output.room.name}.*' + element: + id: 'room-info-view-name'If the element is different from the initial room info name display, provide the correct stable testID and update accordingly.
🧹 Nitpick comments (2)
.maestro/tests/room/room-info.yaml (2)
60-63: Duplicated wait for edit-button visibility.Lines 60–63 repeat the wait for
room-info-view-edit-buttonvisibility that is already checked at lines 55–58. This redundancy is unnecessary and can slow test execution.Consider removing the duplicate wait:
- extendedWaitUntil: - visible: - id: 'room-info-view-edit-button' - timeout: 60000 - tapOn: id: 'room-info-view-edit-button'
193-193:centerElement: trueplacement for switch elements needs verification.The addition of
centerElement: trueto scrollUntilVisible steps for thet(type) andro(read-only) fields suggests these elements require centered positioning for reliable interaction. This is reasonable for toggle switches, but verify that this setting doesn't introduce regressions on different device sizes or orientations.Also applies to: 200-200
📜 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 (2)
.maestro/tests/assorted/profile.yaml(3 hunks).maestro/tests/room/room-info.yaml(6 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: ESLint and Test / run-eslint-and-test
🔇 Additional comments (10)
.maestro/tests/assorted/profile.yaml (4)
105-105: Robust scroll positioning improvements.Adding
centerElement: trueto thesescrollUntilVisiblesteps improves tap accuracy and element visibility, which should help reduce test flakiness on iOS. Good addition for consistency across profile update flows.Also applies to: 130-130, 153-153
100-100: Verify keyboard hide utility integration.The PR mentions adding a utility to hide the keyboard for better reliability. Lines 100, 116–117, and 124–125 currently use text-based taps to hide the keyboard. Confirm whether these should be replaced with the new keyboard hide utility for consistency and improved robustness, or whether text-based dismissal is intentional here.
Also applies to: 116-117, 124-125
201-203: Appropriate use of retry logic.Adding
retryTapIfNoChange: truefor the password button tap improves reliability for this critical interaction, aligning with the PR's focus on addressing iOS test flakiness.
139-144: Appropriate platform-specific handling for iOS email interaction.Using
longPressOnfor iOS email field update is a good platform-specific adjustment that should improve text selection and input reliability on iOS..maestro/tests/room/room-info.yaml (6)
144-151: Navigation refactoring improves tap reliability.The switch from
scrollUntilVisiblewith UP direction to explicittapOnfor back and edit buttons aligns well with the PR's goal of avoiding tap-detection issues. The explicit wait for name input visibility before interaction is also a solid stabilization pattern.
156-156: Keyboard hiding consistently applied after text inputs.The repeated application of the
hide-keyboardhelper after each text input (name, topic, announcement, description, password) is a solid pattern for improving test reliability on iOS/macOS. This directly supports the PR's goal of handling macOS hangs and timeouts.Also applies to: 164-164, 172-172, 180-180, 188-188
300-307: Platform-specific toast validation properly wrapped for Android only.The wrapping of success message validation in a
runFlowwithplatform: androidcondition correctly implements the PR's stated goal of skipping toast validation on iOS due to text readability issues. This is appropriate and aligns with the broader strategy to improve test stability.
331-331: Keyboard hiding consistently maintained across multiple input fields.The
hide-keyboardhelper is properly invoked after each of the three text inputs (topic, announcement, description), maintaining consistency with the earlier sections of the test and supporting improved macOS timeout handling.Also applies to: 339-339, 347-347
354-361: Android-only toast validation pattern consistent with earlier sections.The platform-specific wrapping for success message validation is correctly applied here as well, maintaining consistency across all edit operations and aligning with the PR's approach to skip iOS toast checks due to text readability constraints.
394-401: Platform-specific toast validation consistent across all edit operations.The Android-only success message check is properly applied here as well, maintaining uniformity across the room-type change operation and adhering to the PR's strategy.
2 participants
Proposed changes
This PR introduce the following changes to Maestro E2E testing for iOS
nick-fields/retryto handle cases where macOS instances hang or time out.Known Issues:
Run 1: https://github.com/RocketChat/Rocket.Chat.ReactNative/actions/runs/18725063576
Run 2: https://github.com/RocketChat/Rocket.Chat.ReactNative/actions/runs/18753137128
Issue(s)
https://rocketchat.atlassian.net/browse/COMM-53
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Tests
Chores