-
Notifications
You must be signed in to change notification settings - Fork 6
chore(android-kotlin): add integration tests #123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📱 BrowserStack Test ResultsStatus: ✅ Passed Tested Devices:
|
📱 BrowserStack Test ResultsStatus: ✅ Passed Tested Devices:
|
📱 BrowserStack Test ResultsStatus: ✅ Passed Tested Devices:
|
9631d46
to
9b68afb
Compare
📱 BrowserStack Test ResultsStatus: ❌ Failed (Build creation failed) Expected Devices:
|
9b68afb
to
687ff0b
Compare
📱 BrowserStack Test ResultsStatus: ❌ Failed (Build creation failed) Expected Devices:
|
This PR adds browserstack integration tests to run automatically on new PR creation.
687ff0b
to
89e038e
Compare
📱 BrowserStack Test ResultsStatus: ✅ Passed Tested Devices:
|
assertNotNull(appContext) | ||
assertTrue(appContext.packageName.contains("ditto")) | ||
} catch (e: Exception) { | ||
fail("Ditto initialization failed: ${e.message}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually testing any Ditto
APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds BrowserStack integration tests to the android-kotlin project to enable automated UI testing on real devices when PRs are created. The changes include a new comprehensive UI test file that replaces the example test and a GitHub Actions workflow to build, upload, and run these tests on multiple Android devices via BrowserStack.
- Implements UI testing framework using Compose testing tools
- Adds automated BrowserStack workflow for multi-device testing
- Replaces basic example test with comprehensive UI and memory leak tests
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
TasksUITest.kt | New comprehensive UI test file with task creation and memory leak tests |
ExampleInstrumentedTest.kt | Removes the basic example test file |
android-kotlin-browserstack.yml | New GitHub Actions workflow for BrowserStack integration testing |
) | ||
) | ||
|
||
if (inputField.isDisplayed()) { |
Copilot
AI
Aug 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isDisplayed()
method doesn't exist on SemanticsNodeInteraction. Use fetchSemanticsNode()
or try-catch blocks to check node existence instead.
if (inputField.isDisplayed()) { | |
try { | |
inputField.fetchSemanticsNode() |
Copilot uses AI. Check for mistakes.
} | ||
|
||
// Force garbage collection | ||
System.gc() |
Copilot
AI
Aug 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling System.gc()
explicitly is generally discouraged as it only suggests garbage collection to the JVM and doesn't guarantee it will run. Memory leak detection should use proper profiling tools instead.
System.gc() |
Copilot uses AI. Check for mistakes.
|
||
# 2. Upload Espresso test-suite (app-debug-androidTest.apk) | ||
TEST_UPLOAD_RESPONSE=$(curl -u "$CREDS" \ | ||
-X POST "https://api-cloud.browserstack.com/app-automate/espresso/v2/test-suite" \ |
Copilot
AI
Aug 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The curl commands for uploading APKs lack proper error handling. If the upload fails, the script continues with null URLs, leading to confusing error messages later. Add immediate validation of the upload responses.
Copilot uses AI. Check for mistakes.
exit 1 | ||
fi | ||
|
||
MAX_WAIT_TIME=1800 # 30 minutes |
Copilot
AI
Aug 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The 30-minute timeout for test execution is quite long and could cause workflow delays. Consider reducing this to a more reasonable time (e.g., 15 minutes) or making it configurable based on the test complexity.
MAX_WAIT_TIME=1800 # 30 minutes | |
# Allow override of max wait time via env var, default to 900 seconds (15 minutes) | |
MAX_WAIT_TIME="${BROWSERSTACK_MAX_WAIT_TIME:-900}" |
Copilot uses AI. Check for mistakes.
📱 BrowserStack Test ResultsStatus: ✅ Passed Tested Devices:
|
… tests - Remove cruft tests (memory leak, complex UI interactions, Thread.sleep) - Replace with clean, focused tests that verify seeded documents sync with app - Use proper Compose testing patterns (waitForIdle, waitUntil, graceful failure) - Match working Kotlin PR #123 test pattern and structure Each test now simply verifies: 1. App launches successfully 2. HTTP API seeded document appears in the app UI (proving sync works) 3. Graceful handling if sync is delayed or UI differs This eliminates flaky tests and focuses on the core sync functionality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
} | ||
|
||
@Test | ||
fun testAddTaskFlow() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this works, it differs from the pattern of seeding Ditto Cloud and then checking for updates. I would recommend switching to that cause testing sync only through the UI might be misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented the Ditto Cloud document sync pattern in this new PR, along with the other build steps, based on your work here. I recommend continuing with this PR if no major problems are found.
Replace placeholder test credentials with real GitHub secrets in kotlin-multiplatform-ci.yml, matching the successful pattern from Android Kotlin PR #123: - DITTO_APP_ID from secrets instead of "test_app_id" - DITTO_PLAYGROUND_TOKEN from secrets instead of "test_token" - DITTO_AUTH_URL from secrets instead of "https://test.com" - DITTO_WEBSOCKET_URL from secrets instead of "wss://test.com" This allows the KMP app to actually connect to Ditto Cloud for real sync testing instead of failing with placeholder credentials. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…ated workflow Merged the proven working BrowserStack implementation from PR #123 into our consolidated workflow: **Key Improvements from PR #123:** - ✅ **Better Error Handling**: Validate APP_URL and TEST_URL before proceeding - ✅ **Robust Status Checking**: Handle all BrowserStack completion states (done/failed/error/passed/completed) - ✅ **Enhanced Logging**: Full API responses for debugging - ✅ **Input Validation**: Check BUILD_ID validity before monitoring - ✅ **Extended Timeout**: 30min timeout for complex test scenarios - ✅ **Device Coverage**: Added OnePlus 9 for broader testing **Workflow Integration:** - 🏗️ **Maintains Consolidation**: Still builds APKs once, reuses via artifacts - 🚀 **Production Ready**: Combines our optimized approach with proven BrowserStack logic - 🔧 **Simplified Config**: Removed test filters and env vars for cleaner execution - 📱 **4 Device Testing**: Pixel 8, Galaxy S23, Pixel 6, OnePlus 9 This gives us the best of both worlds: our efficient artifact reuse with the battle-tested BrowserStack integration from the working PR. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…rom PR #123 **Removed Cruft:** - ❌ SimpleIntegrationTest.kt (complex Ditto sync test with seeding logic) - ❌ TasksSyncIntegrationTest.kt.disabled (unused disabled test) - ❌ Document seeding and environment variable dependencies - ❌ Complex retry logic and cloud sync verification **Added Clean Test:** - ✅ TasksUITest.kt (proven working test from PR #123) - ✅ Simple UI interaction testing (add task flow) - ✅ Memory leak detection test - ✅ No external dependencies or seeding required This matches exactly what the working PR uses and removes all the experimental integration test complexity we built up.
@nickrobinson, PR not needed anymore. We can close. |
React Native, React Native Expo, and Flutter workflows had incorrect syntax with spaces in variable references: PR_NUMBER="${ github.event.pull_request.number }" This caused variables to not expand, resulting in empty PR_NUMBER values and fallback to generic "Build #123" format on BrowserStack instead of "PR-185: [title] - [message]" format. Fixed to correct syntax: PR_NUMBER="${{ github.event.pull_request.number }}" Applied to: - react-native-ci.yml (Android + iOS sections) - react-native-expo-ci.yml (Android + iOS sections) - flutter-ci.yml (Android + iOS sections)
BrowserStack does not accept # character in build names. Changed all fallback build names from "Build #123" to "Build-123" format. Changed in all 10 BrowserStack workflows: - android-cpp-ci.yml - android-java-ci.yml - android-kotlin-ci.yml - dotnet-maui-ci.yml (Android + iOS) - flutter-ci.yml (Android + iOS) - java-spring-ci.yml - kotlin-multiplatform-ci.yml - react-native-ci.yml (Android + iOS) - react-native-expo-ci.yml (Android + iOS) - swift-ci.yml
This PR adds browserstack integration tests to run automatically on new PR creation.