Add Screenshot Testing with Dropshots #71
Conversation
WalkthroughThis change introduces a comprehensive screenshot testing infrastructure for an Android project. It adds a GitHub Actions workflow for automated screenshot testing across multiple API levels, new Gradle and test dependencies, custom JUnit rules and UI tests, test manifests, and Bash scripts for local and CI test execution. Supporting configuration and ignore files are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/PR
participant GitHub as GitHub Actions
participant Workflow as dropshots-screenshot-test.yml
participant Emulator as Android Emulator
participant Test as MainTest/ScreenshotTestRule
participant Artifacts as Artifact Storage
Dev->>GitHub: Open PR with UI changes
GitHub->>Workflow: Trigger workflow on PR
Workflow->>Workflow: Detect UI changes
alt UI changed
Workflow->>Emulator: Setup and start emulator (API 27, 30, 33)
Emulator->>Test: Run instrumentation tests
Test->>Test: Capture screenshots via ScreenshotTestRule
Test->>Artifacts: Save screenshots as artifacts
Workflow->>Artifacts: Upload screenshots and test reports
Workflow->>GitHub: Post PR comment with screenshots and summary
else No UI change
Workflow-->>GitHub: Skip screenshot tests
end
sequenceDiagram
participant Script as run-screenshot-tests.sh
participant Env as Local/CI Environment
participant Emulator as Android Emulator
participant Gradle as Gradle Test Runner
participant Test as MainTest/ScreenshotTestRule
participant FS as File System
Script->>Env: Validate environment and prerequisites
Script->>Emulator: Create/start emulator for each API level
Emulator->>Gradle: Run instrumentation tests
Gradle->>Test: Execute UI tests and capture screenshots
Test->>FS: Save screenshots to local directories
Script->>Emulator: Shutdown emulator
Script->>FS: Summarize and report screenshot results
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
app/src/androidTest/AndroidManifest.xml (2)
4-10: Re-check storage-permission necessity on API 30+.Dropshots writes to app-internal storage;
WRITE_EXTERNAL_STORAGE/READ_EXTERNAL_STORAGEare scoped-storage-restricted (and ignored on API 33 without additionalREAD_MEDIA_*permissions). If not explicitly required, dropping them simplifies the manifest and avoids run-time permission prompts.- <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" /> - <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" />
16-20: Using ComponentActivity here is harmless but unused.The tests launch their own instrumentation context; this dummy activity is never started. Consider removing it to keep the test manifest minimal.
app/build.gradle.kts (1)
102-108: Duplicate Compose test artefacts inflate build graph.
ui-testandui-test-junit4should only live in one configuration each:
- JVM unit tests rarely need
ui-test; remove if unused.- Instrumentation tests already include both libs below; duplication increases build time.
scripts/run-screenshot-tests.sh (1)
305-312: Improve robustness of file renaming logicThe current implementation could be more robust with better error handling.
- cd "$output_dir" + cd "$output_dir" || { + print_error "Failed to change to directory: $output_dir" + return 1 + } + for file in *.png; do if [[ -f "$file" && "$file" != *"api$api_level"* ]]; then base_name="${file%.png}" - mv "$file" "${base_name}_api${api_level}.png" 2>/dev/null || true + new_name="${base_name}_api${api_level}.png" + if [[ ! -f "$new_name" ]]; then + mv "$file" "$new_name" || print_warning "Failed to rename: $file" + else + print_warning "Target file already exists: $new_name" + fi fi - done 2>/dev/null || true - cd - > /dev/null + done + cd - > /dev/null || true.github/workflows/dropshots-screenshot-test.yml (2)
143-148: Add file existence check before processingThe loop might fail if no PNG files exist.
cd ./screenshots/api-${{ matrix.api-level }} - for file in *.png 2>/dev/null; do + shopt -s nullglob # Make glob return empty if no matches + png_files=(*.png) + if [[ ${#png_files[@]} -eq 0 ]]; then + echo "No PNG files found to process" + else + for file in "${png_files[@]}"; do if [[ -f "$file" && "$file" != *"api${{ matrix.api-level }}"* ]]; then base_name="${file%.png}" mv "$file" "${base_name}_api${{ matrix.api-level }}.png" 2>/dev/null || true fi - done + done + fi
247-247: Remove trailing spaces in JavaScript codeMultiple lines have trailing spaces that should be cleaned up.
Run a trim operation on the affected lines or configure your editor to automatically remove trailing spaces on save. The affected lines are: 247, 254, 261, 263, 273, 296, 299, 303, 311, 316, 323-328.
Also applies to: 254-254, 261-261, 263-263, 273-273, 296-296, 299-299, 303-303, 311-311, 316-316, 323-328
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (1)
195-198: Make animation delays configurable for testingThe hardcoded delays in
LaunchedEffectslow down tests unnecessarily.Consider making the delays configurable:
@Composable -private fun CloudyMainTestContent() { +private fun CloudyMainTestContent( + animationDelay: Long = 100L +) { // ... existing code ... LaunchedEffect(Unit) { - delay(100) // Wait for initial rendering to complete + delay(animationDelay) animationPlayed = true }Then in tests, you can pass
animationDelay = 0Lor usecomposeTestRule.mainClockto control timing without actual delays.Also applies to: 290-293
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/dropshots-screenshot-test.yml(1 hunks)app/build.gradle.kts(4 hunks)app/src/androidTest/AndroidManifest.xml(1 hunks)app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt(1 hunks)build.gradle.kts(1 hunks)gradle.properties(1 hunks)gradle/libs.versions.toml(4 hunks)scripts/run-screenshot-tests.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (1)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
🧬 Code Graph Analysis (1)
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (1)
app/src/main/kotlin/com/skydoves/cloudydemo/theme/PosterTheme.kt (1)
PosterTheme(44-66)
🪛 Shellcheck (0.10.0)
scripts/run-screenshot-tests.sh
[warning] 152-152: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 253-253: \r is just literal 'r' here. For carriage return, use "$(printf '\r')" instead.
(SC1012)
[warning] 314-314: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 377-377: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[warning] 404-404: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 YAMLlint (1.37.1)
.github/workflows/dropshots-screenshot-test.yml
[error] 247-247: trailing spaces
(trailing-spaces)
[error] 254-254: trailing spaces
(trailing-spaces)
[error] 261-261: trailing spaces
(trailing-spaces)
[error] 263-263: trailing spaces
(trailing-spaces)
[error] 273-273: trailing spaces
(trailing-spaces)
[error] 296-296: trailing spaces
(trailing-spaces)
[error] 299-299: trailing spaces
(trailing-spaces)
[error] 303-303: trailing spaces
(trailing-spaces)
[error] 311-311: trailing spaces
(trailing-spaces)
[error] 316-316: trailing spaces
(trailing-spaces)
[error] 323-323: trailing spaces
(trailing-spaces)
[error] 324-324: trailing spaces
(trailing-spaces)
[error] 325-325: trailing spaces
(trailing-spaces)
[error] 328-328: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: Spotless check
- GitHub Check: API check
🔇 Additional comments (6)
build.gradle.kts (1)
12-12: Alias addition looks correct.The Dropshots plugin is declared but not applied, which is exactly what downstream modules (e.g.,
:app) expect. No issues spotted here.gradle.properties (1)
19-22: Nice touch – instrumentation runs will be cleaner and faster.Clearing package data and disabling analytics reduce flakiness and CI noise. 👍
app/build.gradle.kts (3)
28-29: Plugin applied correctly.
:appis the only module that needs Dropshots, so direct application here is appropriate.
71-83: Excellent test logging setup.Full exception format and event coverage will speed up flake triage. ✅
50-57: Update JNI packaging to use the Kotlin DSL glob syntaxThe Android Gradle Kotlin DSL does support glob patterns in
pickFirsts. Rather than hard-coding the ABI path:packaging { jniLibs { pickFirsts.add("lib/*/librenderscript-toolkit.so") } }use the Kotlin DSL’s
+=operator with a glob:android { packagingOptions { jniLibs { pickFirsts += "**/librenderscript-toolkit.so" } } }This ensures only the first
librenderscript-toolkit.sofound in any directory is packaged.Likely an incorrect or invalid review comment.
.github/workflows/dropshots-screenshot-test.yml (1)
302-307: Verify blob URL format for image displayThe current blob URL construction might not display images correctly in the PR comment.
The blob URL format should be verified. For displaying images in GitHub comments, you typically need the raw content URL, not the blob URL:
- const blobUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${context.sha}?blob_sha=${img.blobSha}`; + // For image display, use raw content URL + const imageUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/raw/${img.blobSha}`;Alternatively, consider using GitHub's user-images CDN by uploading through the issues API, which provides more reliable image hosting.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (4)
69-76: Replace Thread.sleep with proper synchronizationUsing
Thread.sleepin tests is unreliable and makes tests unnecessarily slow.@Before fun setup() { - // Verify animation is disabled and wait for initial setup - composeTestRule.activity.runOnUiThread { - // Wait for activity to be ready - Thread.sleep(100) - } + // Wait for activity to be ready using proper synchronization + composeTestRule.waitForIdle() + + // Ensure animations are disabled for consistent screenshots + composeTestRule.activity.runOnUiThread { + // Disable animations if needed + composeTestRule.mainClock.autoAdvance = false + } }
113-118: Implement proper synchronization for blur processingThe 3-second sleep is excessive and doesn't guarantee blur completion. Consider implementing a proper wait mechanism.
Replace the arbitrary sleep with a more reliable approach:
- // Wait for Cloudy animation to complete - composeTestRule.waitForIdle() - - // Additional wait for native blur processing to complete - Thread.sleep(3000) + // Wait for animations and blur to complete + composeTestRule.waitForIdle() + + // Advance clock to ensure animations complete + composeTestRule.mainClock.advanceTimeBy(1500) // Animation duration + buffer + composeTestRule.waitForIdle() + + // For native blur processing, implement a callback or use IdlingResource + composeTestRule.waitUntil(timeoutMillis = 5000) { + // Check if blur rendering is complete + // This requires exposing a state from the Cloudy library + true // Placeholder - implement actual completion check + }Consider adding an IdlingResource or callback mechanism to the Cloudy library to properly signal when blur processing is complete.
137-138: Eliminate sleep in loop for better test performanceWith 4 iterations, this adds 8+ seconds to test execution time unnecessarily.
composeTestRule.waitForIdle() - Thread.sleep(2000) // Wait for blur processing to complete + + // Use clock control for deterministic timing + composeTestRule.mainClock.autoAdvance = false + composeTestRule.mainClock.advanceTimeByFrame() + composeTestRule.waitForIdle() + + // For blur radius changes, the effect should be immediate + // If async processing is needed, implement proper synchronizationThis test is iterating through 4 blur values, so eliminating the 2-second sleep saves 8 seconds of test execution time.
155-165: Use Compose test clock for animation controlReplace multiple sleep calls with proper clock control for reliable animation testing.
- // Before animation starts (radius = 0) - composeTestRule.waitForIdle() - Thread.sleep(500) + // Control animation timing programmatically + composeTestRule.mainClock.autoAdvance = false + composeTestRule.waitForIdle() dropshots.assertSnapshot( view = composeTestRule.activity.findViewById(android.R.id.content), name = "cloudy_animation_start_api${Build.VERSION.SDK_INT}" ) - // After animation completes (radius = 45) - Thread.sleep(3000) // Wait for animation to complete + // Advance time to complete animation + // Animation spec: 2000ms duration + 1000ms delay = 3000ms total + composeTestRule.mainClock.advanceTimeBy(3500) + composeTestRule.waitForIdle() dropshots.assertSnapshot( view = composeTestRule.activity.findViewById(android.R.id.content), name = "cloudy_animation_end_api${Build.VERSION.SDK_INT}" ).github/workflows/dropshots-screenshot-test.yml (1)
102-102: Fix carriage return removal syntax in workflowSame issue as in the shell script - incorrect syntax for carriage return removal.
- adb wait-for-device shell 'while [[ -z $(getprop sys.boot_completed | tr -d '\r') ]]; do sleep 1; done' + adb wait-for-device shell 'while [[ -z $(getprop sys.boot_completed | tr -d '\''\r'\'') ]]; do sleep 1; done'
🧹 Nitpick comments (2)
.github/workflows/dropshots-screenshot-test.yml (2)
80-80: Clarify architecture selection logicThe architecture selection logic is functional but could be more readable.
Consider improving readability:
- arch: ${{ matrix.api-level == 27 && 'x86_64' || (runner.arch == 'ARM64' && 'arm64-v8a' || 'x86_64') }} + arch: ${{ matrix.api-level == 27 && 'x86_64' || (runner.arch == 'ARM64' && 'arm64-v8a' || 'x86_64') }}Or use a more explicit approach:
arch: ${{ matrix.api-level == 27 && 'x86_64' || (runner.arch == 'ARM64' && 'arm64-v8a' || 'x86_64') }}
237-237: Remove trailing spacesStatic analysis detected trailing spaces that should be cleaned up.
- const content = fs.readFileSync(screenshot.path, 'base64'); + const content = fs.readFileSync(screenshot.path, 'base64'); - + - const { data: blob } = await github.rest.git.createBlob({ + const { data: blob } = await github.rest.git.createBlob({Apply similar cleanup to all lines with trailing spaces identified by the linter.
Also applies to: 244-244, 251-251, 253-253, 263-263, 286-286, 289-289, 293-293, 301-301, 306-306, 313-313, 314-314, 315-315, 318-318
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/dropshots-screenshot-test.yml(1 hunks)app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (1)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
🪛 YAMLlint (1.37.1)
.github/workflows/dropshots-screenshot-test.yml
[error] 237-237: trailing spaces
(trailing-spaces)
[error] 244-244: trailing spaces
(trailing-spaces)
[error] 251-251: trailing spaces
(trailing-spaces)
[error] 253-253: trailing spaces
(trailing-spaces)
[error] 263-263: trailing spaces
(trailing-spaces)
[error] 286-286: trailing spaces
(trailing-spaces)
[error] 289-289: trailing spaces
(trailing-spaces)
[error] 293-293: trailing spaces
(trailing-spaces)
[error] 301-301: trailing spaces
(trailing-spaces)
[error] 306-306: trailing spaces
(trailing-spaces)
[error] 313-313: trailing spaces
(trailing-spaces)
[error] 314-314: trailing spaces
(trailing-spaces)
[error] 315-315: trailing spaces
(trailing-spaces)
[error] 318-318: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: screenshot-tests (33)
- GitHub Check: screenshot-tests (30)
- GitHub Check: build
- GitHub Check: API check
- GitHub Check: Spotless check
🔇 Additional comments (8)
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (3)
78-103: Excellent API-specific test coverageThe conditional test execution based on API levels is well-structured and ensures comprehensive coverage across different Android versions.
174-242: Well-structured composable test componentsThe test composables are well-designed with proper state management and clear visual feedback for different test scenarios.
244-320: Comprehensive test component coverageThe additional test components effectively cover different blur scenarios and animation states, providing good visual regression coverage.
.github/workflows/dropshots-screenshot-test.yml (5)
1-15: Well-structured workflow configurationThe workflow triggers and environment setup are properly configured for efficient CI execution.
17-33: Efficient change detection strategyThe change detection job effectively prevents unnecessary test execution, optimizing CI resource usage.
64-86: Excellent caching strategy for performanceThe multi-layer caching approach (Gradle dependencies + AVD snapshots) will significantly reduce build and emulator boot times.
192-336: Impressive PR comment generation with GitHub APIThe JavaScript code for creating detailed PR comments with embedded screenshots is well-implemented. The logic for handling existing comments and organizing screenshots by API level is excellent.
112-116: Robust test execution with proper error handlingThe Gradle test execution includes appropriate flags and error handling to ensure tests complete even with issues.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/run-screenshot-tests.sh (1)
257-268: Returning a PID viareturnbreaks – Bash only returns 0-255
return $emulator_pidtruncates or even wraps PIDs (>255) and makes$?useless.
Outside the function you then capture$!, which is the wrong process (it refers to the most recent background job in the caller).Minimal fix – just drop the
returnand echo the PID:- print_success "Emulator API $api_level is ready!" - return $emulator_pid + print_success "Emulator API $api_level is ready!" + echo $emulator_pidand in
main:- start_emulator $api - emulator_pid=$! + emulator_pid=$(start_emulator $api)
🧹 Nitpick comments (2)
.github/workflows/dropshots-screenshot-test.yml (2)
79-81: Expression readability / maintainabilityThe nested ternary in the
arch:andtarget:expressions is hard to read and easy to break. Extract into separate matrix variables or at least split into two simpleifexpressions.Small but effective refactor:
strategy: matrix: api-level: [27,30,33] arch: - ${{ (runner.arch == 'ARM64' && 'arm64-v8a') || 'x86_64' }} target: - ${{ (matrix.api-level == 27 && 'default') || 'google_apis' }}Also applies to: 94-97
142-150: Artifact scope is too broad
path: screenshots/uploads the entire directory for every matrix job.
When several API-level jobs run in parallel they will each re-upload the same folder name, causing collisions and larger storage usage. Point the path directly at the API-specific sub-folder:- path: screenshots/ + path: screenshots/api-${{ matrix.api-level }}/
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/dropshots-screenshot-test.yml(1 hunks)app/build.gradle.kts(4 hunks)app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt(1 hunks)gradle/libs.versions.toml(4 hunks)scripts/run-screenshot-tests.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- gradle/libs.versions.toml
- app/build.gradle.kts
- app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
scripts/run-screenshot-tests.sh (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
.github/workflows/dropshots-screenshot-test.yml (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
🪛 YAMLlint (1.37.1)
.github/workflows/dropshots-screenshot-test.yml
[error] 236-236: trailing spaces
(trailing-spaces)
[error] 243-243: trailing spaces
(trailing-spaces)
[error] 250-250: trailing spaces
(trailing-spaces)
[error] 252-252: trailing spaces
(trailing-spaces)
[error] 262-262: trailing spaces
(trailing-spaces)
[error] 285-285: trailing spaces
(trailing-spaces)
[error] 288-288: trailing spaces
(trailing-spaces)
[error] 292-292: trailing spaces
(trailing-spaces)
[error] 300-300: trailing spaces
(trailing-spaces)
[error] 305-305: trailing spaces
(trailing-spaces)
[error] 312-312: trailing spaces
(trailing-spaces)
[error] 313-313: trailing spaces
(trailing-spaces)
[error] 314-314: trailing spaces
(trailing-spaces)
[error] 317-317: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: screenshot-tests (33)
- GitHub Check: screenshot-tests (27)
- GitHub Check: screenshot-tests (30)
- GitHub Check: build
- GitHub Check: Spotless check
- GitHub Check: API check
🔇 Additional comments (2)
scripts/run-screenshot-tests.sh (2)
382-388: Array-membership check fixed – thanksThe quoted RHS was removed; the regex now works as intended. 👍
145-148:page_sizeparsing will include the literal “bytes)”
vm_statprints:page size of 4096 bytes.
awk '{print $8}'returnsbytes, not4096. This makes the bc expression fail.- page_size=$(vm_stat | grep "page size" | awk '{print $8}') + page_size=$(vm_stat | grep "page size" | grep -oE '[0-9]+')Also guard against
bcabsence or replace with pure bash arithmetic.Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
scripts/run-screenshot-tests.sh (1)
431-433: Array-membership check still risks false positives
[[ " ${API_LEVELS[*]} " =~ ${SELECTED_API} ]]matches substrings (e.g.3matches30).
A small helper avoids surprises:contains() { local needle=$1; shift for item; do [[ "$item" == "$needle" ]] && return 0; done return 1 } if contains "$SELECTED_API" "${API_LEVELS[@]}"; then … fi.github/workflows/dropshots-screenshot-test.yml (1)
129-131: Still swallowing test failures – previous feedback not applied
|| echo "Tests completed with issues"converts a non-zero exit from Gradle
into success, so CI stays green even when instrumentation tests fail.
Drop the guard (or usecontinue-on-error: trueexplicitly).- ./gradlew :app:connectedDebugAndroidTest --no-daemon --no-build-cache \ - -Pandroid.testInstrumentationRunnerArguments.class=com.skydoves.cloudydemo.MainTest || \ - echo "Tests completed with issues" + ./gradlew :app:connectedDebugAndroidTest --no-daemon --no-build-cache \ + -Pandroid.testInstrumentationRunnerArguments.class=com.skydoves.cloudydemo.MainTestAlso applies to: 276-278
🧹 Nitpick comments (4)
scripts/run-screenshot-tests-api27.sh (1)
38-45: Separate declaration & assignment to avoid SC2155 masking
local arch=$(uname -m)masks any failure ofuname -mbecause the exit status oflocalis returned. Split into two lines.- local arch=$(uname -m) + local arch + arch=$(uname -m)scripts/run-screenshot-tests.sh (1)
175-183: Same SC2155 issue forarchincheck_emulator
Echoing the pattern from the API-27 script:- local arch=$(uname -m) + local arch + arch=$(uname -m)This keeps
set -emeaningful..github/workflows/dropshots-screenshot-test.yml (2)
70-97: Heavy duplication – extract a reusable step or composite actionAVD creation, emulator boot-wait logic, screen-setup, and screenshot collection
are nearly identical between the API-27 and matrix jobs. Factor this into a
reusable composite action or at least a YAML anchor to cut ~150 lines, making
future maintenance (timeouts, memory tweaks, etc.) one-liner edits.This keeps the PR focused yet prevents drift across jobs.
Also applies to: 218-244
398-479: Trailing whitespace flagged by yamllintMultiple lines contain trailing spaces, which yamllint already reports. Trim to
keep the workflow lint-clean.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/dropshots-screenshot-test.yml(1 hunks)app/build.gradle.kts(4 hunks)gradle/libs.versions.toml(2 hunks)scripts/run-screenshot-tests-api27.sh(1 hunks)scripts/run-screenshot-tests.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/build.gradle.kts
- gradle/libs.versions.toml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
scripts/run-screenshot-tests.sh (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
.github/workflows/dropshots-screenshot-test.yml (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
🧬 Code Graph Analysis (2)
scripts/run-screenshot-tests-api27.sh (1)
scripts/run-screenshot-tests.sh (15)
print_status(21-23)print_error(33-35)print_success(25-27)check_android_sdk(60-96)check_required_tools(99-131)check_system_requirements(134-168)print_warning(29-31)create_avd(201-237)collect_screenshots(343-371)check_setup(38-57)check_emulator(171-198)start_emulator(240-318)run_tests(321-340)cleanup_emulator(374-378)main(381-465)
scripts/run-screenshot-tests.sh (1)
scripts/run-screenshot-tests-api27.sh (15)
print_status(20-22)print_error(32-34)check_android_sdk(73-109)check_required_tools(112-144)check_system_requirements(147-181)print_success(24-26)print_warning(28-30)create_avd(203-227)collect_screenshots(315-342)check_setup(51-70)check_emulator(184-200)start_emulator(230-292)run_tests(295-312)cleanup_emulator(345-349)main(352-402)
🪛 Shellcheck (0.10.0)
scripts/run-screenshot-tests-api27.sh
[warning] 40-40: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/run-screenshot-tests.sh
[warning] 177-177: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 YAMLlint (1.37.1)
.github/workflows/dropshots-screenshot-test.yml
[error] 398-398: trailing spaces
(trailing-spaces)
[error] 405-405: trailing spaces
(trailing-spaces)
[error] 412-412: trailing spaces
(trailing-spaces)
[error] 414-414: trailing spaces
(trailing-spaces)
[error] 424-424: trailing spaces
(trailing-spaces)
[error] 447-447: trailing spaces
(trailing-spaces)
[error] 450-450: trailing spaces
(trailing-spaces)
[error] 454-454: trailing spaces
(trailing-spaces)
[error] 462-462: trailing spaces
(trailing-spaces)
[error] 467-467: trailing spaces
(trailing-spaces)
[error] 474-474: trailing spaces
(trailing-spaces)
[error] 475-475: trailing spaces
(trailing-spaces)
[error] 476-476: trailing spaces
(trailing-spaces)
[error] 479-479: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: screenshot-tests (30)
- GitHub Check: screenshot-tests (33)
- GitHub Check: screenshot-tests-api27
- GitHub Check: build
- GitHub Check: API check
- GitHub Check: Spotless check
🔇 Additional comments (1)
scripts/run-screenshot-tests-api27.sh (1)
208-214: Hard-coded architecture may break on AArch64 hosts
uname -mreturnsaarch64on many Linux ARM machines, but you translate only"arm64"→arm64-v8a. Consider normalising botharm64andaarch64(and possiblyarmv8*) to avoid “system-image not found” errors.case "$(uname -m)" in arm64|aarch64) arch="arm64-v8a" ;; *) arch="x86_64" ;; esac
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/dropshots-screenshot-test.yml (2)
129-131: Don't mask test failures – drop the|| echoguard
|| echo "Tests completed with issues"forces the step to succeed even when Gradle exits with a non-zero status, so CI stays green while tests actually fail.- ./gradlew :app:connectedDebugAndroidTest --no-daemon --no-build-cache -Pandroid.testInstrumentationRunnerArguments.class=com.skydoves.cloudydemo.MainTest || echo "Tests completed with issues" + ./gradlew :app:connectedDebugAndroidTest --no-daemon --no-build-cache \ + -Pandroid.testInstrumentationRunnerArguments.class=com.skydoves.cloudydemo.MainTestIf you truly want to tolerate failures, use
continue-on-error: trueon the step instead.Also applies to: 276-278
166-175: Prevent secondary failures when artifacts are missing
actions/upload-artifacterrors out when paths don’t exist (e.g., when Gradle aborted early).
Addif-no-files-found: ignoreso that the original test failure remains the only red signal.- with: + with: name: test-reports-api-27 path: | app/build/reports/androidTests/ app/build/outputs/androidTest-results/ + if-no-files-found: ignoreApply the same addition to the matrix block (API 30/33).
Also applies to: 313-321
🧹 Nitpick comments (1)
.github/workflows/dropshots-screenshot-test.yml (1)
395-405: Strip trailing whitespace to satisfy YAML lintersYAMLlint flags several lines here for trailing spaces.
Not critical for execution, but cleaning them up keeps the workflow lint-clean and avoids noisy warnings.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/dropshots-screenshot-test.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
.github/workflows/dropshots-screenshot-test.yml (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
🪛 YAMLlint (1.37.1)
.github/workflows/dropshots-screenshot-test.yml
[error] 398-398: trailing spaces
(trailing-spaces)
[error] 405-405: trailing spaces
(trailing-spaces)
[error] 412-412: trailing spaces
(trailing-spaces)
[error] 414-414: trailing spaces
(trailing-spaces)
[error] 424-424: trailing spaces
(trailing-spaces)
[error] 447-447: trailing spaces
(trailing-spaces)
[error] 450-450: trailing spaces
(trailing-spaces)
[error] 454-454: trailing spaces
(trailing-spaces)
[error] 462-462: trailing spaces
(trailing-spaces)
[error] 467-467: trailing spaces
(trailing-spaces)
[error] 474-474: trailing spaces
(trailing-spaces)
[error] 475-475: trailing spaces
(trailing-spaces)
[error] 476-476: trailing spaces
(trailing-spaces)
[error] 479-479: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: screenshot-tests (33)
- GitHub Check: screenshot-tests-api27
- GitHub Check: screenshot-tests (30)
- GitHub Check: API check
- GitHub Check: build
- GitHub Check: Spotless check
🔇 Additional comments (1)
.github/workflows/dropshots-screenshot-test.yml (1)
103-106: Previous carriage-return issue resolved – looks goodThe loop now checks
sys.boot_completedwithout the problematictr -d '\r'construct.
Good fix.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
scripts/run-screenshot-tests.sh (1)
145-148: Samebcdependency issue as the API 27 scriptThe fallback to
bcwill terminate the script on images wherebcis not present.
Reuse the defensive pattern suggested for the other script.
🧹 Nitpick comments (5)
scripts/run-screenshot-tests-api27.sh (1)
92-96: Hard-codedcmdline-tools/latestpath is brittleSome SDK installs keep the command-line tools under a versioned directory (
cmdline-tools/12.0/bin) without thelatestsymlink.
Consider resolving the newest available directory instead of assuminglatest.scripts/run-screenshot-tests.sh (1)
406-436: Membership check helper is re-defined on every invocationDefining the
contains()function inside the conditional branch recreates the function each timemainis executed and leaks it into the global namespace.Move it to the top-level (next to the other helpers) and mark it
readonly -fif you want to prevent tampering..github/workflows/dropshots-screenshot-test.yml (3)
59-67: Numeric comparison is evaluated before matrix substitution – shellcheck false positive
[[ ${{ matrix.api-level }} -eq 27 ]]is expanded to[[ 27 -eq 27 ]], which is valid.
However, actionlint’s SC2309 warning can be silenced and portability improved by quoting the value and using string comparison:if [[ "${{ matrix.api-level }}" == '27' ]]; thenNot mandatory but removes the warning.
554-572: Large loop uploads only first 20 images – comment says “total screenshots”The script uploads at most 20 screenshots but later reports the count of all screenshots. Either:
- Upload them all (beware size limits), or
- Report the truncated number to avoid misleading statistics.
470-479: Trailing spaces and YAML lint errorsMultiple lines flagged by yamllint for trailing whitespace; they don’t break the workflow but cause noise in CI linters.
Run
yamllint -d relaxed .github/workflows/dropshots-screenshot-test.ymland trim the highlighted lines.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/dropshots-screenshot-test.yml(1 hunks)app/build.gradle.kts(4 hunks)app/src/androidTest/AndroidManifest.xml(1 hunks)scripts/run-screenshot-tests-api27.sh(1 hunks)scripts/run-screenshot-tests.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/androidTest/AndroidManifest.xml
- app/build.gradle.kts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
scripts/run-screenshot-tests.sh (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
.github/workflows/dropshots-screenshot-test.yml (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
🪛 actionlint (1.7.7)
.github/workflows/dropshots-screenshot-test.yml
59-59: shellcheck reported issue in this script: SC2309:warning:2:7: -eq treats this as a variable. Use = to compare as string (or expand explicitly with $var)
(shellcheck)
366-366: if: condition "hashFiles('prebuilt-avd/avd-android-${{ matrix.api-level }}.tar.gz') == ''" is always evaluated to true because extra characters are around ${{ }}
(if-cond)
370-370: if: condition "hashFiles('prebuilt-avd/avd-android-${{ matrix.api-level }}.tar.gz') == ''" is always evaluated to true because extra characters are around ${{ }}
(if-cond)
374-374: if: condition "hashFiles('prebuilt-avd/avd-android-${{ matrix.api-level }}.tar.gz') == ''" is always evaluated to true because extra characters are around ${{ }}
(if-cond)
🪛 YAMLlint (1.37.1)
.github/workflows/dropshots-screenshot-test.yml
[error] 556-556: trailing spaces
(trailing-spaces)
[error] 563-563: trailing spaces
(trailing-spaces)
[error] 570-570: trailing spaces
(trailing-spaces)
[error] 572-572: trailing spaces
(trailing-spaces)
[error] 605-605: trailing spaces
(trailing-spaces)
[error] 608-608: trailing spaces
(trailing-spaces)
[error] 612-612: trailing spaces
(trailing-spaces)
[error] 620-620: trailing spaces
(trailing-spaces)
[error] 625-625: trailing spaces
(trailing-spaces)
[error] 632-632: trailing spaces
(trailing-spaces)
[error] 637-637: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: screenshot-tests (33)
- GitHub Check: screenshot-tests (30)
- GitHub Check: screenshot-tests-api27
- GitHub Check: Spotless check
- GitHub Check: build
- GitHub Check: API check
- Add prebuild avd
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (4)
116-122: Replace repeated Thread.sleep with proper synchronizationThe test still uses multiple
Thread.sleep(500)calls in a loop, creating unnecessary delays and potential flakiness.Consider using Compose test clock control instead:
- repeat(6) { i -> - Thread.sleep(500) - println("Waiting for Compose to render... (${i + 1}/6)") - } - - // Additional wait for any background operations - Thread.sleep(1000) + // Use proper synchronization + composeTestRule.waitForIdle() + + // If additional timing is needed, use controlled advancement + composeTestRule.mainClock.advanceTimeBy(3000) + composeTestRule.waitForIdle()
134-134: Avoid nested Thread.sleep in error handlingAdding more sleep time in the retry logic compounds the performance issue and doesn't guarantee success.
- Thread.sleep(2000) + // Use proper wait mechanism instead of arbitrary delay + composeTestRule.waitForIdle()
163-166: Replace progressive sleep delays in blur state testsMultiple
Thread.sleep(500)calls in loops create cumulative delays without guaranteeing proper state.- repeat(4) { i -> - Thread.sleep(500) - println("Waiting for initial state... (${i + 1}/4)") - } + // Wait for initial blur state to stabilize + composeTestRule.waitForIdle() + composeTestRule.mainClock.advanceTimeBy(2000) + composeTestRule.waitForIdle()
212-215: Use deterministic timing for animation state testsProgressive sleep delays make tests slower and less reliable.
- repeat(4) { i -> - Thread.sleep(500) - println("Waiting for animation start... (${i + 1}/4)") - } + // Control animation timing precisely + composeTestRule.mainClock.autoAdvance = false + composeTestRule.waitForIdle()scripts/run-screenshot-tests.sh (1)
168-169: Variable declaration and assignment should be separatedThe current pattern combines declaration and assignment, which can mask command failures.
- local available_disk - available_disk=$(df . | awk 'NR==2{printf "%.0f", $4/1024/1024}') + local available_disk + available_disk=$(df . | awk 'NR==2{printf "%.0f", $4/1024/1024}')Note: This appears to already be correctly implemented in the current code.
🧹 Nitpick comments (4)
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (1)
58-64: Improve error handling for permission grantsThe current try-catch silently continues on permission failures, which could lead to test failures later.
try { device.executeShellCommand("pm grant ${context.packageName} $permission") println("Granted permission: $permission") } catch (e: Exception) { - println("Warning: Could not grant permission $permission: ${e.message}") + println("Warning: Could not grant permission $permission: ${e.message}") + // Consider failing fast if critical permissions can't be granted + if (permission in listOf(Manifest.permission.WRITE_EXTERNAL_STORAGE, Manifest.permission.READ_EXTERNAL_STORAGE)) { + throw e + } }scripts/run-screenshot-tests.sh (1)
446-446: Consider improving array membership checkThe current
containsfunction implementation is good, but the usage pattern could be more explicit.The current implementation using the
containsfunction is correct and readable. This is already much better than the problematic regex approach that was previously flagged..github/workflows/dropshots-screenshot-test.yml (2)
60-69: Shell-check warning: use quoted string comparison for matrix variable
[[ ${{ matrix.api-level }} -eq 27 ]]triggers SC2309 because the string27is expanded without quoting.
Change to string compare (no functional change, silences linter):-if [[ ${{ matrix.api-level }} -eq 27 ]]; then +if [[ "${{ matrix.api-level }}" == "27" ]]; thenKeeps the script clean and eliminates the warning emitted by
actionlint.
341-350: Conditional AVD creation still runs on every job
if: ${{ hashFiles(format('prebuilt-avd/avd-android-{0}.tar.gz', matrix.api-level)) == '' }}
works, but thedownload-artifactstep above stores the archive asprebuilt-avd/avd-android-${{ matrix.api-level }}.tar.gz.
If extraction fails or no file is present, the cache condition is fine; however, when extraction succeeds the hash changes (it becomes a directory tree) and the expression again evaluates to''.Safer: test the file directly in the workflow expression instead of the directory hash.
if: ${{ !exists(format('prebuilt-avd/avd-android-{0}.tar.gz', matrix.api-level)) }}Prevents unnecessary SDK installs/AVD creation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/dropshots-screenshot-test.yml(1 hunks)app/build.gradle.kts(4 hunks)app/src/androidTest/AndroidManifest.xml(1 hunks)app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt(1 hunks)scripts/run-screenshot-tests-api27.sh(1 hunks)scripts/run-screenshot-tests.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/build.gradle.kts
- scripts/run-screenshot-tests-api27.sh
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
scripts/run-screenshot-tests.sh (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
.github/workflows/dropshots-screenshot-test.yml (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
🪛 actionlint (1.7.7)
.github/workflows/dropshots-screenshot-test.yml
59-59: shellcheck reported issue in this script: SC2309:warning:2:7: -eq treats this as a variable. Use = to compare as string (or expand explicitly with $var)
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: screenshot-tests (30)
- GitHub Check: screenshot-tests (33)
- GitHub Check: screenshot-tests-api27
- GitHub Check: API check
- GitHub Check: build
- GitHub Check: Spotless check
🔇 Additional comments (5)
scripts/run-screenshot-tests.sh (5)
382-383: Variable declaration properly separatedGood implementation of separated declaration and assignment to avoid masking command errors.
240-240: Good practice: Automatic license acceptanceThe script correctly handles unattended execution by piping
yestosdkmanagerto automatically accept licenses.
331-331: Appropriate pattern for returning PID from functionThe function correctly echoes the PID instead of using
returnwith the PID value, which would be limited to exit codes 0-255.
434-434: Proper error propagation implementedThe script correctly exits with non-zero status when tests fail, ensuring CI pipelines can detect failures.
152-160: Robust macOS memory calculation with fallbacksThe memory calculation correctly accounts for page size and includes proper fallback handling when
bcis not available.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
app/src/androidTest/AndroidManifest.xml (1)
9-10: Remove unused ACCESS_NETWORK_STATE permission from Android test manifestThe
ACCESS_NETWORK_STATEpermission isn't referenced anywhere in your test code and isn't needed for screenshot testing. You can safely remove it to keep the manifest minimal.app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (3)
116-122: Replace Thread.sleep with proper synchronizationUsing multiple
Thread.sleepcalls creates unreliable tests and unnecessary delays. Consider implementing proper wait mechanisms or IdlingResources to detect when the UI is truly ready for screenshot capture.
163-166: Eliminate sleep in loop for better test performanceWith multiple iterations, this adds significant time to test execution unnecessarily. Consider using proper synchronization mechanisms to wait for specific UI states rather than arbitrary delays.
Also applies to: 180-183
212-215: Use proper timing control for animation testingReplace multiple sleep calls with deterministic timing mechanisms to make animation tests more reliable and faster.
Also applies to: 229-232
scripts/run-screenshot-tests.sh (1)
254-332: Stdout mixing causes unusable$emulator_pid(same as API-27 script)All log lines + the PID are captured, producing an invalid value.
Redirect logs to stderr or echo only the PID as shown in the previous comment.app/build.gradle.kts (1)
41-44: Incorrect instrumentation runner and argument key – tests won’t startDropshots requires its own runner and uses a dotted argument key.
- testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" - testInstrumentationRunnerArguments["dropshots_threshold"] = "0.15" + // Dropshots runner with threshold argument + testInstrumentationRunner = "com.dropbox.dropshots.DropshotsTestRunner" + testInstrumentationRunnerArguments["dropshots.threshold"] = "0.15"Without this change the instrumentation process crashes before running any test.
.github/workflows/dropshots-screenshot-test.yml (1)
560-567: Screenshot URLs are still broken – use the raw blob or data-URIThis exact problem was flagged earlier; the constructed link does not point to downloadable content, so images won’t render in the PR comment.
- const blobUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${context.sha}?blob_sha=${img.blobSha}`; + // Publish via the GitHub raw blob API + const blobUrl = `https://api.github.com/repos/${context.repo.owner}/${context.repo.repo}/git/blobs/${img.blobSha}`;Or embed inline:
+ const blobUrl = `data:image/png;base64,${content}`;Without this change reviewers will still see broken images.
🧹 Nitpick comments (3)
scripts/run-screenshot-tests-api27.sh (1)
350-358: Useshopt -s nullglobto avoid looping on literal*.pngWhen no screenshots are pulled the pattern remains literal and the
for file in *.pngloop runs once with the string"*.png".
Settingnullglobmakes the pattern vanish when there are no matches:shopt -s nullglob for file in *.png; do … done shopt -u nullglobscripts/run-screenshot-tests.sh (1)
472-479: SC2155 still present – declare & assign separately
local count=$( … )masks command-substitution failures.
Split into two lines as already done elsewhere in the script.- local count - count=$(find "./screenshots/api-$api" -name "*.png" 2>/dev/null | wc -l) + local count + count=$(find "./screenshots/api-$api" -name "*.png" 2>/dev/null | wc -l).github/workflows/dropshots-screenshot-test.yml (1)
355-359:runner.archvalues are “X64 / ARM64”, not lowercaseThe ternary uses
'ARM64'on the left side but lowercase'arm64-v8a'/'x86_64'on the right.
Mixing case works, but the first comparison should reflect the real value for readability and future-proofing:- arch: ${{ runner.arch == 'ARM64' && 'arm64-v8a' || 'x86_64' }} + arch: ${{ runner.arch == 'ARM64' && 'arm64-v8a' || 'x86_64' }}(Only a stylistic nit – functional behaviour is unchanged.)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/dropshots-screenshot-test.yml(1 hunks)app/build.gradle.kts(4 hunks)app/src/androidTest/AndroidManifest.xml(1 hunks)app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt(1 hunks)build.gradle.kts(1 hunks)gradle.properties(1 hunks)gradle/libs.versions.toml(2 hunks)scripts/run-screenshot-tests-api27.sh(1 hunks)scripts/run-screenshot-tests.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
gradle/libs.versions.toml (1)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
scripts/run-screenshot-tests.sh (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
.github/workflows/dropshots-screenshot-test.yml (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
🧬 Code Graph Analysis (1)
scripts/run-screenshot-tests-api27.sh (1)
scripts/run-screenshot-tests.sh (15)
print_status(29-31)print_error(41-43)print_success(33-35)check_android_sdk(68-104)check_required_tools(107-139)check_system_requirements(142-181)print_warning(37-39)create_avd(215-251)collect_screenshots(357-385)check_setup(46-65)check_emulator(184-212)start_emulator(254-332)run_tests(335-354)cleanup_emulator(388-392)main(395-480)
🪛 actionlint (1.7.7)
.github/workflows/dropshots-screenshot-test.yml
59-59: shellcheck reported issue in this script: SC2309:warning:2:7: -eq treats this as a variable. Use = to compare as string (or expand explicitly with $var)
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: screenshot-tests (30)
- GitHub Check: screenshot-tests (33)
- GitHub Check: screenshot-tests-api27
- GitHub Check: build
🔇 Additional comments (8)
build.gradle.kts (1)
12-12: LGTM! Proper plugin declaration for multi-module setup.The Dropshots plugin is correctly declared with
apply falsein the root build script, following Gradle best practices for multi-module projects.gradle.properties (1)
19-22: LGTM! Appropriate test optimizations for screenshot testing.The instrumentation runner arguments correctly configure clean test environments by clearing package data and disabling analytics, which are essential for reliable screenshot testing.
gradle/libs.versions.toml (2)
22-22: LGTM! Clean Dropshots integration in version catalog.The Dropshots version, plugin, and library entries are properly configured with consistent version references. This avoids the duplicate version key issues and follows Gradle version catalog best practices.
Also applies to: 34-34, 39-39
15-15: Well-structured AndroidX test dependencies.The new test dependencies (androidxTestRules, uiTestJunit4, androidx-junit, androidx-ui-test-manifest, androidx-compose-ui-test-junit4) are properly organized and will support comprehensive UI testing with Dropshots.
Also applies to: 23-23, 37-39, 52-52, 56-56
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (3)
45-76: LGTM! Comprehensive permission setup for screenshot testing.The setup method properly handles permissions across different API levels with appropriate error handling and logging.
78-105: Well-designed API-specific test structure.The conditional tests based on API levels and the generic fallback test provide good coverage across different Android versions.
124-145: LGTM! Good error handling and retry logic.The screenshot capture includes proper error handling with retry attempts and comprehensive logging, which will help with debugging test failures.
app/build.gradle.kts (1)
50-57:pickFirstsexclusion can hide native libs on some ABIsPutting
librenderscript-toolkit.soin bothpickFirstsandexcludes
means the library is silently dropped on ABIs where it is the only match,
causingUnsatisfiedLinkErrorat runtime.Verify that the toolkit is indeed duplicated across ABIs; otherwise keep only
pickFirsts.
| set -e | ||
|
|
||
| echo "🔧 Starting Cloudy Library Screenshot Tests for API 27..." | ||
|
|
There was a problem hiding this comment.
Add pipefail to avoid silent errors in pipelines
set -e doesn’t catch failures in the left-hand side of a pipeline.
If adb shell … fails (e.g. because the device is offline) the script will silently continue.
Enabling pipefail makes the script abort on the first failing command in a pipeline.
-set -e
+set -euo pipefail📝 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.
| set -e | |
| echo "🔧 Starting Cloudy Library Screenshot Tests for API 27..." | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| echo "🔧 Starting Cloudy Library Screenshot Tests for API 27..." | |
| # …rest of the script… |
🤖 Prompt for AI Agents
In scripts/run-screenshot-tests-api27.sh around lines 6 to 9, the script uses
'set -e' but does not enable 'pipefail', which means failures in the left side
of pipelines can be missed. Add 'set -o pipefail' after 'set -e' to ensure the
script aborts on any failure within a pipeline command, preventing silent
errors.
| print_error "Emulator boot timeout after ${boot_timeout}s" | ||
| return 1 | ||
| fi | ||
|
|
||
| print_status "Emulator booted in ${elapsed}s" | ||
|
|
||
| # Wait a bit more for system to stabilize | ||
| sleep 10 | ||
|
|
||
| # Unlock screen | ||
| adb shell input keyevent 82 | ||
|
|
||
| # Set resolution | ||
| adb shell wm size 1080x1920 | ||
| adb shell wm density 440 | ||
|
|
||
| print_success "Emulator API $API_LEVEL is ready!" | ||
| echo $emulator_pid | ||
| } | ||
|
|
||
| # Run tests | ||
| run_tests() { | ||
| print_status "Running Dropshots tests for API $API_LEVEL..." | ||
|
|
||
| # Execute tests | ||
| ./gradlew :app:connectedDebugAndroidTest \ | ||
| --no-daemon \ | ||
| --no-build-cache \ | ||
| -Pandroid.testInstrumentationRunnerArguments.class=com.skydoves.cloudydemo.MainTest \ | ||
| || { | ||
| print_error "Tests failed for API $API_LEVEL" | ||
| return 1 | ||
| } | ||
|
|
||
| # Collect screenshots | ||
| collect_screenshots | ||
|
|
||
| print_success "Tests completed successfully for API $API_LEVEL" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PID echo polluted by log lines – capture will be garbage
start_emulator prints status lines and the PID to stdout, then the caller captures
everything with emulator_pid=$(start_emulator).
The variable will contain coloured log lines + the PID, making it unusable for further
processing.
Redirect logs to stderr (or echo only the PID) and keep stdout clean:
- print_status "Emulator started with PID: $emulator_pid"
+ print_status "Emulator started with PID: $emulator_pid" >&2
…
- echo $emulator_pid
+ echo "$emulator_pid"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In scripts/run-screenshot-tests-api27.sh around lines 294 to 332, the
start_emulator function outputs both status messages and the emulator PID to
stdout, causing the captured PID variable to include unwanted log lines. To fix
this, modify start_emulator to redirect all log and status messages to stderr or
ensure only the PID is echoed to stdout, so that when the caller captures the
output, the PID variable contains only the PID without any log pollution.
| # Check if SDK tools exist | ||
| if [[ ! -f "$SDK_PATH/cmdline-tools/latest/bin/sdkmanager" ]]; then | ||
| print_error "sdkmanager not found at $SDK_PATH/cmdline-tools/latest/bin/sdkmanager" | ||
| print_status "Please install Android SDK Command-line Tools" | ||
| print_status "You can install it via Android Studio or download from Google" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Add SDK tools to PATH | ||
| export PATH="$SDK_PATH/cmdline-tools/latest/bin:$SDK_PATH/emulator:$SDK_PATH/platform-tools:$PATH" | ||
|
|
||
| print_success "Android SDK environment verified" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
cmdline-tools/latest hard-coded – breaks when only versioned dirs exist
The API-27 script already implements a robust fallback to the newest
versioned cmdline-tools directory. Re-use the same logic here to avoid
false “sdkmanager not found” errors on fresh SDK installs.
Consider extracting the shared helper to avoid divergence.
🤖 Prompt for AI Agents
In scripts/run-screenshot-tests.sh around lines 92 to 104, the script hard-codes
the path to cmdline-tools/latest which fails if only versioned directories
exist. To fix this, implement or reuse the existing logic from the API-27 script
that dynamically finds the newest versioned cmdline-tools directory instead of
relying on "latest". Refactor this logic into a shared helper function to avoid
duplication and ensure consistent SDK path resolution across scripts.
| echo "🔧 Starting Cloudy Library Screenshot Tests..." | ||
|
|
||
| # Color definitions | ||
| RED='\033[0;31m' | ||
| GREEN='\033[0;32m' | ||
| YELLOW='\033[1;33m' | ||
| BLUE='\033[0;34m' | ||
| NC='\033[0m' # No Color | ||
|
|
There was a problem hiding this comment.
Harden script with pipefail and nounset
Same concern as in the API-27 helper: pipelines can hide errors and
unset variables go unnoticed.
-set -e
+set -euo pipefailCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In scripts/run-screenshot-tests.sh around lines 8 to 16, the script lacks strict
error handling for pipelines and unset variables. Add 'set -o pipefail' to
ensure pipeline errors are not hidden and 'set -o nounset' to treat unset
variables as errors. Place these set commands near the start of the script
before any commands execute to harden the script's robustness.
| # API 27 uses x86_64, others auto-detect architecture | ||
| if [[ ${{ matrix.api-level }} -eq 27 ]]; then | ||
| arch="x86_64" | ||
| else | ||
| if [[ "$(uname -m)" == "arm64" ]]; then | ||
| arch="arm64-v8a" | ||
| else | ||
| arch="x86_64" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
uname -m check misses common ARM values
uname -m returns aarch64 (or occasionally arm64) on modern arm hosts.
The current test only recognises the literal string arm64, so ARM runners will fall back to x86_64 system-images and fail to start.
- if [[ "$(uname -m)" == "arm64" ]]; then
+ machine=$(uname -m)
+ # aarch64 ↔ arm64 are equivalent in this context
+ if [[ "$machine" == "arm64" || "$machine" == "aarch64" ]]; then📝 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.
| # API 27 uses x86_64, others auto-detect architecture | |
| if [[ ${{ matrix.api-level }} -eq 27 ]]; then | |
| arch="x86_64" | |
| else | |
| if [[ "$(uname -m)" == "arm64" ]]; then | |
| arch="arm64-v8a" | |
| else | |
| arch="x86_64" | |
| fi | |
| fi | |
| # API 27 uses x86_64, others auto-detect architecture | |
| if [[ ${{ matrix.api-level }} -eq 27 ]]; then | |
| arch="x86_64" | |
| else | |
| machine=$(uname -m) | |
| # aarch64 ↔ arm64 are equivalent in this context | |
| if [[ "$machine" == "arm64" || "$machine" == "aarch64" ]]; then | |
| arch="arm64-v8a" | |
| else | |
| arch="x86_64" | |
| fi | |
| fi |
🤖 Prompt for AI Agents
In .github/workflows/dropshots-screenshot-test.yml around lines 60 to 69, the
uname -m check only matches "arm64" but misses the common ARM value "aarch64".
Update the condition to check for both "arm64" and "aarch64" strings to
correctly detect ARM architectures and set arch="arm64-v8a" accordingly,
preventing fallback to x86_64 on ARM hosts.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/dropshots-screenshot-test.yml (1)
368-374: Broken image URLs are still using the incorrect/blob/endpoint
This was flagged earlier; the links will render as 404 images.
Switch to theraw.githubusercontent.compath or GitHub’s blobs API.- const blobUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${context.sha}?blob_sha=${img.blobSha}`; + // Provide the raw image content + const blobUrl = `https://raw.githubusercontent.com/${context.repo.owner}/${context.repo.repo}/${context.sha}?raw=true&blob_sha=${img.blobSha}`;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/dropshots-screenshot-test.yml(1 hunks).gitignore(1 hunks)app/build.gradle.kts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- app/build.gradle.kts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
.github/workflows/dropshots-screenshot-test.yml (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: screenshot-tests (30)
- GitHub Check: screenshot-tests (33)
- GitHub Check: API check
- GitHub Check: Spotless check
- GitHub Check: build
Speeds up screenshot tests by enabling AVD caching, KVM acceleration, and optimizing emulator boot parameters. Reduces wait times in tests and adjusts target system images for faster AVD creation. Increases concurrency for non-API 27 tests to improve overall efficiency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
scripts/run-screenshot-tests-api27.sh (2)
254-276: PID capture polluted by log lines – makes$emulator_pidunreliable
start_emulatorlogs to STDOUT and echoes the PID, so the caller captures the whole chunk:emulator_pid=$(start_emulator)
$emulator_pidwill contain ANSI-coloured “[INFO] …” lines plus the PID, which is unusable (even though you don't currently use it).
Either redirect status messages to STDERR or drop the capture:- print_status "Emulator started with PID: $emulator_pid" + print_status "Emulator started with PID: $emulator_pid" >&2 … - echo $emulator_pid + echo "$emulator_pid"
6-6: Addpipefail(+-u) to avoid silent pipeline failures
set -ealone does not abort on failures occurring on the left side of a pipeline and ignores unset variables.
Harden the script up-front:-set -e +set -euo pipefail.github/workflows/dropshots-screenshot-test.yml (3)
129-136: Uploading screenshots withoutif-no-files-foundwill break the jobIf the test phase aborts before screenshots are produced,
actions/upload-artifactraises an error and hides the real failure.
Add the same guard you already use for the test-report upload:path: screenshots/api-27/ + if-no-files-found: ignore
246-254: Same missing guard for the matrix uploadsReplicate the
if-no-files-found: ignoresafeguard here, otherwise the matrix job will fail when no screenshots are generated.path: screenshots/api-${{ matrix.api-level }}/ + if-no-files-found: ignore
396-402: PR comment shows broken images – incorrectblobUrlconstruction
https://github.com/…/blob/${context.sha}?blob_sha=…does not resolve to raw image data.
Replace with the raw endpoint:- const blobUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${context.sha}?blob_sha=${img.blobSha}`; + const blobUrl = `https://raw.githubusercontent.com/${context.repo.owner}/${context.repo.repo}/${context.sha}?raw=true&path=${img.blobSha}`;or use
api.github.com/repos/…/git/blobs/{sha}with properAccept: application/vnd.github.v3.rawheaders.
🧹 Nitpick comments (2)
scripts/run-screenshot-tests-api27.sh (2)
262-273: Consider enabling HW acceleration for x86_64 CI runners
-accel offdisables KVM/Hyper-V and slows the boot by several minutes.
On GitHub’s Ubuntu runners KVM is available – switching to-accel on(or dropping the flag) measurably cuts boot time.If you had to turn it off for a specific edge-case, add a short comment explaining why.
350-357: Quote variables when renaming filesUnquoted
$file/$base_namewill break on filenames with spaces or*.
Minor, but safe defaults are cheap:- mv "$file" "${base_name}_api${API_LEVEL}.png" 2>/dev/null || true + mv -- "$file" "${base_name}_api${API_LEVEL}.png" 2>/dev/null || true
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/dropshots-screenshot-test.yml(1 hunks)app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt(1 hunks)scripts/run-screenshot-tests-api27.sh(1 hunks)scripts/run-screenshot-tests.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt
- scripts/run-screenshot-tests.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
.github/workflows/dropshots-screenshot-test.yml (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
🧬 Code Graph Analysis (1)
scripts/run-screenshot-tests-api27.sh (1)
scripts/run-screenshot-tests.sh (15)
print_status(29-31)print_error(41-43)print_success(33-35)check_android_sdk(68-104)check_required_tools(107-139)check_system_requirements(142-181)print_warning(37-39)create_avd(215-253)collect_screenshots(359-387)check_setup(46-65)check_emulator(184-212)start_emulator(256-334)run_tests(337-356)cleanup_emulator(390-394)main(397-482)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: screenshot-tests (30)
- GitHub Check: screenshot-tests-api27
- GitHub Check: screenshot-tests (33)
- GitHub Check: build
- GitHub Check: Spotless check
- GitHub Check: API check
🔇 Additional comments (1)
.github/workflows/dropshots-screenshot-test.yml (1)
101-103: Allow test failures to fail the workflowGood job removing the previous
|| echo …guard.
Confirmreactivecircus/android-emulator-runnerpropagates Gradle’s exit code; otherwise wrap the./gradlewcall inset -e.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
app/src/androidTest/AndroidManifest.xml (1)
6-6: Remove unused ACCESS_NETWORK_STATE permission from Android test manifestThe
ACCESS_NETWORK_STATEpermission isn't needed for screenshot testing and should be removed to keep the manifest minimal.app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (5)
72-78: Replace Thread.sleep with proper synchronizationUsing
Thread.sleepin loops makes tests unnecessarily slow and unreliable. The current implementation adds 1.4 seconds of fixed delay.- repeat(3) { i -> - Thread.sleep(300) - println("Waiting for Compose to render... (${i + 1}/3)") - } - - // Reduced wait for background operations - Thread.sleep(500) + // Wait for activity to be ready using proper synchronization + composeTestRule.waitForIdle() + + // For native operations, implement proper callback or IdlingResource + composeTestRule.waitUntil(timeoutMillis = 5000) { + // Check if native libraries are loaded and ready + true // Implement actual readiness check + }
118-122: Replace Thread.sleep with proper synchronizationThe repeated sleep calls add unnecessary delay and make tests unreliable.
- repeat(2) { i -> - Thread.sleep(300) - println("Waiting for initial state... (${i + 1}/2)") - } + // Wait for initial state using proper synchronization + composeTestRule.waitForIdle() + + // Control animation timing if needed + composeTestRule.mainClock.autoAdvance = false + composeTestRule.mainClock.advanceTimeByFrame()
135-139: Replace Thread.sleep with proper synchronizationThe loop with sleep calls adds 1.2 seconds of unnecessary delay.
- repeat(3) { i -> - Thread.sleep(400) - println("Waiting for blur animation... (${i + 1}/3)") - } + // Wait for blur animation using proper synchronization + composeTestRule.waitForIdle() + + // Advance animation clock precisely + composeTestRule.mainClock.advanceTimeBy(1200) // Animation duration + composeTestRule.waitForIdle()
167-171: Replace Thread.sleep with proper synchronizationAnother instance of Thread.sleep in a loop that should be replaced with proper synchronization.
- repeat(2) { i -> - Thread.sleep(300) - println("Waiting for animation start... (${i + 1}/2)") - } + // Wait for animation start using proper synchronization + composeTestRule.waitForIdle() + + // Control animation timing programmatically + composeTestRule.mainClock.autoAdvance = false
184-188: Replace Thread.sleep with proper synchronizationThe final instance of Thread.sleep in a loop that adds unnecessary delay.
- repeat(3) { i -> - Thread.sleep(400) - println("Waiting for animation end... (${i + 1}/3)") - } + // Wait for animation end using proper synchronization + composeTestRule.mainClock.advanceTimeBy(1200) // Animation duration + composeTestRule.waitForIdle().github/workflows/dropshots-screenshot-test.yml (3)
130-137: Add if-no-files-found guard for screenshot uploadsThe screenshot artifact upload will fail if the directory doesn't exist, masking the real test failure.
with: name: screenshots-api-27 path: screenshots/api-27/ + if-no-files-found: ignore retention-days: 7
248-255: Add if-no-files-found guard for matrix screenshot uploadsThe matrix job uploads need the same protection against missing directories.
with: name: screenshots-api-${{ matrix.api-level }} path: screenshots/api-${{ matrix.api-level }}/ + if-no-files-found: ignore retention-days: 7
398-398: Fix broken image links in PR commentsThe blob URL construction creates invalid links that won't display images properly.
- const blobUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${context.sha}?blob_sha=${img.blobSha}`; + const blobUrl = `https://raw.githubusercontent.com/${context.repo.owner}/${context.repo.repo}/${context.sha}/${img.blobSha}`;Or use data URLs for inline embedding:
- const blobUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/blob/${context.sha}?blob_sha=${img.blobSha}`; + const blobUrl = `data:image/png;base64,${content}`;
🧹 Nitpick comments (3)
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (1)
74-75: Replace println with proper loggingUsing
printlnin tests makes output harder to filter and manage. Consider using Android's Log class or a proper logging framework.- println("Waiting for Compose to render... (${i + 1}/3)") + Log.d("MainTest", "Waiting for Compose to render... (${i + 1}/3)").github/workflows/dropshots-screenshot-test.yml (2)
340-340: Consider increasing screenshot upload limitThe current limit of 20 screenshots may be insufficient for comprehensive testing across multiple API levels and test scenarios.
- for (const screenshot of allScreenshots.slice(0, 20)) { // Limit to 20 images + for (const screenshot of allScreenshots.slice(0, 50)) { // Increased limit for comprehensive testing
91-91: Optimize emulator options for better performanceThe current emulator options are comprehensive but could be optimized for faster startup and better compatibility.
- emulator-options: -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none -partition-size 4096 -accel on -no-snapshot-save -memory 2048 -cores 2 -netfast -wipe-data + emulator-options: -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none -partition-size 4096 -accel on -no-snapshot-save -memory 2048 -cores 2 -netfast -wipe-data -qemu -m 2048
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/dropshots-screenshot-test.yml(1 hunks)app/build.gradle.kts(4 hunks)app/src/androidTest/AndroidManifest.xml(1 hunks)app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/build.gradle.kts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
.github/workflows/dropshots-screenshot-test.yml (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: screenshot-tests (33)
- GitHub Check: screenshot-tests (30)
- GitHub Check: screenshot-tests-api27
- GitHub Check: build
- GitHub Check: Spotless check
- GitHub Check: API check
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (5)
71-71: Thread.sleep usage still present despite past reviews.The Thread.sleep(1000) call remains in the code, which was extensively flagged in previous reviews for causing unreliable and slow tests. This should be replaced with proper synchronization using ComposeTestRule methods.
Refer to the previous review comments about replacing Thread.sleep with:
composeTestRule.waitForIdle()composeTestRule.mainClock.advanceTimeBy()- Proper IdlingResource implementation
117-117: Additional Thread.sleep usage in blur states test.Similar to the main screen capture, this test also uses Thread.sleep(400) which should be replaced with proper synchronization methods as discussed in previous reviews.
131-131: Animation timing should use test clock control.The Thread.sleep(1000) for animation duration should be replaced with ComposeTestRule clock control for deterministic testing.
160-160: Animation start timing uses Thread.sleep.The Thread.sleep(400) for animation start timing should be replaced with proper synchronization.
174-174: Animation completion timing uses Thread.sleep.The Thread.sleep(1000) for animation completion should be replaced with clock control.
🧹 Nitpick comments (3)
app/src/androidTest/kotlin/com/skydoves/cloudydemo/ScreenshotTestRule.kt (1)
47-57: Consider adding cleanup in the finally block.The
apply()method has an empty finally block. Consider adding cleanup logic to remove old screenshots or limit directory size to prevent test artifacts from accumulating.} finally { - // Cleanup if needed + // Clean up old screenshots to prevent accumulation + try { + val files = screenshotDir.listFiles() + if (files != null && files.size > 50) { // Keep last 50 screenshots + files.sortBy { it.lastModified() } + files.take(files.size - 50).forEach { it.delete() } + } + } catch (e: Exception) { + println("Cleanup failed: ${e.message}") + }.github/workflows/dropshots-screenshot-test.yml (2)
112-112: Fix trailing spaces in workflow file.Static analysis detected trailing spaces that should be removed for consistency.
- adb pull /data/data/com.skydoves.cloudydemo/cache/screenshots/ ./screenshots/api-27/ 2>/dev/null || echo "No internal screenshots directory" + adb pull /data/data/com.skydoves.cloudydemo/cache/screenshots/ ./screenshots/api-27/ 2>/dev/null || echo "No internal screenshots directory"
231-231: Fix trailing spaces in matrix job script.Similar trailing spaces issue in the matrix job script.
- adb pull /data/data/com.skydoves.cloudydemo/cache/screenshots/ ./screenshots/api-${{ matrix.api-level }}/ 2>/dev/null || echo "No internal screenshots directory" + adb pull /data/data/com.skydoves.cloudydemo/cache/screenshots/ ./screenshots/api-${{ matrix.api-level }}/ 2>/dev/null || echo "No internal screenshots directory"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/dropshots-screenshot-test.yml(1 hunks)app/build.gradle.kts(4 hunks)app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt(1 hunks)app/src/androidTest/kotlin/com/skydoves/cloudydemo/ScreenshotTestRule.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/build.gradle.kts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
.github/workflows/dropshots-screenshot-test.yml (2)
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:12:51.545Z
Learning: User l2hyunwoo prefers to keep PRs focused on their primary objective and avoid scope creep. They acknowledge technical debt items but prefer to address them in separate PRs rather than expanding the current PR scope.
Learnt from: l2hyunwoo
PR: skydoves/Cloudy#63
File: cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt:0-0
Timestamp: 2025-07-06T06:13:55.799Z
Learning: In the Cloudy project, the UIImage.asImageBitmap() function in cloudy/src/iosMain/kotlin/com/skydoves/cloudy/PlatformBitmap.ios.kt currently uses a placeholder implementation that generates synthetic gradients instead of extracting actual CGImage pixels. This is known technical debt that should be addressed in a future PR focused on pixel conversion optimization.
🪛 detekt (1.23.8)
app/src/androidTest/kotlin/com/skydoves/cloudydemo/ScreenshotTestRule.kt
[warning] 86-86: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🪛 YAMLlint (1.37.1)
.github/workflows/dropshots-screenshot-test.yml
[error] 112-112: trailing spaces
(trailing-spaces)
[error] 231-231: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: screenshot-tests (30)
- GitHub Check: screenshot-tests (33)
- GitHub Check: screenshot-tests-api27
- GitHub Check: Spotless check
- GitHub Check: build
- GitHub Check: API check
🔇 Additional comments (7)
app/src/androidTest/kotlin/com/skydoves/cloudydemo/ScreenshotTestRule.kt (2)
33-45: Well-designed initialization using internal cache directory.The approach of using internal cache directory (
context.cacheDir) is excellent as it avoids requiring storage permissions while still allowing CI collection of screenshots.
76-85: External save exception handling is appropriate.The exception handling for external save is well-implemented - it's expected to fail in some environments, and the error is appropriately logged without failing the test.
app/src/androidTest/kotlin/com/skydoves/cloudydemo/MainTest.kt (2)
33-60: Well-structured API-level specific testing approach.The approach of having separate tests for different API levels with conditional execution is excellent for ensuring comprehensive testing across Android versions. The generic test with timestamp ensures coverage even when API-specific conditions aren't met.
79-99: Retry logic is well-implemented for screenshot capture.The retry mechanism with error handling provides good resilience for screenshot capture failures, which can occur due to timing issues in UI tests.
.github/workflows/dropshots-screenshot-test.yml (3)
38-150: Comprehensive screenshot testing workflow with proper API-level separation.The workflow design is excellent with separate jobs for API 27 (requiring x86_64) and matrix jobs for API 30/33. The caching strategy, emulator configuration, and artifact handling are well-implemented.
299-445: Excellent PR comment generation with embedded screenshots.The GitHub script for creating PR comments with embedded screenshots is well-implemented. The use of base64 data URLs for inline images and the organized presentation by API level provides excellent visibility into test results.
84-93: Optimize emulator configuration for screenshot consistency.The emulator configuration is well-optimized for screenshot testing with fixed screen resolution, disabled animations, and appropriate memory settings. The use of
swiftshader_indirectGPU ensures consistent rendering across different CI environments.
| } catch (e: Exception) { | ||
| println("Screenshot capture failed: ${e.message}") | ||
| throw AssertionError("Failed to capture screenshot: ${e.message}") | ||
| } |
There was a problem hiding this comment.
Exception handling masks important error information.
The caught exception is only logged without proper error propagation. This could hide underlying issues that should cause the test to fail.
} catch (e: Exception) {
println("Screenshot capture failed: ${e.message}")
- throw AssertionError("Failed to capture screenshot: ${e.message}")
+ throw AssertionError("Failed to capture screenshot: ${e.message}", e)
}Also consider logging the full stack trace:
} catch (e: Exception) {
- println("Screenshot capture failed: ${e.message}")
+ println("Screenshot capture failed: ${e.message}")
+ e.printStackTrace()
throw AssertionError("Failed to capture screenshot: ${e.message}", e)
}📝 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.
| } catch (e: Exception) { | |
| println("Screenshot capture failed: ${e.message}") | |
| throw AssertionError("Failed to capture screenshot: ${e.message}") | |
| } | |
| } catch (e: Exception) { | |
| println("Screenshot capture failed: ${e.message}") | |
| e.printStackTrace() | |
| throw AssertionError("Failed to capture screenshot: ${e.message}", e) | |
| } |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 86-86: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
In app/src/androidTest/kotlin/com/skydoves/cloudydemo/ScreenshotTestRule.kt
around lines 86 to 89, the current exception handling only prints the exception
message and throws a new AssertionError with the message, which masks the
original stack trace. To fix this, log the full stack trace of the caught
exception and rethrow the original exception or wrap it preserving the cause, so
the test fails with complete error information and stack trace for easier
debugging.
🎯 Goal
This PR implements comprehensive screenshot testing for the Cloudy library using Dropshots with real Android emulators. Initially, we considered using Roborazzi for testing, but discovered that loading native libraries (RenderScript Toolkit) is not possible in Roborazzi's testing environment. Therefore, we pivoted to using actual Android emulators to ensure proper testing of the native blur functionality across different API levels (27, 30, 33).
The implementation provides:
🛠 Implementation details
Test Infrastructure
Cross-Platform Compatibility
CI/CD Pipeline
Local Development
✍️ Explain examples
Test Configuration (MainTest.kt)
Architecture-Aware Emulator Configuration (GitHub Actions)
Local Testing Script (run-screenshot-tests.sh)
Comprehensive Environment Validation
This implementation ensures reliable testing of the Cloudy library's native RenderScript functionality across different Android versions and host architectures, providing confidence in the library's compatibility and performance.
Preparing a pull request for review
Ensure your change is properly formatted by running:
Then dump binary API of this library that is public in sense of Kotlin visibilities and ensures that the public binary API wasn't changed in a way that make this change binary incompatible.
Please correct any failures before requesting a review.
Code reviews
All submissions, including submissions by project members, require review. We use GitHub pull requests for this purpose. Consult GitHub Help for more information on using pull requests.
Summary by CodeRabbit
New Features
Chores