Harden E2E UiAutomator text-wait helpers against recomposition#6383
Harden E2E UiAutomator text-wait helpers against recomposition#6383
Conversation
Rework UiObject2.waitForText into BySelector.waitForText that re-resolves the selector on each poll, absorbing StaleObjectException from mid-poll recomposition. Bypass the findObject() extension (which lies about nullability) by calling device.findObject(this) directly. Accept a mustBeEqual flag so the channel-preview assertion can substring-match and avoid timing out when the rendered preview has trailing whitespace. Update the 4 robot call sites.
waitToAppear and waitToAppear(withIndex) previously NPE'd via the findObject() extension's non-null declaration when the selector didn't match before timeout. Now they throw IllegalStateException with the selector, timeout, and (for the indexed overload) matched object count.
andremion
added
pr:internal
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
SDK Size Comparison 📏
|
WalkthroughThe changes refactor E2E test infrastructure to improve message preview assertions and enhance the underlying UI automation wait mechanisms. Test assertions are updated to use non-strict text matching for deleted message previews, and the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
stream-chat-android-e2e-test/src/main/kotlin/io/getstream/chat/android/e2e/test/uiautomator/Wait.kt (2)
67-104: Polling + stale-absorbing text wait — looks correct.The re-resolve-on-each-poll strategy with
StaleObjectExceptionswallowed insidecurrentTextOrNull()is the right fix for recomposition-driven staleness, and returning the last observed text keeps timeout diagnostics informative when the caller wraps the result inassertEquals.Two minor observations:
- The loop shape is
check → sleep, so a match that lands during the final 50 msThread.sleepwindow is not observed before returninglastText. Harmless in practice givendefaultTimeout, but if you ever tighten timeouts you may want a final post-loop check.- When the node never materialises at all,
lastTextstays as the initial"", which is indistinguishable from a node that legitimately had empty text. For a future iteration, returningString?(null = never observed) would make assertion failures slightly easier to read. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-e2e-test/src/main/kotlin/io/getstream/chat/android/e2e/test/uiautomator/Wait.kt` around lines 67 - 104, Add a final post-loop check in BySelector.waitForText to catch a match that may appear during the last Thread.sleep window: after the while loop and before returning lastText, call currentTextOrNull(), update lastText if non-null, and if it matches (respecting mustBeEqual/expectedText) return it; use the existing currentTextOrNull() helper and keep the function signature unchanged (POLL_INTERVAL_MILLIS can remain as-is).
28-55: Descriptive errors on timeout — nice upgrade from implicit NPE/AIOOBE.The
error(...)messages include selector, timeout, and (for the indexed variant) matched count, which will make flaky-test triage much easier.One small gotcha worth keeping in mind (not introduced by this PR):
wait(...)delegates toUntil.hasObject(this), which only waits for at least one match. ForwaitToAppear(withIndex = N)withN > 0, a slow emulator that has rendered only the first item will fall through tofindObjects()immediately, get fewer thanN+1items, and throw — even though more matches may arrive shortly after. If you see this pattern in E2E flakes, consider extendingwaitto poll forfindObjects().size > withIndexin the indexed overload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-e2e-test/src/main/kotlin/io/getstream/chat/android/e2e/test/uiautomator/Wait.kt` around lines 28 - 55, The indexed overload BySelector.waitToAppear(withIndex:Int, timeOutMillis:Long) currently calls wait(timeOutMillis) which only guarantees at least one match; change it to actively poll until device.findObjects(this).size > withIndex or the timeout elapses (use elapsed time/timestamps to enforce timeOutMillis) and then return the object at withIndex or throw the descriptive error; keep the non-indexed variant as-is and reuse device.findObjects(this) for the size check so slow-rendering additional matches are waited for instead of immediately failing.stream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/tests/ChannelListTests.kt (1)
174-200: Rename and assertion update accurately reflect post-#6212 behavior.Keeping the same
@AllureId("5821")preserves traceability while the semantic has flipped from "previous message shown" to "deleted placeholder shown". The variable rename (previousMessage/lastMessage) reads clearly against the deletion semantics.Optional: since the name emphasises "not the previous message", you could additionally assert the preview no longer contains
previousMessage(e.g. via a negative variant ofassertMessageInChannelPreview). Not required —assertDeletedMessageInChannelPreview()already captures the critical expectation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/tests/ChannelListTests.kt` around lines 174 - 200, Update the test to reflect the new behavior after `#6212`: rename the message variables to clarify roles (use previousMessage and lastMessage) and replace the old channel-preview assertion with userRobot.assertDeletedMessageInChannelPreview() in test_channelPreviewShowsDeletedPlaceholder_whenLastMessageIsDeleted; optionally add a negative assertion (e.g., assertMessageInChannelPreview does not contain previousMessage) if you want to ensure the previous message is not shown.stream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/robots/UserRobotChannelListAsserts.kt (2)
34-45:mustBeEqual = false+assertEqualspairing is subtle — worth a comment.The substring poll lets
waitForTextreturn as soon as the preview containsexpectedPreview(tolerating trailing whitespace or late-appended characters), thentrimEnd()+assertEqualsenforces the stricter shape. It's correct, but a drive-by reader could reasonably expect "non-strict wait → non-strict assert". A one-liner comment explaining "substring match to absorb trailing whitespace from composer/preview layout; strict comparison aftertrimEnd()" would save future-you a double-take. No behavior change required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/robots/UserRobotChannelListAsserts.kt` around lines 34 - 45, In UserRobot.assertMessageInChannelPreview, add a one-line comment above the wait/assert sequence explaining that Channel.messagePreview.waitForText is called with mustBeEqual = false to allow a substring match (to absorb trailing whitespace or late-appended characters from the composer/preview layout), and then trimEnd() + assertEquals enforces the strict equality; reference the call sites Channel.messagePreview.waitForText(mustBeEqual = false) and the following trimEnd() + assertEquals to make the intent explicit for future readers.
47-56: UseR.string.stream_compose_message_deleted_previewfor the deleted-message preview text.Sibling assertions (
assertEditedMessage,assertAlsoInTheChannelLabelInChannel) useappContext.getString(R.string.*)to source expected text, ensuring the test automatically tracks any string or localisation changes. Replace the hardcodedDELETED_PREVIEW_TEXTconstant with:val expectedLabel = appContext.getString(R.string.stream_compose_message_deleted_preview)This keeps the test in sync with the resource and avoids silent desync if the string is renamed or translated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/robots/UserRobotChannelListAsserts.kt` around lines 47 - 56, The test currently uses a hardcoded DELETED_PREVIEW_TEXT in assertDeletedMessageInChannelPreview; replace that constant with the resource string fetched from the app context (R.string.stream_compose_message_deleted_preview) and use that fetched value (e.g., expectedLabel = appContext.getString(R.string.stream_compose_message_deleted_preview)) when calling Channel.messagePreview.waitForText and in the assertion message so the test follows localization and resource changes; update or remove the DELETED_PREVIEW_TEXT constant and adjust the assertDeletedMessageInChannelPreview function accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@stream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/robots/UserRobotChannelListAsserts.kt`:
- Around line 34-45: In UserRobot.assertMessageInChannelPreview, add a one-line
comment above the wait/assert sequence explaining that
Channel.messagePreview.waitForText is called with mustBeEqual = false to allow a
substring match (to absorb trailing whitespace or late-appended characters from
the composer/preview layout), and then trimEnd() + assertEquals enforces the
strict equality; reference the call sites
Channel.messagePreview.waitForText(mustBeEqual = false) and the following
trimEnd() + assertEquals to make the intent explicit for future readers.
- Around line 47-56: The test currently uses a hardcoded DELETED_PREVIEW_TEXT in
assertDeletedMessageInChannelPreview; replace that constant with the resource
string fetched from the app context
(R.string.stream_compose_message_deleted_preview) and use that fetched value
(e.g., expectedLabel =
appContext.getString(R.string.stream_compose_message_deleted_preview)) when
calling Channel.messagePreview.waitForText and in the assertion message so the
test follows localization and resource changes; update or remove the
DELETED_PREVIEW_TEXT constant and adjust the
assertDeletedMessageInChannelPreview function accordingly.
In
`@stream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/tests/ChannelListTests.kt`:
- Around line 174-200: Update the test to reflect the new behavior after `#6212`:
rename the message variables to clarify roles (use previousMessage and
lastMessage) and replace the old channel-preview assertion with
userRobot.assertDeletedMessageInChannelPreview() in
test_channelPreviewShowsDeletedPlaceholder_whenLastMessageIsDeleted; optionally
add a negative assertion (e.g., assertMessageInChannelPreview does not contain
previousMessage) if you want to ensure the previous message is not shown.
In
`@stream-chat-android-e2e-test/src/main/kotlin/io/getstream/chat/android/e2e/test/uiautomator/Wait.kt`:
- Around line 67-104: Add a final post-loop check in BySelector.waitForText to
catch a match that may appear during the last Thread.sleep window: after the
while loop and before returning lastText, call currentTextOrNull(), update
lastText if non-null, and if it matches (respecting mustBeEqual/expectedText)
return it; use the existing currentTextOrNull() helper and keep the function
signature unchanged (POLL_INTERVAL_MILLIS can remain as-is).
- Around line 28-55: The indexed overload BySelector.waitToAppear(withIndex:Int,
timeOutMillis:Long) currently calls wait(timeOutMillis) which only guarantees at
least one match; change it to actively poll until device.findObjects(this).size
> withIndex or the timeout elapses (use elapsed time/timestamps to enforce
timeOutMillis) and then return the object at withIndex or throw the descriptive
error; keep the non-indexed variant as-is and reuse device.findObjects(this) for
the size check so slow-rendering additional matches are waited for instead of
immediately failing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f82506c2-5eec-4f66-aff2-5059a4a15589
📒 Files selected for processing (4)
stream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/robots/UserRobotChannelListAsserts.ktstream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/robots/UserRobotMessageListAsserts.ktstream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/tests/ChannelListTests.ktstream-chat-android-e2e-test/src/main/kotlin/io/getstream/chat/android/e2e/test/uiautomator/Wait.kt
andremion
force-pushed
the
chore/fix-e2e-tests
branch
from
ecbe0ae to
77017b5
Compare
VelikovPetar
left a comment
VelikovPetar
left a comment
There was a problem hiding this comment.
I believe that we should be able to uncomment the following test after merging this: test_quotedReplyInThreadAndAlsoInChannel
…xt survives recomposition.
Let's see: a9d51bd |
|
2 participants
Goal
Compose-driven nodes get recycled mid-poll, causing
waitForTextto hitStaleObjectExceptionor NPE and fail tests deterministically on slow emulators. Tighten the helpers so assertions survive recomposition and surface timeouts with a clear error.Implementation
stream-chat-android-e2e-test / Wait.ktwaitForTextmoved fromUiObject2receiver toBySelector. Re-resolves the node on every iteration viadevice.findObject(this), swallowsStaleObjectExceptionas "no match yet", sleeps 50 ms per poll, and returns the matchedString(or last observed text on timeout).waitToAppear(both overloads) now throws a descriptiveIllegalStateExceptionon timeout instead of leaking a null NPE orArrayIndexOutOfBoundsException.stream-chat-android-compose-sample / robotsUserRobotChannelListAssertsandUserRobotMessageListAssertsmigrated to the newBySelector.waitForText(…)signature, dropping the priorwaitToAppear().textpattern that was susceptible to the same recomposition races.API —
waitForTextreceiver and return type changed (UiObject2 → BySelector,UiObject2 → String). Internal to the test-infra module; all in-repo consumers migrated.Testing
E2E run required (device/emulator). On
stream-chat-android-compose-samplewith the mock server:assertEditedMessage,assertQuotedMessage,assertMentionWasApplied,assertThreadReplyLabel) andChannelListTests.test_channelPreviewShowsPreviousMessage_whenLastMessageIsDeleted. Expect: no intermittentStaleObjectExceptionin logcat; previously flaky suites pass consistently.waitToAppearcall site and re-run. Expect:IllegalStateException: waitToAppear timed out after 10000ms; no object matched selector: …instead of an NPE.Follow-up tasks (out of scope here)
Separate PRs planned under
refactor/e2e-uiautomator-helpersandperf/e2e-test-runtime:BySelector.wait()discards its boolean — either return it or throw on timeout; migrate call sites that currently do.wait().isDisplayed().Wait.kt(waitForCount,waitForTextToChange) and toUiObject2.isDisplayed()inElement.kt.assertSystemMessagecurrently discardsisDisplayed()— add the missingassertTrue/assertFalse.openChannelhitsArrayIndexOutOfBoundsExceptionwhen the channel list hasn't loaded; switch towaitToAppear(withIndex = …).scrollUp/DownUntilDisplayed) NPE on miss — replace with descriptive error.sleep(500/1000/5000)with predicate waits where possible; reducedefaultTimeoutfrom 10 s and add an explicit long timeout for known-slow ops.UiObject2.typeText(direct.text =) instead ofadb shell input textwhere the field is addressable — the shell path types character-by-character and dominates runtime for long strings.RetryRuleattachment recording fires per failed attempt — record only the final attempt.No UI changes.
Summary by CodeRabbit