Output too large. Showing first 8,000 and last 32,000 characters. For full output see: /home/Messier82/.gemini/tmp/service-book/tool-outputs/session-6179d957-b93b-4512-9500-1894e0d9d848/run_shell_command_1779353500081_0.txt Output: Add store screenshot generation yorvana/android#112 Open • Mesya82 wants to merge 17 commits into master from implement-store-screenshots • about 1 day ago +816 -112 • ✓ Checks passing ## Summary • add a managed-device store screenshot suite with seeded demo fixture data and stable emulator status/display setup • generate phone, 7-inch tablet, and 10-inch tablet Play Store PNG assets into distribution/store-assets/screenshots • extract shared androidTest vault/wait helpers and document the screenshot generation workflow ## Verification • ./gradlew generateStoreScreenshots • ./gradlew ktlintCheck detekt lintDebug • ./gradlew pixel2api33Check Closes #111 Mesya82 (Member) • 1d ## Review Overview: Adds a managed-device store screenshot suite (Pixel 6, Nexus 7, Pixel Tablet at API 34) that seeds a demo vault, configures system UI demo mode, and captures 18 PNGs into distribution/store-assets/screenshots/{phone, tablet- 7,tablet-10}/ . Extracts shared androidTest helpers ( testsupport/ ) so SmokeTest and the new suite share vault setup and await helpers. ### Code correctness & risk • additionalTestOutputDir widened from /sdcard/test-outputs/smoke to /sdcard/test-outputs ( app/build.gradle. kts:72 ). This silently changes the smoke device's auto-pull root — any tooling that grepped the old path no longer finds it, and unrelated artifacts under /sdcard/test-outputs/ will now be pulled with smoke runs too. If the goal is just to share the prefix with /sdcard/test-outputs/screenshots/ , keep smoke under /sdcard/test-outputs/smoke/ and screenshots under /sdcard/test-outputs/screenshots/ — they don't need a common parent. • Deviation from the #111 plan: screencap -p instead of captureRoboImage() ( StoreScreenshotBase.kt:30-39 ). The amended #111 plan called for bare captureRoboImage() calls. This PR uses adb screencap via uiAutomation. executeShellCommand instead. Practically this is more faithful to "real platform rendering" — it captures the framebuffer including system UI — but #111 should be updated to reflect what shipped, otherwise the next reader expects Roborazzi. • Nested Gradle invocation ( registerStoreScreenshotCopy , Exec task at app/build.gradle.kts:180-191 ). . /gradlew is invoked from inside a Gradle task to re-trigger the gradle.startParameter.taskNames detection so testInstrumentationRunnerArguments["package"] gets set per device. This works but is fragile: nested invocations don't inherit --offline / --no-daemon /profile flags from the outer build, and they bypass the configuration cache. Cleaner option: configure testInstrumentationRunnerArguments per-managed-device rather than relying on outer-task detection. • wm size overrides per device ( StoreScreenshotBase.configureDisplay , lines 70-77). Forcing wm size 1080x2400 on Pixel 6, 1200x1920 on Nexus 7, 2560x1600 on Pixel Tablet. The values match native defaults so this is largely a no-op, but if AGP's GMD ever changes default density/size for these AVDs, the override silently desyncs the screenshot from what real users see. Consider removing if the goal is just "use the device default." • outputBucket() device-hint fallback ( StoreScreenshotBase.kt:42-58 ) tries to derive a bucket from getprop / Build.MODEL / wm size when -PstoreScreenshotBucket isn't passed. In normal use the property is always set by the wrapper task, so this fallback is dead weight that just delays an "Unknown screenshot device size" error. Either drop it and require the property, or error fast. ### Style & conventions • testsupport/ extraction is clean; MainActivityComposeRule typealias keeps helper signatures readable. The SmokeTest diff is mechanical and preserves behavior. • Copy task's duplicatesStrategy = DuplicatesStrategy.FAIL plus doLast missing-file check is solid belt-and- braces — failure modes surface as a clear error rather than empty output. • Sequential chaining ( copyPhone → cleanTablet7 → runTablet7 → copyTablet7 ) is required because additional- output roots aren't namespaced per device — good call to serialize rather than fight that. ### Test coverage • Six tests, one per screen. Each sets up state from scratch via installFreshVault + fixture, so ordering doesn't matter. Adequate. • @SdkSuppress(minSdkVersion = 34) on StoreScreenshotTest prevents accidental execution on the Pixel 2 API 33 smoke device — good belt for the per-task package filter's suspenders. ### Security ... [56,425 characters omitted] ... exit() writes it back with the matching overlay ( DemoMode.kt:43-50 ). No more silent gesture→3-button conversion on dev machines. • Wildcard include("**/screen_*.png") removed. Only the precise include("**/$outputDirectory/screen_*.png") pattern remains ( app/build.gradle.kts:325 ), so a device-side path mistake will surface as a missing-file error instead of being silently slurped. • testBuildType trade-off addressed via -PscreenshotBuild . Default is debug for speed/stability; release is opt- in for marketing assets only ( app/build.gradle.kts:46-49, 71 ). This is the right shape — coverage stays on the fast debug path, marketing-grade captures use the production-representative release variant on demand. ### New issues introduced in this delta • generateGmdCoverage is broken again under -PscreenshotBuild . The new code uses pixel2api33${testVariantCap}AndroidTest and reads from outputs/managed_device_code_coverage/$testVariant/pixel2api33/coverage.ec ( app/build.gradle.kts:260, 263 ). When - PscreenshotBuild is set, testVariant=release , so it depends on pixel2api33ReleaseAndroidTest and looks for the release coverage.ec . But enableAndroidTestCoverage = isCoverageEnabled was removed from the release block in this PR (was added at L93 in 849f538 , gone now). The release variant will not emit coverage.ec , so ./gradlew generateGmdCoverage -Pcoverage -PscreenshotBuild fails the existing-file check exactly like before. The L259 comment ("Coverage is always collected from the debug variant of the smoke device") describes the intent — the code doesn't match. Two clean fixes: • Hardcode debug for generateGmdCoverage : replace $testVariant / $testVariantCap here with literal "debug" / "Debug" . Matches the comment and is the right call given coverage is variant-agnostic. • Or re-add enableAndroidTestCoverage = isCoverageEnabled to the release block. Then both variants work, at the cost of extra release-instrumentation coverage cost. First option is the cleaner one — coverage is a debug concern, screenshots are a release concern, they shouldn't be tied. • forceHideDeveloperPanel should be removed entirely. Now that screenshot generation runs against the release variant ( -PscreenshotBuild ), BuildConfig.DEBUG is already false for those captures and the Developer section is naturally excluded. The flag adds production-surface mutable state on YorvanaApplication , a showDeveloperPanel field on SettingsState , plumbing in StoreScreenshotBase , and parallel test-isolation hazards (never reset in @After , read only at ViewModel construction so a pre-warm would cache the wrong value) — all to solve a problem release minification already solves. Drop: the forceHideDeveloperPanel field on YorvanaApplication , the showDeveloperPanel field on SettingsState , the app.forceHideDeveloperPanel = true line in StoreScreenshotBase. @Before , the hideDeveloperPanel = "true" args in the GMD reflection block, and revert SettingsScreen to if (BuildConfig.DEBUG) . The SettingsScreenshotTest debug tests can mock the section in directly without going through SettingsState. The contract becomes "screenshots require -PscreenshotBuild ", which the docs already say. ### Smaller observations • Removing the if (name.contains("Release")) { enabled = false } guard lets ./gradlew test now also execute testReleaseUnitTest (the AGP test meta-task aggregates all variants). Roughly doubles unit-test wall time for anyone running plain ./gradlew test . If the team wanted the release variant runnable on demand only, the guard could come back — verifyWithCoverage no longer needs it disabled, since the dependency was switched to testDebugUnitTest . Non-blocking either way; just worth deciding deliberately rather than as a side-effect of unblocking coverage. • originalNavMode malformed-value handling. runShell("settings get secure navigation_mode").trim() returns "null" on Android when the setting is unset. exit() would then run settings put secure navigation_mode null , which is a no-op on most platforms but unspecified. Cheap guard: if (restoreMode !in setOf("0", "1", "2")) "0" else restoreMode . • SettingsScreenshotTest correctly opts the two debug-section tests into showDeveloperPanel = true and leaves the rest at default. Test isolation is intact. • YorvanaApplication.forceHideDeveloperPanel is a public mutable var on Application — a bit of a smell as a production API surface, but the comment scopes it to instrumentation tests and there's no other consumer. Acceptable. • Docs ( GEMINI.md , TESTING_SETUP.md ) clearly call out the -PscreenshotBuild flag and the debug/release split. Good. ### Bottom line Two things to fix: 1. generateGmdCoverage symmetry: in fixing one collision, the release enableAndroidTestCoverage flag was removed but the GMD-coverage task still follows $testVariant . The L259 comment already says coverage is debug-only — make the code agree (hardcode "debug" / "Debug" in that task). 2. Drop forceHideDeveloperPanel and all its plumbing — -PscreenshotBuild makes BuildConfig.DEBUG false at capture time, which is the actual mechanism keeping the Developer section out of marketing assets. The flag is redundant production-surface state. After that, please run ./gradlew verifyWithCoverage -Pcoverage and ./gradlew generateGmdCoverage -Pcoverage (no - PscreenshotBuild ) once before merge to confirm the default coverage path is green end-to-end. Everything else is paper cuts; the architectural shape ( -PscreenshotBuild , polled nav-mode, nav-mode restore, mustRunAfter on devices) is the right one. View the full review: https://github.com/yorvana/android/pull/112#issuecomment-4505985168 Mesya82 (Member) • 36m Re-review at 19ffd2c — one new commit since review-11 ( e00c5f6 ), plus a full sweep across the PR (base 003a006 , 1.4k-line diff). ## Delta: resolved since review-11 • generateGmdCoverage symmetry fixed. Task and path now hardcoded to debug ( app/build.gradle.kts:261, 264 — pixel2api33DebugAndroidTest and outputs/managed_device_code_coverage/debug/pixel2api33 ). Comment at L259–260 now matches the code. ./gradlew generateGmdCoverage -Pcoverage works regardless of -PscreenshotBuild . Coverage and screenshots are correctly decoupled on the variant axis. • forceHideDeveloperPanel removed end-to-end. All five callsites gone: field deleted from YorvanaApplication , showDeveloperPanel field deleted from SettingsState , app.forceHideDeveloperPanel = true removed from StoreScreenshotBase.@Before , SettingsScreen reverted to if (BuildConfig.DEBUG) (L550), SettingsScreenshotTest reverted to setupScreen(isDark = …) without the opt-in flag. _state is back to MutableStateFlow(SettingsState()) . Contract is now "marketing screenshots require -PscreenshotBuild ," matching the docs. • if (name.contains("Release")) { enabled = false } guard restored ( app/build.gradle.kts:515-517 ). Safe now because verifyWithCoverage was switched to testDebugUnitTest in the prior round. ./gradlew test is back to debug-only wall time. • originalNavMode malformed-value clamp added ( DemoMode.kt:45-46 ). capturedMode !in setOf("0", "1", "2") falls back to "0" . Handles the "null" literal that settings get returns for unset. ## Full-sweep findings (not previously called out) • Dead hideDeveloperPanel instrumentation args in app/build.gradle.kts:232, 237, 242 . These were the GMD-side mirror of forceHideDeveloperPanel . With the application field gone, grep confirms zero readers of this arg anywhere in app/src . All three lines are dead and the L231 comment ("Hide developer panel even if we are in a debug build (when -PscreenshotBuild is NOT used)") describes a behavior the code no longer implements. Strip them — the whole point of dropping forceHideDeveloperPanel was that release minification handles this. • distribution/store-listing.md:208 still says ./gradlew generateStoreScreenshots with no -PscreenshotBuild . This is the marketing-asset workflow doc — the one place someone preparing store assets is most likely to read. GEMINI.md:10 and TESTING_SETUP.md:158 correctly call out the flag; this file should too, with a one-line "use - PscreenshotBuild for production-representative assets" hint. Without it, anyone following store-listing.md will ship debug-build screenshots to Play Store. • StoreScreenshotBase.captureStoreScreenshot() capture check is weak. runShell("ls -l $outputPath") then check(fileName in result) — relies on executeShellCommand only routing stdout (not stderr) into the descriptor, so a missing file produces empty output and the assertion fails. That holds today ( UiAutomation.executeShellCommand does discard stderr), but it's a fragile invariant for a marketing-asset gate. Two clearer alternatives: • runShell("stat -c %s $outputPath").trim().toLongOrNull()?.let { it > 0 } == true — directly asserts non-empty file. • Read on the JVM side: File(outputPath).length() > 0 after the screencap returns. Non-blocking, but the current check would pass if anyone ever changed runShell to fold stderr into the same descriptor. • testInstrumentationRunnerArguments["additionalTestOutputDir"] set in defaultConfig ( app/build.gradle.kts:65 ) is overridden in the reflective per-device block at L207. The default value is /sdcard/test-outputs/smoke ; the per- device block sets /sdcard/test-outputs/screenshots on the three screenshot devices and keeps /sdcard/test- outputs/smoke on pixel2api33. Consistent, but the L65 default is now redundant — every code path that reads additionalTestOutputDir is the reflective override. Either pin it as a documented baseline or drop it. Minor. • outputBucket() in StoreScreenshotBase.kt:48-63 correctly normalizes via short/long for the three current GMD devices. Future-proofing nit: if anyone adds another phone/tablet to the device list, classification will silently coerce to phone . A Log.w in the else branch with the actual short/long would surface the surprise faster. Non- blocking. • StoreScreenshotTest scenarios are deterministic and well-scoped. Each test goes through the proper UI path (FAB tap, scroll-to-node, awaitTag/awaitText) rather than seeding state shortcuts. screen_06_paywall_dialog correctly uses setBillingOverride(FORCE_FREE) rather than fabricating UI state. Good. • screenshots-fixture/ JSON data is internally consistent — vehicle.json declares recordCount = 6, lastServiceDate = 2025-07-15 , six records exist in records/ , the latest dated 2025-07-15. categories.json defines coolant-flush which is the category for that latest record. The denormalized fields will match what the UI computes. Good. • testsupport/ extraction is a clean refactor. ComposeWaitHelpers.kt , VaultTestSetup.kt , ScreenshotConstants. kt are reused by both SmokeTest and the new screenshot suite without behavior changes — the SmokeTest diff ( +28/- 88 ) is pure extraction, no scenario weakening. The shared MainActivityComposeRule typealias is the right abstraction. • SmokeTest step logs use Log.d("SmokeTest", ...) — fine in instrumentation context, won't show up in CI artifacts unless logcat is captured. Existing pattern; non-blocking. • proguard-rules.pro test-surface keeps ( -keep class com.sun.jna.** , -keep class net.bytebuddy.** , plus the Compose -keep s) are the trade-off of running instrumentation tests against the release variant. With - PscreenshotBuild opt-in only, this cost is bounded — the default debug GMD path ( pixel2api33DebugAndroidTest ) doesn't need them. Acceptable. • tasks.withType heap config ( maxHeapSize = "1536m" ) unchanged. Fine. • .gitignore adds .antigravitycli/ — fine, matches the earlier 91d3594 cleanup intent. • TESTING_SETUP.md Section 7.1 correctly describes the -PscreenshotBuild debug/release split, why GMD- instrumented screenshots are the exception to the Roborazzi rule, and how package / notPackage isolate the smoke and screenshot devices. The "hiding developer settings" wording still makes sense now that release minification (rather than a runtime flag) is the mechanism. ## Bottom line Both review-11 blockers cleanly resolved, plus two smaller observations addressed (Release-disable restored, nav- mode clamp). Two things to clean up before merge: 1. Strip the dead hideDeveloperPanel GMD args at app/build.gradle.kts:232, 237, 242 (and the stale L231 comment). 2. Add -PscreenshotBuild to distribution/store-listing.md:208 so the marketing-asset workflow doc matches GEMINI. md and TESTING_SETUP.md. Capture-check robustness ( stat -based) and the L65 redundant default are non-blocking nits. Run ./gradlew verifyWithCoverage -Pcoverage and ./gradlew generateGmdCoverage -Pcoverage end-to-end once before merging — coverage chain has been the source of every prior collision and is worth verifying empirically. View the full review: https://github.com/yorvana/android/pull/112#issuecomment-4506152512 Mesya82 (Member) • 1m • Newest comment Re-review at 9dea594 — one new commit since review-12 ( 19ffd2c ), plus a full sweep across the PR (base 003a006 ). ## Delta: resolved since review-12 • Dead hideDeveloperPanel GMD args stripped ( app/build.gradle.kts:227-237 ). All three map["hideDeveloperPanel"] = "true" lines removed alongside the stale "Hide developer panel even if we are in a debug build" comment. Grep confirms zero references remain. • -PscreenshotBuild added to distribution/store-listing.md:208 with an IMPORTANT callout explaining the release -buildType pivot and why it matters for marketing assets. The marketing-asset workflow doc now matches GEMINI.md:10 and TESTING_SETUP.md:158 . • Capture check is now robust ( StoreScreenshotBase.kt:43-49 ). runShell("stat -c %s $outputPath").trim(). toLongOrNull() ?: 0L and check(fileSize > 0) . No longer depends on executeShellCommand discarding stderr; the failure message also reports the raw shell output for diagnostics. • outputBucket() else branch logs the surprise ( StoreScreenshotBase.kt:65-68 ). Unknown dimensions now produce Log.w("StoreScreenshot", "Unknown device dimensions: ${w}x${h}. Falling back to 'phone'.") before the fallback. Future device additions will leave a breadcrumb in logcat. • L65 redundant additionalTestOutputDir default removed and replaced with explicit per-device assignment for pixel2api33 inside the reflective block ( app/build.gradle.kts:239-242 ). All four device tasks now route through the same configuration path — no implicit defaultConfig fallback. Cleaner. ## New concern in the delta • argsProperty.set(provider) → argsProperty.putAll(provider) ( app/build.gradle.kts:223 ) is a semantic change, not a cosmetic rename. set replaces the property's value; putAll accumulates entries into the existing value. The comment at L220-221 ("evaluated specifically for this task instance … prevents aliasing/merging issues when AGP shares the underlying property object across tasks") still argues for the isolation property that set provides — but putAll does the opposite of isolation if AGP genuinely shares the MapProperty.Corroborating evidence that AGP does share state across these tasks: the copy task at app/build.gradle.kts:304-309 uses include("**/$outputDirectory/screen_*.png") with a wildcard, with the comment "AGP sometimes aliases instrumentation arguments across GMD tasks, leading to screenshots being saved in the wrong bucket subfolder on the host." So aliasing has been observed in practice — which means putAll (which accumulates instead of replacing) is moving in the wrong direction.Concrete risk: if AGP reuses the same MapProperty across the four GMD device tasks, putAll from pixel2api33 ( notPackage , additionalTestOutputDir=smoke ) and putAll from phonePixel6api34 ( package , additionalTestOutputDir=screenshots ) would merge into a map containing both package and notPackage with additionalTestOutputDir overwritten by whichever was configured last. AndroidJUnitRunner would then apply both filters — excluding the screenshot package on the phone device because notPackage excludes it — and silently skip the screenshot tests. Either revert to set , or pair putAll with empty() to reset first, or update the comment to reflect actual semantics. ## Full-sweep findings (not previously called out) • BillingManagerImplTest.kt is +608 lines of scope creep. Added in this PR ( new file , base 003a006 ) but unrelated to the "Add store screenshot generation" theme. It is @Category(LocalOnly::class) so it's correctly excluded from CI runs — the fidelity cost is bounded — but bisect/blame on future billing-test failures will point at a screenshots commit, which is misleading. Easier to review (and revert if needed) as its own PR. • tasks.matching { it.name == deviceTaskName } ( app/build.gradle.kts:294, 302 ) is silent on the no-match case. If testVariantCap ever resolves to something unexpected (e.g., the user passes -PscreenshotBuild=true thinking it's a value rather than just presence-checking), the matched set is empty and the copy task wires up zero dependencies — generateStoreScreenshots will succeed with stale or missing files. tasks.named(deviceTaskName) would fail loudly at configuration time. Worth swapping unless the lazy semantics are intentional. • Outer generateStoreScreenshots missing-file check duplicates the per-bucket checks ( app/build.gradle.kts:381- 394 ). Each copyXxxStoreScreenshots task already throws a GradleException listing the missing PNGs for its bucket; the outer task then re-walks all three buckets. If any bucket is incomplete, the per-task check fires first and Gradle stops — the outer check is dead code. Pick one layer. • storeAdditionalOutputRoots = listOf(layout.buildDirectory.dir(...)) ( app/build.gradle.kts:284-287 ) is a single- element list. The iteration via forEach { outputRoot -> from(outputRoot) {...} } works fine, but the list wrapping has no current purpose. Either inline as a single dir or add a comment explaining the future-proofing. • getAdditionalTestOutputDir().set(...) in the device-task config ( app/build.gradle.kts:195-200 ) duplicates a value derived from the additionalTestOutputDir instrumentation arg in the per-task block at L226-242. The host- pull dir uses $testVariant/$deviceDirName and the device-side arg uses literal /sdcard/test-outputs/screenshots . They're independent strings — if someone changes one without updating the other, the copy task finds no files and the per-bucket missing-check fires with a confusing "missing PNG" message rather than "device-side and host-side paths drifted." Worth pinning both to a single source of truth (e.g., extract the device-side path into a constant alongside screenshotTestPackage ). • doFirst { delete(storeAdditionalOutputRoots) } runs on the device task itself, not the copy task ( app/build. gradle.kts:295-298 ). This wipes ALL three buckets' outputs before any single device runs. So ./gradlew phonePixel6api34ReleaseAndroidTest -PscreenshotBuild followed by ./gradlew copyTablet10StoreScreenshots would find an empty tablet-10 output. The inline comment acknowledges the side-effect but doesn't explain when it'd surprise someone. Acceptable if the documented workflow is always generateStoreScreenshots end-to-end; risky if anyone debug-runs individual device tasks. Non-blocking. • StoreScreenshotTest uses real UI paths rather than state shortcuts — FAB taps, performScrollTo , awaitTag/awaitText, setBillingOverride(FORCE_FREE) for the paywall test. @SdkSuppress(minSdkVersion = 34) correctly gates this suite to the API 34 store devices and excludes accidental runs on pixel2api33 (API 33 anyway, but defense-in-depth). Good. • SmokeTest refactor (-88/+28) is pure extraction into testsupport/ — awaitTagVisible , awaitTagGone , awaitText , resetYorvanaState , installFreshVault . Scenarios are unchanged byte-for-byte. No fidelity regression. • screenshots-fixture/ data is internally consistent: vehicle.json declares recordCount=6, lastServiceDate=2025- 07-15 ; six record files exist ( 2024-03 through 2025-07 ); categories.json 's coolant-flush is the category for the latest record. Denormalized fields will match the UI's on-disk computation (ADR-008). Good. • DemoMode.exit() ordering ( DemoMode.kt:48-54 ) re-enables the overlay before settings put secure navigation_mode . If the broadcast races the settings put on a slow emulator, briefly the wrong nav style could render — but exit() runs in @After so test screenshots are already captured. Non-blocking. • proguard-rules.pro test-surface keeps ( -keep com.sun.jna.** , -keep net.bytebuddy.** , Compose nodes) plus dontwarns are bounded by -PscreenshotBuild opt-in. The default debug GMD path ( pixel2api33DebugAndroidTest ) doesn't need them; only release instrumentation does. Acceptable trade-off. • TESTING_SETUP.md section 7.1 correctly describes the debug/release split, why GMD-instrumented screenshots are the exception to the Roborazzi rule, and the package / notPackage isolation pattern. Wording matches the code. ## Bottom line All three review-12 cleanups landed exactly as suggested, plus L65 baseline cleanup as a bonus. Two things worth resolving before merge: 1. set → putAll semantics — pick one of: revert to set , prepend empty() , or update the comment to drop the "shares the underlying property" framing. Given the corroborating "AGP aliases args" comment at L306, set is the safer answer. 2. Move BillingManagerImplTest.kt (+608 lines) to its own PR if practical — keeps the screenshots PR scoped and makes future bisect cleaner. (Or rename the PR title to reflect the actual scope.) The other full-sweep notes (matching-named tasks, redundant outer check, single-element list, host/device path duplication, single-device delete side-effect) are non-blocking polish — useful to address eventually but won't break the merge. View the full review: https://github.com/yorvana/android/pull/112#issuecomment-4506412128 View this pull request on GitHub: https://github.com/yorvana/android/pull/112 Process Group PGID: 373700