[WIP] test: add e2e testing infrastructure (iOS + Android)#2777
Draft
testableapple wants to merge 13 commits into
Draft
[WIP] test: add e2e testing infrastructure (iOS + Android)#2777testableapple wants to merge 13 commits into
testableapple wants to merge 13 commits into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces a complete Patrol-based Flutter E2E testing infrastructure for ChangesPatrol E2E Testing Infrastructure
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over GHActions,Fastlane: CI trigger (on-demand or nightly)
end
participant GHActions
participant Fastlane
participant MockDriver as Mock Driver
participant MockServer as Mock Server (per-test)
participant PatrolTest as patrol test
participant Device as Android Emulator / iOS Simulator
participant AllureTestOps
GHActions->>Fastlane: run_e2e_test(device, mock_server_branch)
Fastlane->>MockDriver: start_mock_server (clone + bundle exec driver)
Fastlane->>PatrolTest: patrol test --device
PatrolTest->>Device: instrument app
Device->>MockDriver: MockServer.start(testName)
MockDriver->>MockServer: fork per-test server
MockServer-->>Device: url + wsUrl
Device->>Device: authController.debugConnectionOverride = override
Device->>MockServer: HTTP/WS test traffic
Device-->>PatrolTest: test result
PatrolTest->>Device: print ALLURE-RESULT:: chunks via logcat/simctl
Fastlane->>Device: collect_allure_results (read log chunks → allure-results/*.json)
Fastlane->>AllureTestOps: allurectl upload
Fastlane->>MockDriver: stop_mock_server POST /stop
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
testableapple
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
.github/workflows/e2e_test.yml-108-111 (1)
108-111:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
timeout-minutesto the iOS e2e step.The iOS test run lacks an explicit timeout, similar to the Android job.
🔧 Proposed fix
- name: Run e2e (iOS simulator) + timeout-minutes: 60 run: | cd sample_app/ios bundle exec fastlane run_e2e_test device:${{ env.device_id }} mock_server_branch:main🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e_test.yml around lines 108 - 111, The "Run e2e (iOS simulator)" step in the iOS e2e workflow is missing an explicit timeout configuration. Add a `timeout-minutes` property to this step to set a maximum execution time for the iOS simulator test run, similar to what exists in the Android job to prevent the workflow from running indefinitely in case of failures or hangs..github/workflows/e2e_test_cron.yml-135-136 (1)
135-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
timeout-minutesto the iOS e2e step for consistency with Android.The Android job has
timeout-minutes: 90on the emulator runner, but the iOS job lacks an explicit timeout.🔧 Proposed fix
- name: Run e2e (iOS simulator) + timeout-minutes: 90 run: cd sample_app/ios && bundle exec fastlane run_e2e_test device:${{ env.device_id }} mock_server_branch:${{ env.mock_server_branch }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e_test_cron.yml around lines 135 - 136, The iOS e2e test step named "Run e2e (iOS simulator)" is missing the timeout-minutes configuration that the Android job has. Add a timeout-minutes field set to 90 to this step to maintain consistency with the Android emulator runner's timeout configuration. This should be added as a step-level property at the same indentation level as the name and run fields..github/workflows/e2e_test.yml-53-61 (1)
53-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
timeout-minutesto the Android emulator step.Unlike the nightly workflow which specifies
timeout-minutes: 90, this step lacks a timeout. If tests hang, the job will run until the workflow's default timeout (6 hours), wasting compute and blocking the concurrency group.🔧 Proposed fix
- name: Run e2e (Android emulator) uses: reactivecircus/android-emulator-runner@v2 + timeout-minutes: 60 with: api-level: 34🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e_test.yml around lines 53 - 61, The "Run e2e (Android emulator)" step that uses reactivecircus/android-emulator-runner@v2 is missing a timeout-minutes configuration. Add timeout-minutes: 90 as a property at the same indentation level as the uses and with properties to prevent tests from hanging indefinitely and consuming resources until the default 6-hour workflow timeout is reached.sample_app/integration_test/pages/message_list_page.dart-4-6 (1)
4-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd doc comments for
MessageListPagepublic API.
MessageListPage(and exposedcomposer) is public and should be documented.As per coding guidelines: “All public APIs must have doc comments (public_member_api_docs)”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/pages/message_list_page.dart` around lines 4 - 6, The MessageListPage class and its public static constant composer are missing documentation comments required by the coding guidelines. Add doc comments (starting with ///) to both the MessageListPage abstract final class and the static const composer field to document their purpose and usage, ensuring all public APIs are properly documented.Source: Coding guidelines
sample_app/integration_test/mock_server/data_types.dart-1-25 (1)
1-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd doc comments for exported enums and constants.
AttachmentType,ReactionType,MessageDeliveryStatus, andforbiddenWordare public APIs and need Dart doc comments.As per coding guidelines: “All public APIs must have doc comments (public_member_api_docs)”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/mock_server/data_types.dart` around lines 1 - 25, Add Dart doc comments to the public APIs in this file to comply with the public_member_api_docs guideline. For the three enums AttachmentType, ReactionType, and MessageDeliveryStatus, add doc comments above their enum declarations that describe their purpose and usage. Similarly, add a doc comment above the forbiddenWord constant that explains what it represents. Use the standard Dart doc comment format (///) for all public members to ensure they are properly documented.Source: Coding guidelines
sample_app/integration_test/robots/backend_robot.dart-1-54 (1)
1-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply package imports and doc comments for public robot API.
This file uses a relative import and exposes public APIs without Dart doc comments.
As per coding guidelines: “Always use package imports instead of relative imports (always_use_package_imports)” and “All public APIs must have doc comments (public_member_api_docs)”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/robots/backend_robot.dart` around lines 1 - 54, Replace the relative import statement with a package import for the mock_server module in the BackendRobot file. Additionally, add Dart doc comments (using ///) to all public APIs including the BackendRobot class definition and all public methods: generateChannels, failNewMessages, freezeNewMessages, revokeToken, invalidateToken, invalidateTokenDate, invalidateTokenSignature, and breakTokenGeneration. Each doc comment should clearly describe the purpose and behavior of the method or class.Source: Coding guidelines
sample_app/integration_test/mock_server/mock_server.dart-7-87 (1)
7-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the public
MockServerAPI.
MockServerand its public members are exported test infrastructure APIs and should have Dart doc comments for maintainability.As per coding guidelines: “All public APIs must have doc comments (public_member_api_docs)”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/mock_server/mock_server.dart` around lines 7 - 87, Add Dart doc comments to all public members of the MockServer class to comply with the public_member_api_docs guideline. Document the MockServer class itself, and all public methods and properties: the url and wsUrl fields, the start() static factory method, and the instance methods stop(), post(), get(), and waitUntilReady(). Each comment should explain the purpose and usage of the respective public API member.Source: Coding guidelines
sample_app/integration_test/robots/participant_robot.dart-1-176 (1)
1-176:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwitch to package imports and document the public robot surface.
This file uses relative imports and exports a large public API (
ParticipantRobotmethods) without doc comments.As per coding guidelines: “Always use package imports instead of relative imports (always_use_package_imports)” and “All public APIs must have doc comments (public_member_api_docs)”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/robots/participant_robot.dart` around lines 1 - 176, The ParticipantRobot class in participant_robot.dart has two issues: the imports at the top use relative paths instead of package imports, and all public methods lack documentation comments. Replace the relative imports (using ../ paths) with package imports using the package: prefix for the data_types and mock_server modules. Then add doc comments to the ParticipantRobot class and all its public methods including startTyping, stopTyping, sendMessage, editMessage, deleteMessage, quoteMessage, uploadGiphy, pinMessage, unpinMessage, uploadAttachment, addReaction, deleteReaction, and their related variants to document their purpose and behavior.Source: Coding guidelines
sample_app/integration_test/support/stream_test_env.dart-5-8 (1)
5-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse package imports and add doc comments for public test env API.
This file uses relative imports and exposes public APIs (
StreamTestEnv,setUp,tearDown) without doc comments.As per coding guidelines: “Always use package imports instead of relative imports (always_use_package_imports)” and “All public APIs must have doc comments (public_member_api_docs)”.
Also applies to: 10-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/support/stream_test_env.dart` around lines 5 - 8, Replace all relative imports in this file with package imports by converting imports like '../mock_server/mock_server.dart', '../robots/backend_robot.dart', '../robots/participant_robot.dart', and '../robots/user_robot.dart' to use the package: prefix (e.g., 'package:sample_app/...'). Additionally, add documentation comments to all public APIs exposed by this file, specifically the StreamTestEnv class, setUp function, and tearDown function, ensuring each public member has a clear doc comment explaining its purpose and usage.Source: Coding guidelines
sample_app/integration_test/support/stream_test.dart-9-13 (1)
9-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the public
streamTestwrapper.
streamTestis public and should have a doc comment describing intended usage and behavior.As per coding guidelines:
**/*.dart: All public APIs must have doc comments (public_member_api_docs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/support/stream_test.dart` around lines 9 - 13, The public streamTest function lacks a doc comment which is required by the public_member_api_docs guideline. Add a documentation comment above the streamTest function that clearly describes its purpose as a test wrapper, documents the parameters (description, callback, and allureId), and explains its intended usage and behavior for stream-based integration testing.Source: Coding guidelines
sample_app/integration_test/PLAN.md-233-237 (1)
233-237:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpecify languages for fenced code blocks.
Two fenced blocks are missing language identifiers, triggering MD040. Add a language like
text,dart,kotlin, orrubyas appropriate.Also applies to: 275-321
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/PLAN.md` around lines 233 - 237, The fenced code blocks in PLAN.md at lines 233-237 and 275-321 are missing language identifiers after the opening triple backticks, which triggers the MD040 linting rule. Add an appropriate language identifier (such as `text`, `ruby`, or `dart`) immediately after the triple backticks for each fenced code block to specify the content type and resolve the linting violation.Source: Linters/SAST tools
sample_app/integration_test/allure/allure.dart-7-140 (1)
7-140:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd doc comments for the exported Allure API.
This file exposes public members (
allureResultMarker,AllureStatus,Allure, and public methods) without API docs, which will trippublic_member_api_docsin linted runs.As per coding guidelines:
**/*.dart: All public APIs must have doc comments (public_member_api_docs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/allure/allure.dart` around lines 7 - 140, Add documentation comments (/// style) to all public members in the file to satisfy the public_member_api_docs lint rule. This includes the allureResultMarker constant, the AllureStatus enum and its enum values, the Allure class, and its public methods (startTest, step, and stopTest). Each doc comment should clearly describe the purpose and usage of the member, ensuring that all exported APIs have proper documentation.Source: Coding guidelines
sample_app/integration_test/support/stream_test.dart-7-7 (1)
7-7:⚠️ Potential issue | 🟡 MinorSwitch this relative import to a package import.
Use
package:sample_app/integration_test/allure/allure.dartinstead of the relative path. Relative imports make refactoring across directories more fragile and violate the always_use_package_imports guideline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/support/stream_test.dart` at line 7, Replace the relative import statement in stream_test.dart with a package import to improve maintainability and follow the always_use_package_imports guideline. Change the import from using the relative path '../allure/allure.dart' to use the full package path format 'package:sample_app/integration_test/allure/allure.dart' instead. This makes the codebase more resilient to directory refactoring and improves code clarity.Source: Coding guidelines
🧹 Nitpick comments (7)
.github/workflows/e2e_test_cron.yml (1)
162-168: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffConsider replacing archived Slack action.
The
8398a7/action-slack@v3repository is archived. While it still functions, consider migrating to an actively maintained alternative likeslackapi/slack-github-actionfor long-term support.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e_test_cron.yml around lines 162 - 168, Replace the archived `8398a7/action-slack@v3` action with the actively maintained `slackapi/slack-github-action`. Update the uses statement to reference the new action, and adjust the input parameters accordingly since the new action has a different API - the text and fields parameters should be replaced with the appropriate payload structure expected by slackapi/slack-github-action, and ensure the SLACK_WEBHOOK_URL environment variable is properly passed to the new action's configuration.Source: Linters/SAST tools
sample_app/integration_test/support/step.dart (1)
1-4: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse package import and document the public helper API.
This helper is fine functionally, but it violates the Dart style rules configured for this repo (relative import + missing doc comment on a public API).
Suggested patch
-import '../allure/allure.dart'; +import 'package:sample_app/integration_test/allure/allure.dart'; +/// Runs an Allure step with the provided [description]. Future<T> step<T>(String description, Future<T> Function() body) => Allure.instance.step(description, body);As per coding guidelines,
**/*.dartmust use package imports and all public APIs must have doc comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/support/step.dart` around lines 1 - 4, The import statement for the allure module uses a relative import path, but the repo requires package imports for all Dart files. Additionally, the public step function is missing a documentation comment as required by the Dart style guidelines. Convert the relative import statement to use the package import syntax, and add a doc comment above the step function definition that documents its purpose, parameters, and return type to comply with the coding standards for public APIs.Source: Coding guidelines
sample_app/integration_test/pages/channel_list_page.dart (1)
3-5: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDocument public page-object API members.
ChannelListPageand its publicchannelTilemember should have doc comments to satisfy the repo Dart API documentation rule.As per coding guidelines, all public APIs in
**/*.dartmust have doc comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/pages/channel_list_page.dart` around lines 3 - 5, Add doc comments to the public API members in the ChannelListPage class to comply with the Dart documentation requirement. The ChannelListPage class itself needs a doc comment explaining its purpose, and the static channelTile constant member needs a doc comment describing what it represents. Place the doc comments immediately before each member declaration using the triple-slash (///) syntax to ensure the repository's API documentation generation includes these definitions.Source: Coding guidelines
sample_app/integration_test/robots/user_robot.dart (1)
3-33: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReplace relative imports and add docs for the public robot API.
The robot implementation looks good, but this file currently violates Dart repo rules on package imports and public API docs.
Suggested import direction
-import '../pages/channel_list_page.dart'; -import '../pages/message_list_page.dart'; -import '../support/predefined_users.dart'; +import 'package:sample_app/integration_test/pages/channel_list_page.dart'; +import 'package:sample_app/integration_test/pages/message_list_page.dart'; +import 'package:sample_app/integration_test/support/predefined_users.dart';As per coding guidelines,
**/*.dartmust use package imports and all public APIs must have doc comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/robots/user_robot.dart` around lines 3 - 33, Replace all relative imports in the UserRobot class file with absolute package imports (convert imports like '../pages/channel_list_page.dart' to use the package: syntax). Additionally, add documentation comments using the /// syntax to the public UserRobot class and all its public methods including login, openChannel, and sendMessage to document their purpose, parameters, and return values according to Dart documentation guidelines.Source: Coding guidelines
sample_app/integration_test/support/predefined_users.dart (1)
3-26: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd public API docs and split the token literal to satisfy max line length.
This file exposes public APIs without doc comments, and the JWT line likely exceeds the 120-char limit. Please document
UserCredentials/PredefinedUsersand break long literals across adjacent string segments.As per coding guidelines, public APIs in
**/*.dartrequire doc comments and lines in**/*.{dart,dart.bak}must not exceed 120 characters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/support/predefined_users.dart` around lines 3 - 26, Add doc comments to the public classes UserCredentials and PredefinedUsers to document their purpose and usage, and split the long JWT token literal in the qaTest1 constant across multiple adjacent string segments to ensure no line exceeds the 120 character limit as per coding guidelines.Source: Coding guidelines
sample_app/integration_test/robots/user_robot_message_list_asserts.dart (1)
1-7: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMake the extension file lint-compliant (package import + API docs).
Please switch to package import and add doc comments for the public extension and assertion method.
As per coding guidelines, Dart files must use package imports and public APIs must have doc comments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/robots/user_robot_message_list_asserts.dart` around lines 1 - 7, The file uses a relative import statement instead of a package import, and the public extension UserRobotMessageListAsserts and its public method assertMessage lack doc comments. Replace the relative import with a package import statement that includes the appropriate package path, and add doc comments (using ///) to both the UserRobotMessageListAsserts extension and the assertMessage method to document their purpose and usage.Source: Coding guidelines
sample_app/integration_test/message_list_test.dart (1)
3-6: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse package imports in the integration test file.
Please replace these relative imports with
package:sample_app/...imports to satisfy the Dart import policy.As per coding guidelines,
**/*.dartshould always use package imports instead of relative imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample_app/integration_test/message_list_test.dart` around lines 3 - 6, Replace all four relative imports at the top of the message_list_test.dart file with package imports. Convert the imports of user_robot_message_list_asserts.dart, step.dart, stream_test.dart, and stream_test_env.dart from relative paths (using ./) to package imports using the format package:sample_app/path/to/file.dart. Ensure the paths after package:sample_app/ reflect the correct location of these imported files within the project structure.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e_test_cron.yml:
- Around line 62-65: The current workflow creates a separate Allure launch for
each matrix cell (3 Android variants and 2 iOS variants), but they should all
upload to a single shared launch. Create a new `setup` job that runs once before
the matrix jobs to generate a launch_id, then extract that launch_id as an
output so the android and ios jobs can consume it via `needs: setup`. Remove the
"Create Allure launch" step from both the android and ios job definitions, and
update their "Upload Allure results" steps to use the shared launch_id from the
setup job output instead of creating new launches in each matrix cell.
- Around line 1-20: The E2E Tests Nightly workflow in the e2e_test_cron.yml file
is missing explicit minimal permissions at the workflow level. Add a
workflow-level permissions block to the file (after the concurrency section or
at the top level, similar to how other workflow files are structured) and define
explicit minimal permissions that match what is configured in the corresponding
on-demand workflow. This ensures the workflow operates with the principle of
least privilege.
In @.github/workflows/e2e_test.yml:
- Around line 1-16: Add a workflow-level permissions block to the e2e workflow
to restrict permissions to only what is needed. The permissions block should be
added after the 'on:' section (after the pull_request trigger configuration) and
before the 'concurrency:' section. Since this workflow only needs to checkout
code and upload artifacts, add a permissions block that specifies contents: read
for code checkout, and any other minimal permissions required for the specific
operations the workflow performs. This replaces the default overly-broad
permissions with an explicitly minimal set.
In `@sample_app/Allurefile`:
- Around line 14-18: The adb logcat command in the Android branch does not
utilize the options[:device] parameter, which causes it to retrieve logs from
the default device when multiple devices are connected. Modify the adb logcat -d
command to include the device specifier by passing options[:device] to the adb
command using the -s flag (adb -s <device> syntax) so that it respects the
device parameter consistently with the iOS branch that already uses
options[:device] for simctl.
In `@sample_app/Fastfile`:
- Line 59: The adb logcat clear command in the current implementation does not
specify the target device when options[:device] is provided. Modify the sh('adb
logcat -c') command to include the device specification flag (such as -s with
the device name) when options[:device] is present, making it consistent with the
device targeting pattern used in the collect_allure_results function. Ensure the
device parameter is only appended to the command when it is available in the
options hash.
In `@sample_app/integration_test/mock_server/mock_server.dart`:
- Around line 18-24: The MockServer.start() method makes an HTTP call via _get()
to the mock driver without an explicit timeout, which can cause indefinite hangs
if the driver is unresponsive. Add explicit request timeouts to the _get(),
_statusCode(), and post() helper methods to prevent blocking indefinitely. Each
HTTP request should include a timeout duration parameter (typically in the range
of seconds) to ensure the application can fail fast and proceed to error
handling or retry logic when the driver is unresponsive.
In `@sample_app/integration_test/robots/backend_robot.dart`:
- Around line 15-26: The messagesText and repliesText parameters are being
directly concatenated into the query string without URL-encoding, which will
cause the request parameters to be corrupted if these values contain reserved
characters like ampersands, question marks, spaces, or equals signs. Apply
proper URL-encoding to the messagesText and repliesText values before they are
inserted into the messagesTextParam and repliesTextParam string construction.
Use Dart's URI encoding utility to ensure these parameters are safely encoded
for the query string in the _mockServer.post() call.
In `@sample_app/integration_test/support/predefined_users.dart`:
- Around line 18-23: Remove the hardcoded token from the `qaTest1`
UserCredentials constant in the `PredefinedUsers` class. Instead of embedding
the JWT directly in the fixture, implement a mechanism to generate short-lived
test tokens dynamically from the mock server on each test run. Alternatively,
inject the token from secure CI secrets at runtime. Update the `UserCredentials`
class or the test initialization to accept dynamically generated tokens rather
than relying on static fixture values.
In `@sample_app/integration_test/support/stream_test_env.dart`:
- Around line 30-33: The tearDown() method in the stream test environment class
does not handle exceptions properly - if authController.debugReset() throws an
error, the mockServer.stop() call will be skipped, leaving the server in an
unclean state for subsequent tests. Wrap the teardown logic in a try/finally
block where authController.debugReset() is in the try block and
mockServer.stop() is in the finally block to ensure the mock server is always
stopped regardless of whether the auth controller reset succeeds or fails.
In `@sample_app/ios/Runner.xcodeproj/project.pbxproj`:
- Line 83: Replace the hardcoded SDK version path in the Foundation.framework
file reference (A6B213AE2D543B7BADA38E33). Instead of using the explicit path
containing `iPhoneOS18.0.sdk`, update the path attribute to use a dynamic
SDK-relative reference such as using the `SDKROOT` variable or a relative path
pattern like `System/Library/Frameworks/Foundation.framework` that will work
across different Xcode versions and CI environments. Apply the same fix to other
SDK version-pinned framework references mentioned at lines 103-105.
---
Minor comments:
In @.github/workflows/e2e_test_cron.yml:
- Around line 135-136: The iOS e2e test step named "Run e2e (iOS simulator)" is
missing the timeout-minutes configuration that the Android job has. Add a
timeout-minutes field set to 90 to this step to maintain consistency with the
Android emulator runner's timeout configuration. This should be added as a
step-level property at the same indentation level as the name and run fields.
In @.github/workflows/e2e_test.yml:
- Around line 108-111: The "Run e2e (iOS simulator)" step in the iOS e2e
workflow is missing an explicit timeout configuration. Add a `timeout-minutes`
property to this step to set a maximum execution time for the iOS simulator test
run, similar to what exists in the Android job to prevent the workflow from
running indefinitely in case of failures or hangs.
- Around line 53-61: The "Run e2e (Android emulator)" step that uses
reactivecircus/android-emulator-runner@v2 is missing a timeout-minutes
configuration. Add timeout-minutes: 90 as a property at the same indentation
level as the uses and with properties to prevent tests from hanging indefinitely
and consuming resources until the default 6-hour workflow timeout is reached.
In `@sample_app/integration_test/allure/allure.dart`:
- Around line 7-140: Add documentation comments (/// style) to all public
members in the file to satisfy the public_member_api_docs lint rule. This
includes the allureResultMarker constant, the AllureStatus enum and its enum
values, the Allure class, and its public methods (startTest, step, and
stopTest). Each doc comment should clearly describe the purpose and usage of the
member, ensuring that all exported APIs have proper documentation.
In `@sample_app/integration_test/mock_server/data_types.dart`:
- Around line 1-25: Add Dart doc comments to the public APIs in this file to
comply with the public_member_api_docs guideline. For the three enums
AttachmentType, ReactionType, and MessageDeliveryStatus, add doc comments above
their enum declarations that describe their purpose and usage. Similarly, add a
doc comment above the forbiddenWord constant that explains what it represents.
Use the standard Dart doc comment format (///) for all public members to ensure
they are properly documented.
In `@sample_app/integration_test/mock_server/mock_server.dart`:
- Around line 7-87: Add Dart doc comments to all public members of the
MockServer class to comply with the public_member_api_docs guideline. Document
the MockServer class itself, and all public methods and properties: the url and
wsUrl fields, the start() static factory method, and the instance methods
stop(), post(), get(), and waitUntilReady(). Each comment should explain the
purpose and usage of the respective public API member.
In `@sample_app/integration_test/pages/message_list_page.dart`:
- Around line 4-6: The MessageListPage class and its public static constant
composer are missing documentation comments required by the coding guidelines.
Add doc comments (starting with ///) to both the MessageListPage abstract final
class and the static const composer field to document their purpose and usage,
ensuring all public APIs are properly documented.
In `@sample_app/integration_test/PLAN.md`:
- Around line 233-237: The fenced code blocks in PLAN.md at lines 233-237 and
275-321 are missing language identifiers after the opening triple backticks,
which triggers the MD040 linting rule. Add an appropriate language identifier
(such as `text`, `ruby`, or `dart`) immediately after the triple backticks for
each fenced code block to specify the content type and resolve the linting
violation.
In `@sample_app/integration_test/robots/backend_robot.dart`:
- Around line 1-54: Replace the relative import statement with a package import
for the mock_server module in the BackendRobot file. Additionally, add Dart doc
comments (using ///) to all public APIs including the BackendRobot class
definition and all public methods: generateChannels, failNewMessages,
freezeNewMessages, revokeToken, invalidateToken, invalidateTokenDate,
invalidateTokenSignature, and breakTokenGeneration. Each doc comment should
clearly describe the purpose and behavior of the method or class.
In `@sample_app/integration_test/robots/participant_robot.dart`:
- Around line 1-176: The ParticipantRobot class in participant_robot.dart has
two issues: the imports at the top use relative paths instead of package
imports, and all public methods lack documentation comments. Replace the
relative imports (using ../ paths) with package imports using the package:
prefix for the data_types and mock_server modules. Then add doc comments to the
ParticipantRobot class and all its public methods including startTyping,
stopTyping, sendMessage, editMessage, deleteMessage, quoteMessage, uploadGiphy,
pinMessage, unpinMessage, uploadAttachment, addReaction, deleteReaction, and
their related variants to document their purpose and behavior.
In `@sample_app/integration_test/support/stream_test_env.dart`:
- Around line 5-8: Replace all relative imports in this file with package
imports by converting imports like '../mock_server/mock_server.dart',
'../robots/backend_robot.dart', '../robots/participant_robot.dart', and
'../robots/user_robot.dart' to use the package: prefix (e.g.,
'package:sample_app/...'). Additionally, add documentation comments to all
public APIs exposed by this file, specifically the StreamTestEnv class, setUp
function, and tearDown function, ensuring each public member has a clear doc
comment explaining its purpose and usage.
In `@sample_app/integration_test/support/stream_test.dart`:
- Around line 9-13: The public streamTest function lacks a doc comment which is
required by the public_member_api_docs guideline. Add a documentation comment
above the streamTest function that clearly describes its purpose as a test
wrapper, documents the parameters (description, callback, and allureId), and
explains its intended usage and behavior for stream-based integration testing.
- Line 7: Replace the relative import statement in stream_test.dart with a
package import to improve maintainability and follow the
always_use_package_imports guideline. Change the import from using the relative
path '../allure/allure.dart' to use the full package path format
'package:sample_app/integration_test/allure/allure.dart' instead. This makes the
codebase more resilient to directory refactoring and improves code clarity.
---
Nitpick comments:
In @.github/workflows/e2e_test_cron.yml:
- Around line 162-168: Replace the archived `8398a7/action-slack@v3` action with
the actively maintained `slackapi/slack-github-action`. Update the uses
statement to reference the new action, and adjust the input parameters
accordingly since the new action has a different API - the text and fields
parameters should be replaced with the appropriate payload structure expected by
slackapi/slack-github-action, and ensure the SLACK_WEBHOOK_URL environment
variable is properly passed to the new action's configuration.
In `@sample_app/integration_test/message_list_test.dart`:
- Around line 3-6: Replace all four relative imports at the top of the
message_list_test.dart file with package imports. Convert the imports of
user_robot_message_list_asserts.dart, step.dart, stream_test.dart, and
stream_test_env.dart from relative paths (using ./) to package imports using the
format package:sample_app/path/to/file.dart. Ensure the paths after
package:sample_app/ reflect the correct location of these imported files within
the project structure.
In `@sample_app/integration_test/pages/channel_list_page.dart`:
- Around line 3-5: Add doc comments to the public API members in the
ChannelListPage class to comply with the Dart documentation requirement. The
ChannelListPage class itself needs a doc comment explaining its purpose, and the
static channelTile constant member needs a doc comment describing what it
represents. Place the doc comments immediately before each member declaration
using the triple-slash (///) syntax to ensure the repository's API documentation
generation includes these definitions.
In `@sample_app/integration_test/robots/user_robot_message_list_asserts.dart`:
- Around line 1-7: The file uses a relative import statement instead of a
package import, and the public extension UserRobotMessageListAsserts and its
public method assertMessage lack doc comments. Replace the relative import with
a package import statement that includes the appropriate package path, and add
doc comments (using ///) to both the UserRobotMessageListAsserts extension and
the assertMessage method to document their purpose and usage.
In `@sample_app/integration_test/robots/user_robot.dart`:
- Around line 3-33: Replace all relative imports in the UserRobot class file
with absolute package imports (convert imports like
'../pages/channel_list_page.dart' to use the package: syntax). Additionally, add
documentation comments using the /// syntax to the public UserRobot class and
all its public methods including login, openChannel, and sendMessage to document
their purpose, parameters, and return values according to Dart documentation
guidelines.
In `@sample_app/integration_test/support/predefined_users.dart`:
- Around line 3-26: Add doc comments to the public classes UserCredentials and
PredefinedUsers to document their purpose and usage, and split the long JWT
token literal in the qaTest1 constant across multiple adjacent string segments
to ensure no line exceeds the 120 character limit as per coding guidelines.
In `@sample_app/integration_test/support/step.dart`:
- Around line 1-4: The import statement for the allure module uses a relative
import path, but the repo requires package imports for all Dart files.
Additionally, the public step function is missing a documentation comment as
required by the Dart style guidelines. Convert the relative import statement to
use the package import syntax, and add a doc comment above the step function
definition that documents its purpose, parameters, and return type to comply
with the coding standards for public APIs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea4abfd1-bfdb-44c3-a296-04aa453ec6b9
⛔ Files ignored due to path filters (2)
sample_app/android/Gemfile.lockis excluded by!**/*.locksample_app/ios/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
.github/workflows/e2e_test.yml.github/workflows/e2e_test_cron.yml.gitignoremelos.yamlsample_app/Allurefilesample_app/Fastfilesample_app/android/app/build.gradlesample_app/android/app/src/androidTest/java/io/getstream/chat/android/flutter/sample/MainActivityTest.javasample_app/integration_test/PLAN.mdsample_app/integration_test/allure/allure.dartsample_app/integration_test/message_list_test.dartsample_app/integration_test/mock_server/data_types.dartsample_app/integration_test/mock_server/mock_server.dartsample_app/integration_test/pages/channel_list_page.dartsample_app/integration_test/pages/message_list_page.dartsample_app/integration_test/robots/backend_robot.dartsample_app/integration_test/robots/participant_robot.dartsample_app/integration_test/robots/user_robot.dartsample_app/integration_test/robots/user_robot_message_list_asserts.dartsample_app/integration_test/support/predefined_users.dartsample_app/integration_test/support/step.dartsample_app/integration_test/support/stream_test.dartsample_app/integration_test/support/stream_test_env.dartsample_app/ios/Podfilesample_app/ios/Runner.xcodeproj/project.pbxprojsample_app/ios/Runner.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolvedsample_app/ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcschemesample_app/ios/Runner.xcworkspace/xcshareddata/swiftpm/Package.resolvedsample_app/ios/RunnerUITests/RunnerUITests.msample_app/ios/add_patrol_target.rbsample_app/lib/app.dartsample_app/lib/auth/auth_controller.dartsample_app/pubspec.yaml
Comment on lines
+1
to
+20
| name: E2E Tests Nightly | ||
|
|
||
| on: | ||
| schedule: | ||
| - cron: "0 1 * * 1-5" # weeknights at 01:00 UTC | ||
| workflow_dispatch: | ||
| inputs: | ||
| mock_server_branch: | ||
| description: "Mock server branch" | ||
| type: string | ||
| required: true | ||
| default: main | ||
|
|
||
| concurrency: | ||
| group: e2e-nightly-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| env: | ||
| flutter_version: "3.x" | ||
| mock_server_branch: ${{ github.event.inputs.mock_server_branch || 'main' }} |
Contributor
There was a problem hiding this comment.
Add a workflow-level permissions block.
Same as the on-demand workflow, this needs explicit minimal permissions.
🛡️ Proposed fix
concurrency:
group: e2e-nightly-${{ github.ref }}
cancel-in-progress: true
+permissions:
+ contents: read
+
env:
flutter_version: "3.x"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| name: E2E Tests Nightly | |
| on: | |
| schedule: | |
| - cron: "0 1 * * 1-5" # weeknights at 01:00 UTC | |
| workflow_dispatch: | |
| inputs: | |
| mock_server_branch: | |
| description: "Mock server branch" | |
| type: string | |
| required: true | |
| default: main | |
| concurrency: | |
| group: e2e-nightly-${{ github.ref }} | |
| cancel-in-progress: true | |
| env: | |
| flutter_version: "3.x" | |
| mock_server_branch: ${{ github.event.inputs.mock_server_branch || 'main' }} | |
| name: E2E Tests Nightly | |
| on: | |
| schedule: | |
| - cron: "0 1 * * 1-5" # weeknights at 01:00 UTC | |
| workflow_dispatch: | |
| inputs: | |
| mock_server_branch: | |
| description: "Mock server branch" | |
| type: string | |
| required: true | |
| default: main | |
| concurrency: | |
| group: e2e-nightly-${{ github.ref }} | |
| cancel-in-progress: true | |
| permissions: | |
| contents: read | |
| env: | |
| flutter_version: "3.x" | |
| mock_server_branch: ${{ github.event.inputs.mock_server_branch || 'main' }} |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 1-169: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e_test_cron.yml around lines 1 - 20, The E2E Tests
Nightly workflow in the e2e_test_cron.yml file is missing explicit minimal
permissions at the workflow level. Add a workflow-level permissions block to the
file (after the concurrency section or at the top level, similar to how other
workflow files are structured) and define explicit minimal permissions that
match what is configured in the corresponding on-demand workflow. This ensures
the workflow operates with the principle of least privilege.
Source: Linters/SAST tools
Comment on lines
+62
to
+65
| - name: Create Allure launch | ||
| env: | ||
| ALLURE_TOKEN: ${{ secrets.ALLURE_TOKEN }} | ||
| run: cd sample_app/android && bundle exec fastlane allure_launch |
Contributor
There was a problem hiding this comment.
Each matrix cell creates its own Allure launch instead of sharing one.
With the matrix strategy, each Android (3 cells) and iOS (2 cells) job independently runs fastlane allure_launch, creating 5 separate launches per nightly run. The PR objective states results should upload to a shared launch.
Consider extracting the launch creation into a separate setup job that runs once and outputs the launch ID for matrix jobs to consume:
🔧 Architectural suggestion
jobs:
setup:
runs-on: ubuntu-latest
outputs:
launch_id: ${{ steps.create.outputs.launch_id }}
steps:
- uses: actions/checkout@v6
- uses: ruby/setup-ruby@v1
with:
ruby-version: "3.3"
bundler-cache: true
working-directory: sample_app/android
- name: Create Allure launch
id: create
env:
ALLURE_TOKEN: ${{ secrets.ALLURE_TOKEN }}
run: |
cd sample_app/android
launch_id=$(bundle exec fastlane allure_launch)
echo "launch_id=$launch_id" >> "$GITHUB_OUTPUT"
android:
needs: setup
# ... existing config ...
steps:
# Remove "Create Allure launch" step
# In "Upload Allure results":
- name: Upload Allure results
env:
ALLURE_TOKEN: ${{ secrets.ALLURE_TOKEN }}
ALLURE_LAUNCH_ID: ${{ needs.setup.outputs.launch_id }}
run: cd sample_app/android && bundle exec fastlane allure_uploadAlso applies to: 123-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e_test_cron.yml around lines 62 - 65, The current
workflow creates a separate Allure launch for each matrix cell (3 Android
variants and 2 iOS variants), but they should all upload to a single shared
launch. Create a new `setup` job that runs once before the matrix jobs to
generate a launch_id, then extract that launch_id as an output so the android
and ios jobs can consume it via `needs: setup`. Remove the "Create Allure
launch" step from both the android and ios job definitions, and update their
"Upload Allure results" steps to use the shared launch_id from the setup job
output instead of creating new launches in each matrix cell.
Comment on lines
+1
to
+16
| name: e2e | ||
|
|
||
| on: | ||
| workflow_dispatch: | ||
| pull_request: | ||
| types: [labeled, synchronize] | ||
|
|
||
| # Heavy mobile e2e — keep off the default PR path; run on demand or when a PR | ||
| # carries the `e2e` label. The nightly schedule lives in e2e_test_cron.yml. | ||
| concurrency: | ||
| group: e2e-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| env: | ||
| flutter_version: "3.x" | ||
|
|
Contributor
There was a problem hiding this comment.
Add a workflow-level permissions block.
The workflow uses default permissions, which are overly broad. Since this workflow only needs to checkout code and upload artifacts, add a minimal permissions block.
🛡️ Proposed fix
concurrency:
group: e2e-${{ github.ref }}
cancel-in-progress: true
+permissions:
+ contents: read
+
env:
flutter_version: "3.x"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| name: e2e | |
| on: | |
| workflow_dispatch: | |
| pull_request: | |
| types: [labeled, synchronize] | |
| # Heavy mobile e2e — keep off the default PR path; run on demand or when a PR | |
| # carries the `e2e` label. The nightly schedule lives in e2e_test_cron.yml. | |
| concurrency: | |
| group: e2e-${{ github.ref }} | |
| cancel-in-progress: true | |
| env: | |
| flutter_version: "3.x" | |
| name: e2e | |
| on: | |
| workflow_dispatch: | |
| pull_request: | |
| types: [labeled, synchronize] | |
| # Heavy mobile e2e — keep off the default PR path; run on demand or when a PR | |
| # carries the `e2e` label. The nightly schedule lives in e2e_test_cron.yml. | |
| concurrency: | |
| group: e2e-${{ github.ref }} | |
| cancel-in-progress: true | |
| permissions: | |
| contents: read | |
| env: | |
| flutter_version: "3.x" |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 1-124: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e_test.yml around lines 1 - 16, Add a workflow-level
permissions block to the e2e workflow to restrict permissions to only what is
needed. The permissions block should be added after the 'on:' section (after the
pull_request trigger configuration) and before the 'concurrency:' section. Since
this workflow only needs to checkout code and upload artifacts, add a
permissions block that specifies contents: read for code checkout, and any other
minimal permissions required for the specific operations the workflow performs.
This replaces the default overly-broad permissions with an explicitly minimal
set.
Source: Linters/SAST tools
Comment on lines
+14
to
+18
| log = if ios | ||
| `xcrun simctl spawn #{options[:device]} log show --last 30m --style compact 2>/dev/null` | ||
| else | ||
| `adb logcat -d 2>/dev/null` | ||
| end |
Contributor
There was a problem hiding this comment.
Android logcat command ignores the device: option.
The iOS branch correctly passes options[:device] to simctl, but the Android branch uses adb logcat -d without a device specifier. When multiple emulators or devices are connected, this retrieves logs from the default device rather than the one specified via device:.
Proposed fix
log = if ios
`xcrun simctl spawn #{options[:device]} log show --last 30m --style compact 2>/dev/null`
else
- `adb logcat -d 2>/dev/null`
+ device_flag = options[:device] ? "-s #{options[:device]}" : ''
+ `adb #{device_flag} logcat -d 2>/dev/null`
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| log = if ios | |
| `xcrun simctl spawn #{options[:device]} log show --last 30m --style compact 2>/dev/null` | |
| else | |
| `adb logcat -d 2>/dev/null` | |
| end | |
| log = if ios | |
| `xcrun simctl spawn #{options[:device]} log show --last 30m --style compact 2>/dev/null` | |
| else | |
| device_flag = options[:device] ? "-s #{options[:device]}" : '' | |
| `adb #{device_flag} logcat -d 2>/dev/null` | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sample_app/Allurefile` around lines 14 - 18, The adb logcat command in the
Android branch does not utilize the options[:device] parameter, which causes it
to retrieve logs from the default device when multiple devices are connected.
Modify the adb logcat -d command to include the device specifier by passing
options[:device] to the adb command using the -s flag (adb -s <device> syntax)
so that it respects the device parameter consistently with the iOS branch that
already uses options[:device] for simctl.
| # Options: device:, target: (single file), local_server:, mock_server_branch:. | ||
| lane :run_e2e_test do |options| | ||
| start_mock_server(local_server: options[:local_server], branch: options[:mock_server_branch]) | ||
| sh('adb logcat -c') rescue nil |
Contributor
There was a problem hiding this comment.
adb logcat -c should specify device when provided.
Consistent with the issue in collect_allure_results, the logcat clear command should target the specific device when options[:device] is provided.
Proposed fix
start_mock_server(local_server: options[:local_server], branch: options[:mock_server_branch])
- sh('adb logcat -c') rescue nil
+ device_flag = options[:device] ? "-s #{options[:device]}" : ''
+ sh("adb #{device_flag} logcat -c") rescue nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sh('adb logcat -c') rescue nil | |
| start_mock_server(local_server: options[:local_server], branch: options[:mock_server_branch]) | |
| device_flag = options[:device] ? "-s #{options[:device]}" : '' | |
| sh("adb #{device_flag} logcat -c") rescue nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sample_app/Fastfile` at line 59, The adb logcat clear command in the current
implementation does not specify the target device when options[:device] is
provided. Modify the sh('adb logcat -c') command to include the device
specification flag (such as -s with the device name) when options[:device] is
present, making it consistent with the device targeting pattern used in the
collect_allure_results function. Ensure the device parameter is only appended to
the command when it is available in the options hash.
Comment on lines
+18
to
+24
| static Future<MockServer> start({String? testName}) async { | ||
| final name = testName ?? _currentTestName(); | ||
| final driverUrl = 'http://$_host:$_driverPort'; | ||
| final port = (await _get('$driverUrl/start/$name')).trim(); | ||
| final server = MockServer._('http://$_host:$port', 'ws://$_host:$port'); | ||
| await server.waitUntilReady(); | ||
| return server; |
Contributor
There was a problem hiding this comment.
Add explicit timeouts to mock-driver HTTP calls.
MockServer.start() depends on _get('/start/...'); if the driver is unresponsive, this can hang indefinitely before waitUntilReady() even runs. Apply request timeouts for _get/_statusCode/post.
Suggested patch
static Future<String> _get(String url) async {
final client = HttpClient();
try {
- final req = await client.getUrl(Uri.parse(url));
- final res = await req.close();
+ final req = await client.getUrl(Uri.parse(url)).timeout(
+ const Duration(seconds: 5),
+ );
+ final res = await req.close().timeout(const Duration(seconds: 5));
return res.transform(utf8.decoder).join();
} finally {
client.close(force: true);
}
}Also applies to: 65-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sample_app/integration_test/mock_server/mock_server.dart` around lines 18 -
24, The MockServer.start() method makes an HTTP call via _get() to the mock
driver without an explicit timeout, which can cause indefinite hangs if the
driver is unresponsive. Add explicit request timeouts to the _get(),
_statusCode(), and post() helper methods to prevent blocking indefinitely. Each
HTTP request should include a timeout duration parameter (typically in the range
of seconds) to ensure the application can fail fast and proceed to error
handling or retry logic when the driver is unresponsive.
Comment on lines
+15
to
+26
| final messagesTextParam = | ||
| messagesText != null ? 'messages_text=$messagesText&' : ''; | ||
| final repliesTextParam = | ||
| repliesText != null ? 'replies_text=$repliesText&' : ''; | ||
| await _mockServer.post( | ||
| 'mock?' | ||
| '$messagesTextParam' | ||
| '$repliesTextParam' | ||
| 'channels=$channelsCount&' | ||
| 'messages=$messagesCount&' | ||
| 'replies=$repliesCount', | ||
| ); |
Contributor
There was a problem hiding this comment.
URL-encode text query parameters instead of string concatenation.
messagesText and repliesText are inserted directly into the query string; reserved characters (&, ?, spaces, =) will corrupt request parameters.
Suggested patch
- final messagesTextParam =
- messagesText != null ? 'messages_text=$messagesText&' : '';
- final repliesTextParam =
- repliesText != null ? 'replies_text=$repliesText&' : '';
- await _mockServer.post(
- 'mock?'
- '$messagesTextParam'
- '$repliesTextParam'
- 'channels=$channelsCount&'
- 'messages=$messagesCount&'
- 'replies=$repliesCount',
- );
+ final query = <String, String>{
+ 'channels': '$channelsCount',
+ 'messages': '$messagesCount',
+ 'replies': '$repliesCount',
+ if (messagesText != null) 'messages_text': messagesText,
+ if (repliesText != null) 'replies_text': repliesText,
+ };
+ final endpoint = Uri(path: 'mock', queryParameters: query).toString();
+ await _mockServer.post(endpoint);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| final messagesTextParam = | |
| messagesText != null ? 'messages_text=$messagesText&' : ''; | |
| final repliesTextParam = | |
| repliesText != null ? 'replies_text=$repliesText&' : ''; | |
| await _mockServer.post( | |
| 'mock?' | |
| '$messagesTextParam' | |
| '$repliesTextParam' | |
| 'channels=$channelsCount&' | |
| 'messages=$messagesCount&' | |
| 'replies=$repliesCount', | |
| ); | |
| final query = <String, String>{ | |
| 'channels': '$channelsCount', | |
| 'messages': '$messagesCount', | |
| 'replies': '$repliesCount', | |
| if (messagesText != null) 'messages_text': messagesText, | |
| if (repliesText != null) 'replies_text': repliesText, | |
| }; | |
| final endpoint = Uri(path: 'mock', queryParameters: query).toString(); | |
| await _mockServer.post(endpoint); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sample_app/integration_test/robots/backend_robot.dart` around lines 15 - 26,
The messagesText and repliesText parameters are being directly concatenated into
the query string without URL-encoding, which will cause the request parameters
to be corrupted if these values contain reserved characters like ampersands,
question marks, spaces, or equals signs. Apply proper URL-encoding to the
messagesText and repliesText values before they are inserted into the
messagesTextParam and repliesTextParam string construction. Use Dart's URI
encoding utility to ensure these parameters are safely encoded for the query
string in the _mockServer.post() call.
Comment on lines
+18
to
+23
| static const qaTest1 = UserCredentials( | ||
| id: 'qatest1', | ||
| name: 'QA test 1', | ||
| token: | ||
| 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyX2lkIjoicWF0ZXN0MSJ9.fnelU7HcP7QoEEsCGteNlF1fppofzNlrnpDQuIgeKCU', | ||
| ); |
Contributor
There was a problem hiding this comment.
Do not commit long-lived JWTs in test fixtures.
PredefinedUsers.qaTest1.token is hardcoded credential material. Even in test code, this can be reused outside intended flows and is hard to rotate safely. Prefer generating short-lived test tokens from the mock server per test run (or injecting via secure CI secrets).
🧰 Tools
🪛 Betterleaks (1.5.0)
[high] 22-22: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sample_app/integration_test/support/predefined_users.dart` around lines 18 -
23, Remove the hardcoded token from the `qaTest1` UserCredentials constant in
the `PredefinedUsers` class. Instead of embedding the JWT directly in the
fixture, implement a mechanism to generate short-lived test tokens dynamically
from the mock server on each test run. Alternatively, inject the token from
secure CI secrets at runtime. Update the `UserCredentials` class or the test
initialization to accept dynamically generated tokens rather than relying on
static fixture values.
Source: Linters/SAST tools
Comment on lines
+30
to
+33
| Future<void> tearDown() async { | ||
| await authController.debugReset(); | ||
| await mockServer.stop(); | ||
| } |
Contributor
There was a problem hiding this comment.
Make teardown cleanup resilient with try/finally.
If authController.debugReset() throws, mockServer.stop() is skipped, which can leak server state into subsequent tests.
Suggested patch
Future<void> tearDown() async {
- await authController.debugReset();
- await mockServer.stop();
+ try {
+ await authController.debugReset();
+ } finally {
+ await mockServer.stop();
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sample_app/integration_test/support/stream_test_env.dart` around lines 30 -
33, The tearDown() method in the stream test environment class does not handle
exceptions properly - if authController.debugReset() throws an error, the
mockServer.stop() call will be skipped, leaving the server in an unclean state
for subsequent tests. Wrap the teardown logic in a try/finally block where
authController.debugReset() is in the try block and mockServer.stop() is in the
finally block to ensure the mock server is always stopped regardless of whether
the auth controller reset succeeds or fails.
| 97C146FD1CF9000F007C117D /* Assets.xcassets */ = {isa = PBXFileReference; lastKnownFileType = folder.assetcatalog; path = Assets.xcassets; sourceTree = "<group>"; }; | ||
| 97C147001CF9000F007C117D /* Base */ = {isa = PBXFileReference; lastKnownFileType = file.storyboard; name = Base; path = Base.lproj/LaunchScreen.storyboard; sourceTree = "<group>"; }; | ||
| 97C147021CF9000F007C117D /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = "<group>"; }; | ||
| A6B213AE2D543B7BADA38E33 /* Foundation.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Foundation.framework; path = Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS18.0.sdk/System/Library/Frameworks/Foundation.framework; sourceTree = DEVELOPER_DIR; }; |
Contributor
There was a problem hiding this comment.
Use an SDK-relative Foundation framework reference instead of a version-pinned SDK path.
Line 83 hardcodes iPhoneOS18.0.sdk under DEVELOPER_DIR; this is toolchain-specific and can fail when CI/local Xcode has a different installed SDK folder version. Use SDKROOT + System/Library/Frameworks/Foundation.framework (or remove explicit Foundation linking if unnecessary for this target).
Suggested fix
- A6B213AE2D543B7BADA38E33 /* Foundation.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Foundation.framework; path = Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS18.0.sdk/System/Library/Frameworks/Foundation.framework; sourceTree = DEVELOPER_DIR; };
+ A6B213AE2D543B7BADA38E33 /* Foundation.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Foundation.framework; path = System/Library/Frameworks/Foundation.framework; sourceTree = SDKROOT; };Also applies to: 103-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sample_app/ios/Runner.xcodeproj/project.pbxproj` at line 83, Replace the
hardcoded SDK version path in the Foundation.framework file reference
(A6B213AE2D543B7BADA38E33). Instead of using the explicit path containing
`iPhoneOS18.0.sdk`, update the path attribute to use a dynamic SDK-relative
reference such as using the `SDKROOT` variable or a relative path pattern like
`System/Library/Frameworks/Foundation.framework` that will work across different
Xcode versions and CI environments. Apply the same fix to other SDK
version-pinned framework references mentioned at lines 103-105.
xsahil03x
reviewed
Jun 22, 2026
| dirExists: test | ||
|
|
||
| e2e:run: | ||
| run: cd sample_app && patrol test |
Member
There was a problem hiding this comment.
You can add a --depends-on="patrol" instead of navigating to sample_app and run the test command instead.
Check how we do it in the update:goldens command
Collaborator
Author
There was a problem hiding this comment.
will check it out, thanks a mill @xsahil03x
just a quick note, this PR is still in draft, i need to test and adjust a lot of things
Member
There was a problem hiding this comment.
Yes sure, I was just taking a quick look
Port the iOS-style sources_matrix lane and is_check_required gating so the e2e Fastlane lanes self-skip unless sample_app, packages, melos.yaml, or the e2e workflows changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ntain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ntain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a Patrol-based end-to-end testing infrastructure for the Flutter
sample_app, running on iOS and Android against the sharedstream-chat-test-mock-server, mirroring the Robot / PageObject / DSL conventions from the native Android & iOS SDKs.A detailed, phase-by-phase plan lives in
sample_app/integration_test/PLAN.md.How it's wired
debugConnectionOverrideon theAuthControllersingleton (noIntentextras / env vars like the native repos need).isE2eTestRunguards Firebase/push/notifications during the in-process boot, andAuthController.debugReset()isolates state between bundled tests.MockServer,BackendRobot,ParticipantRobot(faithful ports of the Android robots), a fluentUserRobot+ assertion extensions, page objects, and aStreamTestEnv+step()BDD wrapper.integration_test/allure/) emits Allure 2 results via chunked log markers;collect_allure_resultsreassembles them (adb logcat /simctl log), andallure_upload/allure_launchpush to Allure TestOps (project 135).Fastfile(mock-server + e2e run lanes) +Allurefile(Allure lanes).e2e_test.yml(PR-labele2e+ manual) and a separate nightlye2e_test_cron.yml(matrix coverage + Slack-on-failure), mirroring the native repos' split.Validated locally (Android emulator + iOS simulator)
run_e2e_testFastlane lane, and real Allure uploads to TestOps.Summary by CodeRabbit