test: improve android e2e build and test time#6576
Conversation
Rohit3523
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
Rohit3523
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
…eactNative into maestro-improvements
…eactNative into maestro-improvements
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.github/workflows/build-pr.yml (1)
83-88: Approve Android E2E sharding; document shard count rationale. The workflow already accepts ashardinput—add a brief comment explaining why 13 shards were chosen (e.g., based on test distribution or runner capacity)..maestro/tests/onboarding/legal.yaml (1)
5-6: LGTM! Tag metadata added for test sharding.The addition of the
test-1tag enables parallel test execution. Note that tag placement varies across test files (some useonFlowStart, others useonFlowComplete, and some use top-level placement). While this doesn't affect functionality, consider standardizing the placement for consistency..github/workflows/maestro-android.yml (3)
53-58: Consider adding error handling for critical ADB operations.The script performs critical setup steps (enable touch indicators, install APK, launch app) without checking for failures. If any of these fail, subsequent test execution will proceed in an invalid state.
Consider adding error checks for critical operations:
+set -e # Exit on error + adb shell settings put system show_touches 1 -adb install app-experimental-release.apk +adb install -r app-experimental-release.apk || { echo "APK installation failed"; exit 1; } adb shell monkey -p chat.rocket.reactnative -c android.intent.category.LAUNCHER 1 -sleep 10 -adb shell am force-stop chat.rocket.reactnative + +# Wait for app to be ready instead of fixed sleep +timeout 30 bash -c 'until adb shell "dumpsys window | grep -q mCurrentFocus.*chat.rocket.reactnative"; do sleep 1; done' || echo "App may not be fully started" +adb shell am force-stop chat.rocket.reactnative || trueNote: Added
-rflag toadb installto replace existing installation if present, and replaced the fixedsleep 10with a more robust check for app readiness.
67-80: Screen recording may fail silently.The screen recording process redirects all output to
/dev/null, which means failures (e.g., insufficient storage, permission issues) won't be visible. Additionally, there's no verification that the video was successfully created before attempting to pull it.Consider adding basic error detection:
echo "Starting screen recording..." -adb shell screenrecord /sdcard/test_run.mp4 > /dev/null 2>&1 & +adb shell screenrecord --time-limit 180 /sdcard/test_run.mp4 2>&1 | tee screenrecord.log & RECORD_PID=$! +sleep 1 +if ! ps -p $RECORD_PID > /dev/null 2>&1; then + echo "Warning: Screen recording may have failed to start" +fiAlso consider adding a time limit to prevent excessively large video files and verify the file exists before pulling:
echo "Pulling video from device..." +if adb shell "[ -f /sdcard/test_run.mp4 ]"; then adb pull /sdcard/test_run.mp4 test_run_${SHARD}_attempt_${ATTEMPT}.mp4 || true +else + echo "Warning: Video file not found on device" +fi adb shell rm /sdcard/test_run.mp4 || true
60-93: Consider resetting app state between retry attempts.The retry logic correctly tracks success/failure, but it doesn't reset the app state between attempts. A failing test might leave the app in an inconsistent state that affects subsequent retries, potentially causing false negatives.
Add app state cleanup before each retry attempt:
while [ $ATTEMPT -le $MAX_RETRIES ]; do echo "Attempt $ATTEMPT of $MAX_RETRIES" + + # Reset app state before retry (except first attempt) + if [ $ATTEMPT -gt 1 ]; then + echo "Resetting app state..." + adb shell am force-stop chat.rocket.reactnative || true + adb shell pm clear chat.rocket.reactnative || echo "Could not clear app data" + sleep 2 + fi echo "Starting screen recording..."
📜 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 (48)
.github/workflows/build-pr.yml(2 hunks).github/workflows/e2e-build-android.yml(2 hunks).github/workflows/maestro-android.yml(3 hunks).maestro/tests/accessibilityAndAppearance/ToastsAndDialogs.yml(1 hunks).maestro/tests/assorted/accessibility-and-appearance.yaml(1 hunks).maestro/tests/assorted/broadcast.yaml(1 hunks).maestro/tests/assorted/change-avatar.yaml(1 hunks).maestro/tests/assorted/changeserver.yaml(1 hunks).maestro/tests/assorted/deeplink.yaml(1 hunks).maestro/tests/assorted/delete-server.yaml(1 hunks).maestro/tests/assorted/display-perf.yaml(1 hunks).maestro/tests/assorted/e2e-encryption.yaml(1 hunks).maestro/tests/assorted/i18n.yaml(1 hunks).maestro/tests/assorted/in-app-notification.yaml(1 hunks).maestro/tests/assorted/join-from-directory.yaml(2 hunks).maestro/tests/assorted/join-protected-room.yaml(1 hunks).maestro/tests/assorted/join-public-room.yaml(1 hunks).maestro/tests/assorted/profile.yaml(4 hunks).maestro/tests/assorted/setting.yaml(2 hunks).maestro/tests/assorted/status.yaml(1 hunks).maestro/tests/assorted/user-preferences.yaml(1 hunks).maestro/tests/onboarding/change-password.yaml(1 hunks).maestro/tests/onboarding/forgot-password.yaml(1 hunks).maestro/tests/onboarding/legal.yaml(1 hunks).maestro/tests/onboarding/login/invalid-credentials.yaml(1 hunks).maestro/tests/onboarding/login/login.yaml(1 hunks).maestro/tests/onboarding/register/create-account.yaml(1 hunks).maestro/tests/onboarding/register/email-used.yaml(1 hunks).maestro/tests/onboarding/register/username-used.yaml(1 hunks).maestro/tests/onboarding/roomslist.yaml(1 hunks).maestro/tests/onboarding/server-history.yaml(1 hunks).maestro/tests/onboarding/workspace/invalid-workspace.yaml(1 hunks).maestro/tests/onboarding/workspace/valid-workspace.yaml(1 hunks).maestro/tests/room/create-dm-group.yaml(1 hunks).maestro/tests/room/create-room.yaml(1 hunks).maestro/tests/room/discussion.yaml(3 hunks).maestro/tests/room/ignoreuser.yaml(3 hunks).maestro/tests/room/jump-to-message.yaml(1 hunks).maestro/tests/room/mark-as-unread.yaml(1 hunks).maestro/tests/room/room-actions.yaml(1 hunks).maestro/tests/room/room-info.yaml(1 hunks).maestro/tests/room/room.yaml(1 hunks).maestro/tests/room/share-message.yaml(1 hunks).maestro/tests/room/threads.yaml(1 hunks).maestro/tests/room/utils/clear-cache.yaml(1 hunks).maestro/tests/teams/convert-team.yaml(1 hunks).maestro/tests/teams/create-team.yaml(1 hunks).maestro/tests/teams/team.yaml(1 hunks)
🔇 Additional comments (59)
.maestro/tests/assorted/accessibility-and-appearance.yaml (1)
7-8: LGTM! Test tagging enables parallel execution.The addition of the
test-5tag correctly implements the test sharding strategy described in the PR objectives, enabling this test to run in parallel with other tagged tests and contributing to the reported ~83% reduction in test execution time..maestro/tests/accessibilityAndAppearance/ToastsAndDialogs.yml (1)
7-8: LGTM! Tag addition enables test sharding.The addition of the
test-13tag is correct and aligns with the PR objectives to enable parallel test execution through GitHub Actions matrix strategy.Note: The AI summary incorrectly states that "Removed the launch-app flow step" — line 11 shows
- runFlow: ../../helpers/launch-app.yamlis still present in the code..maestro/tests/teams/create-team.yaml (1)
7-8: LGTM! Tag added for test parallelization.The
test-4tag has been correctly added to enable parallel test execution via GitHub Actions matrix, consistent with the PR's objective to reduce E2E test time by ~83%..maestro/tests/teams/convert-team.yaml (1)
7-8: LGTM! Tag added for test parallelization.The
test-4tag has been correctly added, consistent with the parallel execution strategy across team-related tests..maestro/tests/teams/team.yaml (1)
7-8: LGTM! Tag added for test parallelization.The
test-4tag has been correctly added, completing the consistent tagging of all three team-related test flows for parallel execution..maestro/tests/room/room-info.yaml (1)
7-8: LGTM! Metadata tag added for test sharding.The addition of the
test-12tag enables this test to be grouped and executed in parallel as part of the GitHub Actions matrix strategy described in the PR objectives..maestro/tests/assorted/user-preferences.yaml (1)
7-8: LGTM! Metadata tag added for test sharding.The
test-8tag enables parallel execution of this test in the GitHub Actions matrix, consistent with other assorted test files in this PR..maestro/tests/onboarding/register/email-used.yaml (1)
6-7: LGTM! Metadata tag added for test sharding.The
test-2tag enables this onboarding test to run in parallel with other onboarding flows as part of the GitHub Actions matrix strategy..maestro/tests/room/create-dm-group.yaml (1)
7-8: LGTM! Metadata tag added for test sharding.The
test-10tag groups this test with other room-related flows for parallel execution, contributing to the ~83% reduction in test execution time noted in the PR objectives..maestro/tests/assorted/changeserver.yaml (1)
7-8: LGTM! Metadata tag added for test sharding.The
test-5tag enables parallel execution of this test alongside other assorted flows in the GitHub Actions matrix..maestro/tests/assorted/in-app-notification.yaml (1)
7-8: LGTM! Tag metadata added for test sharding.The addition of the
test-7tag enables this test to be selected for parallel execution as part of the PR's test sharding strategy..maestro/tests/assorted/deeplink.yaml (1)
7-8: LGTM! Tag metadata added for test sharding.The addition of the
test-6tag supports the PR's parallel test execution strategy through tag-based sharding..maestro/tests/onboarding/login/login.yaml (1)
8-9: LGTM! Tag metadata added for test sharding.The addition of the
test-2tag supports parallel execution. This file uses top-level tag placement (afteronFlowComplete), which is another placement variant seen across the PR..maestro/tests/room/jump-to-message.yaml (1)
7-8: LGTM! Tag metadata added for test sharding.The addition of the
test-11tag enables this test to participate in the parallel execution strategy introduced in this PR..maestro/tests/room/room-actions.yaml (1)
7-8: LGTM! Test metadata added for sharding.The
test-11tag enables this test to be grouped and executed in parallel as part of the PR's E2E optimization strategy..maestro/tests/assorted/status.yaml (1)
7-8: LGTM! Test metadata added for sharding.The
test-8tag supports parallel test execution as described in the PR objectives..maestro/tests/assorted/delete-server.yaml (1)
7-8: LGTM! Test metadata added for sharding.The
test-6tag enables parallel execution as part of the E2E optimization effort..maestro/tests/assorted/setting.yaml (2)
7-8: LGTM! Test metadata added for sharding.The
test-8tag supports parallel test execution as described in the PR objectives.
110-112: LGTM! Retry flag added to handle flakiness.Adding
retryTapIfNoChange: truefor the clear-cache button tap is appropriate, as this action may occasionally fail or be slow to register. This aligns with the PR's goal of implementing retry logic for flaky tests..maestro/tests/onboarding/roomslist.yaml (1)
7-8: LGTM! Test metadata added for sharding.The
test-1tag enables this test to be grouped and executed in parallel as part of the E2E optimization strategy..maestro/tests/room/room.yaml (1)
8-9: LGTM! Test sharding tag added for parallel execution.The addition of the
test-12tag enables this test to be grouped and executed in parallel as part of the E2E test optimization described in the PR objectives..maestro/tests/assorted/e2e-encryption.yaml (1)
7-8: LGTM! Test sharding tag added for parallel execution.The addition of the
test-9tag enables this E2E encryption test to be grouped and executed in parallel, contributing to the ~83% reduction in test execution time mentioned in the PR objectives..maestro/tests/room/mark-as-unread.yaml (1)
7-8: LGTM! Test sharding tag added for parallel execution.The addition of the
test-11tag enables this test to be grouped and executed in parallel with other room tests sharing the same tag..maestro/tests/assorted/broadcast.yaml (1)
7-8: LGTM! Test sharding tag added for parallel execution.The addition of the
test-5tag enables this broadcast test to be grouped and executed in parallel as part of the E2E test optimization..maestro/tests/onboarding/workspace/valid-workspace.yaml (1)
5-6: LGTM! Test sharding tag added for parallel execution.The addition of the
test-3tag enables this workspace test to be grouped and executed in parallel. Note that this tag is placed underonFlowStart(consistent with onboarding tests), while most other tests place tags underonFlowComplete..maestro/tests/onboarding/register/create-account.yaml (1)
6-7: LGTM! Metadata tag added for test sharding.The
test-2tag enables parallel test execution without altering the flow logic..maestro/tests/onboarding/register/username-used.yaml (1)
6-7: LGTM! Metadata tag added for test sharding.The
test-2tag enables parallel test execution without altering the flow logic..maestro/tests/assorted/join-public-room.yaml (1)
7-8: LGTM! Metadata tag added for test sharding.The
test-7tag enables parallel test execution without altering the flow logic..maestro/tests/assorted/i18n.yaml (1)
7-8: LGTM! Metadata tag added for test sharding.The
test-6tag enables parallel test execution without altering the flow logic..maestro/tests/assorted/change-avatar.yaml (1)
7-8: LGTM! Metadata tag added for test sharding.The
test-5tag enables parallel test execution without altering the flow logic..maestro/tests/room/create-room.yaml (1)
7-8: LGTM! Test sharding tag added successfully.The
test-10tag enables parallel execution of this test suite in CI. The placement underonFlowCompleteis appropriate for organizing test metadata alongside cleanup logic..maestro/tests/onboarding/workspace/invalid-workspace.yaml (1)
3-4: LGTM! Test sharding tag added successfully.The
test-3tag enables parallel execution of this test. The top-level placement is valid for Maestro test configuration..maestro/tests/onboarding/change-password.yaml (1)
8-9: LGTM! Test sharding tag added successfully.The
test-1tag enables parallel execution of this test suite. The placement underonFlowEndalongside cleanup logic is appropriate..maestro/tests/onboarding/login/invalid-credentials.yaml (1)
7-8: LGTM! Test sharding tag added successfully.The
test-2tag enables parallel execution of this test suite. The placement underonFlowEndis consistent with other onboarding tests..maestro/tests/assorted/join-protected-room.yaml (1)
7-8: LGTM! Test sharding tag added successfully.The
test-7tag enables parallel execution of this test suite. The placement underonFlowCompletealongside cleanup logic is appropriate and consistent with the sharding strategy..maestro/tests/room/utils/clear-cache.yaml (1)
31-31: LGTM! Improves test resilience.Adding
retryTapIfNoChange: truehelps handle flaky tap interactions on the clear cache button, aligning with the PR's goal of improving test stability through retry logic..maestro/tests/assorted/display-perf.yaml (1)
7-8: LGTM! Test tagging for parallel execution.Adding the
test-6tag enables shard-based parallel test execution, supporting the PR's goal of reducing test time from ~3 hours to ~30 minutes..maestro/tests/onboarding/forgot-password.yaml (1)
6-7: LGTM! Test tagging for parallel execution.Adding the
test-1tag enables this onboarding flow to be included in the parallel test execution matrix, consistent with the PR's performance improvement goals..maestro/tests/room/ignoreuser.yaml (3)
5-6: LGTM! Test tagging for parallel execution.Adding the
test-11tag enables this room flow to participate in the parallel test matrix.
195-195: LGTM! Improves test resilience for form submission.Adding
retryTapIfNoChange: trueto the report submission tap handles cases where the submit button might not respond immediately, improving test stability for this user reporting flow.
242-242: LGTM! Consistent retry logic for form submission.This second instance of
retryTapIfNoChange: trueon the report submit button maintains consistency with the earlier report flow (line 195) and improves reliability when reporting from a channel context..maestro/tests/room/share-message.yaml (1)
5-6: LGTM! Test tagging for parallel execution.Adding the
test-12tag enables this message sharing flow to be part of the parallel test execution strategy, contributing to the significant test time reduction described in the PR..maestro/tests/room/threads.yaml (1)
7-8: LGTM! Tag enables test sharding.The
test-12tag addition supports parallel execution via GitHub Actions matrix without modifying test logic..maestro/tests/room/discussion.yaml (3)
7-8: LGTM! Tag enables test sharding.The
test-10tag addition supports parallel execution without modifying test behavior.
192-193: Good addition to handle flaky UI interactions.The
retryTapIfNoChange: trueflag improves test stability by retrying the tap oncreate-discussion-submitif the UI state doesn't change, which aligns with the PR's goal of handling flaky tests.
206-208: Good addition to handle flaky UI interactions.The
retryTapIfNoChange: trueflag on theroom-headertap improves test reliability by retrying if no state change occurs..maestro/tests/assorted/join-from-directory.yaml (2)
7-8: LGTM! Tag enables test sharding.The
test-7tag addition supports parallel execution without modifying test logic.
107-109: Verify unconditional debouncing on all platforms
No other platform-specific debounce patterns were found; confirm these eraseText/inputText steps have been tested on Android for timing and reliability..maestro/tests/onboarding/server-history.yaml (1)
7-8: LGTM! Tag enables test sharding.The
test-3tag addition supports parallel execution via GitHub Actions matrix without modifying test behavior..maestro/tests/assorted/profile.yaml (4)
7-8: LGTM! Tag enables test sharding.The
test-8tag addition supports parallel execution without modifying test logic.
76-79: Good improvement for test reliability.Changing from
assertVisibletoscrollUntilVisibleensures the password change button is scrolled into view before interacting with it, which improves test stability when the element is below the fold.
159-160: Good addition to handle animation timing.Adding
waitForAnimationToEndbefore checking for the password sheet input ensures the UI animation completes, reducing flakiness in CI environments.
204-205: Good addition to handle flaky UI interactions.The
retryTapIfNoChange: trueflag on the password change submit button improves test reliability by retrying the tap if no state change occurs, which is consistent with the PR's retry logic for flaky tests..github/workflows/e2e-build-android.yml (3)
31-38: Good caching strategy for Gradle dependencies.The cache paths and key composition look appropriate. The
~/.gradlepath will cache downloaded dependencies and daemon state, which should significantly reduce build times.
76-76: Architecture restriction aligns with E2E testing needs.Restricting to
x86_64is appropriate since the E2E tests run on an x86_64 emulator. This eliminates unnecessary compilation for other architectures (arm64-v8a, armeabi-v7a, x86) and contributes significantly to the build time reduction reported in the PR.
40-49: Approve Android build cache configuration.Configured cache paths (
android/.gradle,android/app/build,android/app/.cxx) cover all existing build artifacts; there is noandroid/builddirectory to include..github/workflows/maestro-android.yml (3)
5-8: Shard input enables parallel execution.The new
shardinput is appropriately typed and marked as required, enabling the workflow to be invoked in parallel for different test shards as described in the PR objectives.
102-102: Timeout prevents indefinite hangs.The 60-minute timeout is a sensible safeguard for E2E tests with retry logic, preventing hung tests from blocking the CI pipeline indefinitely.
116-130: Artifact uploads properly handle sharding and retries.The artifact naming includes the shard number to prevent collisions in parallel runs, and the video upload pattern (
test_run_${{ inputs.shard }}_attempt_*.mp4) correctly captures recordings from all retry attempts. Theif: always()condition ensures artifacts are uploaded even on failure.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
app/containers/Toast.tsx(1 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). (4)
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: E2E Build Android / android-build
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
2 participants
Proposed changes
This pull request improves the Android E2E build by caching the build and using the x86_64 architecture only. I have also added 13 different tags and used an action matrix to run the tests in parallel, which speeds up the test time. Additionally, for flaky tests, I have added retry logic that will rerun a test if it fails, with a maximum of 3 attempts.
Working Action: https://github.com/RocketChat/Rocket.Chat.ReactNative/actions/runs/18139603093
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Tests
Bug Fixes
Chores