Update BrowserStack test device to Samsung S20#3759
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR enhances instrumentation test reliability across the CommCare Android test suite. It rewrites the Wi-Fi toggle utility to use shell commands with state polling, adds explicit waits for UI elements before interactions, and replaces hardcoded dialog button strings with localized resource references. Multiple test files receive import cleanup and formatting improvements. An accessibility enhancement adds a content description to the clear search button, and the CI workflow updates the BrowserStack device target from Samsung Galaxy S10 to S20. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Comment |
![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: 2
🧹 Nitpick comments (3)
app/instrumentation-tests/src/org/commcare/androidTests/SessionExpirationTests.kt (1)
39-43: Conditional notification text may be fragile (but app-count interference is unlikely).
SessionExpirationTests.ktlines 39-43 branches onBuildConfig.DEBUG || AppUtils.getInstalledAppRecords().size <= 1to choose the expected"Logged Into Commcare"vs"Logged Into $APP_NAME". In the instrumentation test suite, other tests generally install a single app in@BeforeusingBaseTest.installApp, which uninstalls the current app when switching to a different app—so they shouldn’t leave multiple installed app records behind to perturbgetInstalledAppRecords().size. The main remaining flake risk is pre-existing apps on the device (or leftover records after uninstall) and any changes in the notification text format; consider asserting on a more stable UI/system indicator rather than app name.🤖 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 `@app/instrumentation-tests/src/org/commcare/androidTests/SessionExpirationTests.kt` around lines 39 - 43, The current conditional in SessionExpirationTests.kt that checks BuildConfig.DEBUG || AppUtils.getInstalledAppRecords().size <= 1 to choose between By.textStartsWith("Logged Into Commcare") and By.textStartsWith("Logged Into $APP_NAME") is brittle; instead make the assertion use a more stable indicator (e.g., match the common prefix "Logged Into" or check a resource id/notification content description) so the test does not depend on installed app count or exact app name. Update the uiDevice.wait call in SessionExpirationTests.kt (the branch using uiDevice.wait(..., 5000) and By.textStartsWith(...)) to assert only the stable portion ("Logged Into") or another stable UI identifier rather than branching on BuildConfig.DEBUG/AppUtils.getInstalledAppRecords().app/instrumentation-tests/src/org/commcare/androidTests/CaseClaimTest.java (1)
23-23: ⚡ Quick winDrop the unused
isRootstatic import.Line 23 isn't referenced anywhere in this file, so it should be removed with the rest of the Java import cleanup. As per coding guidelines,
**/*.{kt,java}: Clean up any unused code and 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 `@app/instrumentation-tests/src/org/commcare/androidTests/CaseClaimTest.java` at line 23, Remove the unused static import of isRoot from CaseClaimTest.java; locate the import statement "import static androidx.test.espresso.matcher.ViewMatchers.isRoot;" and delete it so the file no longer contains that unused symbol, then run a quick compile or organize imports to ensure no other unused imports remain.Source: Coding guidelines
app/instrumentation-tests/src/org/commcare/androidTests/SignatureTests.kt (1)
18-18: ⚡ Quick winPrefer an explicit UI wait over
sleep(1).Line 131 reintroduces a timing-based delay in a helper that callers use right before more
DrawViewinteractions. On slower BrowserStack runs this can still return too early; waiting for the canvas to be back on screen is more deterministic.Suggested fix
-import org.commcare.utils.InstrumentationUtility.sleep @@ Espresso.pressBack() - sleep(1) + onView(isRoot()) + .perform(InstrumentationUtility.waitForView(instanceOf(DrawView::class.java), 5_000, true))Also applies to: 131-131
🤖 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 `@app/instrumentation-tests/src/org/commcare/androidTests/SignatureTests.kt` at line 18, Replace the timing-based sleep(1) in the SignatureTests helper with an explicit UI wait for the DrawView/canvas to be ready instead of a fixed delay: locate the helper in SignatureTests.kt that calls InstrumentationUtility.sleep(1) before interacting with DrawView and replace it with a deterministic wait (e.g., Espresso onView(withId(...)).check(matches(isDisplayed())) or a UiAutomator/ViewAction that waits for the DrawView view to be present/visible/attached) so callers interact only after the canvas is actually on screen.
🤖 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 `@app/instrumentation-tests/src/org/commcare/androidTests/DialogTests.kt`:
- Line 51: The test is using withText(R.id.choice_dialog_panel_2) which treats
the view ID as text; update the matcher to use
withId(R.id.choice_dialog_panel_2) in DialogTests.kt (replace the withText(...)
call where the assertion with doesNotExist() is made) so the assertion targets
the view ID correctly; ensure any imports/reference to withId are available in
the test file.
In
`@app/instrumentation-tests/src/org/commcare/androidTests/FormAttachmentUploadTest.java`:
- Around line 58-62: The current test discards the ViewAction returned by
InstrumentationUtility.waitForView and never performs it, so replace the no-op
call with an actual perform: call
onView(isRoot()).perform(InstrumentationUtility.waitForView(instanceOf(DrawView.class),
5000, true)) before invoking the swipe; keep the subsequent
onView(instanceOf(DrawView.class)).perform(new GeneralSwipeAction(...)) as-is so
the DrawView is properly synchronized and present when swiped in
FormAttachmentUploadTest.
---
Nitpick comments:
In `@app/instrumentation-tests/src/org/commcare/androidTests/CaseClaimTest.java`:
- Line 23: Remove the unused static import of isRoot from CaseClaimTest.java;
locate the import statement "import static
androidx.test.espresso.matcher.ViewMatchers.isRoot;" and delete it so the file
no longer contains that unused symbol, then run a quick compile or organize
imports to ensure no other unused imports remain.
In
`@app/instrumentation-tests/src/org/commcare/androidTests/SessionExpirationTests.kt`:
- Around line 39-43: The current conditional in SessionExpirationTests.kt that
checks BuildConfig.DEBUG || AppUtils.getInstalledAppRecords().size <= 1 to
choose between By.textStartsWith("Logged Into Commcare") and
By.textStartsWith("Logged Into $APP_NAME") is brittle; instead make the
assertion use a more stable indicator (e.g., match the common prefix "Logged
Into" or check a resource id/notification content description) so the test does
not depend on installed app count or exact app name. Update the uiDevice.wait
call in SessionExpirationTests.kt (the branch using uiDevice.wait(..., 5000) and
By.textStartsWith(...)) to assert only the stable portion ("Logged Into") or
another stable UI identifier rather than branching on
BuildConfig.DEBUG/AppUtils.getInstalledAppRecords().
In `@app/instrumentation-tests/src/org/commcare/androidTests/SignatureTests.kt`:
- Line 18: Replace the timing-based sleep(1) in the SignatureTests helper with
an explicit UI wait for the DrawView/canvas to be ready instead of a fixed
delay: locate the helper in SignatureTests.kt that calls
InstrumentationUtility.sleep(1) before interacting with DrawView and replace it
with a deterministic wait (e.g., Espresso
onView(withId(...)).check(matches(isDisplayed())) or a UiAutomator/ViewAction
that waits for the DrawView view to be present/visible/attached) so callers
interact only after the canvas is actually on screen.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d92377e-c44f-4410-98ec-09f53ca580cc
📒 Files selected for processing (14)
.github/workflows/commcare-android-pr-workflow.ymlapp/instrumentation-tests/src/org/commcare/androidTests/CaseClaimTest.javaapp/instrumentation-tests/src/org/commcare/androidTests/DialogTests.ktapp/instrumentation-tests/src/org/commcare/androidTests/FormAttachmentLimitTest.ktapp/instrumentation-tests/src/org/commcare/androidTests/FormAttachmentUploadTest.javaapp/instrumentation-tests/src/org/commcare/androidTests/LoginTest.ktapp/instrumentation-tests/src/org/commcare/androidTests/ManualQuarantineTest.ktapp/instrumentation-tests/src/org/commcare/androidTests/MenuTests.ktapp/instrumentation-tests/src/org/commcare/androidTests/SessionExpirationTests.ktapp/instrumentation-tests/src/org/commcare/androidTests/SignatureTests.ktapp/instrumentation-tests/src/org/commcare/androidTests/XFormErrorTests.ktapp/instrumentation-tests/src/org/commcare/utils/InstrumentationUtility.ktapp/res/layout/entity_select_layout.xmlapp/res/values/strings.xml
| InstrumentationUtility.rotateLeft() | ||
| //TODO Expect dialog to not persist due to a activity lifecycle bug in our dialog framework. | ||
| // TODO Expect dialog to not persist due to a activity lifecycle bug in our dialog framework. | ||
| withText(R.id.choice_dialog_panel_2).doesNotExist() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify matcher misuse patterns in instrumentation tests.
# Expected: no matches for withText(R.id....)
rg -nP 'withText\s*\(\s*R\.id\.' app/instrumentation-tests/src/org/commcare/androidTestsRepository: dimagi/commcare-android
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP 'withText\s*\(\s*R\.id\.' app/instrumentation-tests/src | head -n 50
echo "----"
# Show surrounding lines for the specific occurrence
sed -n '35,70p' app/instrumentation-tests/src/org/commcare/androidTests/DialogTests.ktRepository: dimagi/commcare-android
Length of output: 1558
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP 'withText\s*\(\s*R\.id\.' app/instrumentation-tests/src | head -n 50
echo "----"
# Show surrounding lines for the specific occurrence
sed -n '35,70p' app/instrumentation-tests/src/org/commcare/androidTests/DialogTests.ktRepository: dimagi/commcare-android
Length of output: 1558
Use withId instead of withText for R.id matcher in DialogTests.kt (Line 51)
withText(R.id.choice_dialog_panel_2) uses the view-id integer as a text matcher, which can invalidate the assertion. Switch to withId(R.id.choice_dialog_panel_2).
Proposed fix
- withText(R.id.choice_dialog_panel_2).doesNotExist()
+ withId(R.id.choice_dialog_panel_2).doesNotExist()📝 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
| withText(R.id.choice_dialog_panel_2).doesNotExist() | |
| withId(R.id.choice_dialog_panel_2).doesNotExist() |
🤖 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 `@app/instrumentation-tests/src/org/commcare/androidTests/DialogTests.kt` at
line 51, The test is using withText(R.id.choice_dialog_panel_2) which treats the
view ID as text; update the matcher to use withId(R.id.choice_dialog_panel_2) in
DialogTests.kt (replace the withText(...) call where the assertion with
doesNotExist() is made) so the assertion targets the view ID correctly; ensure
any imports/reference to withId are available in the test file.
Comment on lines
58
to
62
| onView(withText("Gather Signature")) | ||
| .perform(click()); | ||
| InstrumentationUtility.waitForView(instanceOf(DrawView.class), 5000, true); | ||
| onView(instanceOf(DrawView.class)) | ||
| .perform(new GeneralSwipeAction(Swipe.SLOW, GeneralLocation.CENTER_LEFT, GeneralLocation.TOP_RIGHT, Press.FINGER)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the exact context in the test file around the reported lines
FILE="app/instrumentation-tests/src/org/commcare/androidTests/FormAttachmentUploadTest.java"
echo "---- Context: $FILE (lines 40-90) ----"
nl -ba "$FILE" | sed -n '40,90p'
# 2) Find InstrumentationUtility definition
echo "---- Locating InstrumentationUtility ----"
rg -n --hidden --no-ignore -S "class InstrumentationUtility|InstrumentationUtility" app/instrumentation-tests src . || true
# 3) Locate waitForView implementation/signature
echo "---- Locating waitForView(...) definition ----"
rg -n --hidden --no-ignore -S "waitForView\s*\(" . || true
# 4) Extract the method definition (best effort): show around matches
echo "---- Showing method bodies/usages around waitForView(...) ----"
rg -n --hidden --no-ignore -S "waitForView\s*\(" . | head -n 50
# If we can find a file that defines InstrumentationUtility, print around waitForView there
UTIL_FILE="$(rg -l --hidden --no-ignore -S "class InstrumentationUtility" . | head -n 1 || true)"
if [[ -n "${UTIL_FILE}" ]]; then
echo "---- Context: waitForView in $UTIL_FILE ----"
nl -ba "$UTIL_FILE" | sed -n '1,200p'
fi
# 5) Find other usages of waitForView(...) to see intended invocation pattern
echo "---- Other usages of InstrumentationUtility.waitForView(...) ----"
rg -n --hidden --no-ignore -S "InstrumentationUtility\.waitForView\s*\(" . | head -n 50Repository: dimagi/commcare-android
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app/instrumentation-tests/src/org/commcare/androidTests/FormAttachmentUploadTest.java"
echo "---- Context: $FILE (lines 40-90) ----"
awk 'NR>=40 && NR<=90 {printf "%5d\t%s\n", NR, $0}' "$FILE"
echo "---- Locating InstrumentationUtility ----"
rg -n --no-ignore -S "class InstrumentationUtility|InstrumentationUtility" app/instrumentation-tests src . || true
echo "---- Locating waitForView(...) definition ----"
rg -n --no-ignore -S "waitForView\s*\(" . || true
echo "---- Showing a subset of usages around waitForView(...) ----"
rg -n --no-ignore -S "InstrumentationUtility\.waitForView\s*\(" . || true
# Show around the first definition we find for InstrumentationUtility (if any)
UTIL_FILE="$(rg -l --no-ignore -S "class InstrumentationUtility" . | head -n 1 || true)"
if [[ -n "${UTIL_FILE}" ]]; then
echo "---- Context: waitForView in $UTIL_FILE ----"
awk 'NR>=1 && NR<=220 {printf "%5d\t%s\n", NR, $0}' "$UTIL_FILE"
fiRepository: dimagi/commcare-android
Length of output: 50442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the top of the test file (imports + class header)
FILE="app/instrumentation-tests/src/org/commcare/androidTests/FormAttachmentUploadTest.java"
echo "---- File header/imports: $FILE ----"
awk 'NR>=1 && NR<=120 {print NR"\t"$0}' "$FILE"
# 2) Locate waitForView in InstrumentationUtility.kt
UTIL="app/instrumentation-tests/src/org/commcare/utils/InstrumentationUtility.kt"
if [[ -f "$UTIL" ]]; then
echo "---- waitForView matches in $UTIL ----"
rg -n --no-ignore -S "fun\s+waitForView|waitForView\s*\(" "$UTIL"
echo "---- Context around waitForView definition (approx) ----"
# Print regions around any "fun waitForView" line numbers
LINES="$(rg -n --no-ignore -S "fun\s+waitForView" "$UTIL" | cut -d: -f1 | tr '\n' ' ')"
for ln in $LINES; do
start=$((ln-20)); end=$((ln+80))
echo "---- Lines $start..$end around $ln ----"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {print NR"\t"$0}' "$UTIL"
done
else
echo "InstrumentationUtility.kt not found at expected path: $UTIL"
fi
# 3) Search for usages in instrumentation tests only
echo "---- waitForView(...) usages under app/instrumentation-tests/src ----"
rg -n --no-ignore -S "InstrumentationUtility\.waitForView\s*\(|\bwaitForView\s*\(" app/instrumentation-tests/src --glob='*.{kt,java}' --glob='*.kt' --glob='*.java' | head -n 80Repository: dimagi/commcare-android
Length of output: 9126
🏁 Script executed:
rg -n "isRoot|waitForView" app/instrumentation-tests/src/org/commcare/androidTests/FormAttachmentUploadTest.javaRepository: dimagi/commcare-android
Length of output: 154
Fix the waitForView step to actually execute in Espresso
InstrumentationUtility.waitForView(...) returns a ViewAction, but in FormAttachmentUploadTest it’s created and then discarded; it’s not run via onView(...).perform(...), so there’s no synchronization before swiping the DrawView (intended usage is onView(isRoot()).perform(waitForView(...))).
Suggested fix
+import static androidx.test.espresso.matcher.ViewMatchers.isRoot;
@@
- InstrumentationUtility.waitForView(instanceOf(DrawView.class), 5000, true);
+ onView(isRoot())
+ .perform(InstrumentationUtility.waitForView(instanceOf(DrawView.class), 5000, true));📝 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
| onView(withText("Gather Signature")) | |
| .perform(click()); | |
| InstrumentationUtility.waitForView(instanceOf(DrawView.class), 5000, true); | |
| onView(instanceOf(DrawView.class)) | |
| .perform(new GeneralSwipeAction(Swipe.SLOW, GeneralLocation.CENTER_LEFT, GeneralLocation.TOP_RIGHT, Press.FINGER)); | |
| onView(withText("Gather Signature")) | |
| .perform(click()); | |
| onView(isRoot()) | |
| .perform(InstrumentationUtility.waitForView(instanceOf(DrawView.class), 5000, true)); | |
| onView(instanceOf(DrawView.class)) | |
| .perform(new GeneralSwipeAction(Swipe.SLOW, GeneralLocation.CENTER_LEFT, GeneralLocation.TOP_RIGHT, Press.FINGER)); |
🤖 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
`@app/instrumentation-tests/src/org/commcare/androidTests/FormAttachmentUploadTest.java`
around lines 58 - 62, The current test discards the ViewAction returned by
InstrumentationUtility.waitForView and never performs it, so replace the no-op
call with an actual perform: call
onView(isRoot()).perform(InstrumentationUtility.waitForView(instanceOf(DrawView.class),
5000, true)) before invoking the swipe; keep the subsequent
onView(instanceOf(DrawView.class)).perform(new GeneralSwipeAction(...)) as-is so
the DrawView is properly synchronized and present when swiped in
FormAttachmentUploadTest.
OrangeAndGreen
approved these changes
Jun 10, 2026
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
2 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.
Product Description
This PR updates the device used for BrowserStack testing because the previous test device was deprecated. The new device is the Samsung Galaxy S20 running Android 10.
Error message:
This selection evaluated other candidate devices running older versions of Android, including the Huawei P30 and Samsung Galaxy Note 9. Based on the results, the Samsung Galaxy S20 was chosen as the most suitable option. Additional details around selection criteria can be found below:
I've also tried to add 2 additional devices to the test parameters, but it failed because it exceeded the number of parallel sessions allowed by our BrowserStack subscription, which is 5.
Safety Assurance
Safety story
This only affects BrowserStack. Tests were failing on both the initial and selected devices. These issues have been resolved, and the build is now successful.
Labels and Review