Add a recipe for blank or incomplete screenshots - #317
DanielJette wants to merge 2 commits into
Conversation
AndroidTestifyBot
left a comment
There was a problem hiding this comment.
Changes requested
Good page in outline, and the capture-method half is accurate — but the timing half is scoped to the wrong rule, and the third-party examples need to be backed by something we actually compile.
1. The idle-wait claim only holds for ScreenshotRule
Testify waits for your test's UI to be idle before it captures. It launches the activity, applies your view modifications and Espresso actions, then waits on
Espresso.onIdle().
That's an exact description of ScreenshotRule: EspressoHelper.afterInitializeView() runs actions?.invoke() then syncUiThread() → Espresso.onIdle(), registered via addScreenshotObserver(espressoHelper).
ScreenshotScenarioRule has none of it — no EspressoHelper, no onIdle(), no setEspressoActions. This is deliberate: with ActivityScenario the test author already owns view automation, per Android's own guidance on driving an activity to a new state, so Testify steps out of the Espresso mechanism entirely.
As written, the page promises an idle wait that doesn't happen on the newer rule — precisely where blank-screenshot problems are most likely.
Please add a ScreenshotScenarioRule section explaining that idling is the test author's responsibility there, and how to do it. ScreenshotScenarioRuleExampleTests.setEspressoActions in the Legacy sample is the pattern to follow — Espresso is driven directly inside launchActivity { }.use { } before .withScenario(scenario).assertSame(), so perform() does the synchronising:
launchActivity<TestHarnessActivity>().use { scenario ->
scenario.onActivity { activity -> /* inflate / set up */ }
Espresso.onView(withId(R.id.edit_text)).perform(typeText("Testify"))
rule.withScenario(scenario).assertSame()
}(Note the sample's method is named setEspressoActions but calls raw Espresso.onView(...) — there's no such API on the scenario rule.)
2. Please add the idling examples to the samples
The CountingIdlingResource block and the Glide 4 RequestListener don't exist anywhere in the repo, so nothing compiles them or keeps them honest — and Glide's listener signatures (nullability of target, presence of isFirstResource) shifted across 4.x minors while the page pins no minor version.
Please add working versions of both to the sample suite and have the page reference them, the way #318 references FullscreenCaptureExampleTests.kt. That's the difference between a recipe we can verify and one that rots.
3. The decrement() debugging tip needs scoping
temporarily remove the call to
decrement(). The test should now hang and then fail with an Espresso idling timeout
On ScreenshotScenarioRule nothing calls into Espresso, so the test won't hang — it captures immediately and the tip reports a false negative. Please scope it to ScreenshotRule.
4. Query on shadows and elevation
The GPU-drawn list ends with "and some shadows and elevation". The useSoftwareRenderer KDoc in TestifyConfiguration describes the opposite direction — that software rendering is what loses rounded corners, shadows and elevation. Is the mention right for the drawing-cache path, or bleeding in from the software-renderer caveat? As-is the two pages could contradict each other.
Verified correct
- Default View capture really is the drawing cache —
createBitmapFromDrawingCachedoesview.isDrawingCacheEnabled = true; Bitmap.createBitmap(view.drawingCache). - Both Compose rules default to
::pixelCopyCapture, and both callcomposeTestRule.waitForIdle(), so "the Compose rules also wait for the composition to be idle" is right. - Pairing PixelCopy with a small
exactnessis sound givenFuzzyCompare. - Fullscreen capture for other windows is consistent with
FullscreenCaptureMethod.
Reviewed by Claude on behalf of @DanielJette.
2e08906 to
5fb159c
Compare
AndroidTestifyBot
left a comment
There was a problem hiding this comment.
Changes requested
All four items from the previous review are addressed, and the image-loading section is far better for being lifted from code the repo compiles. I checked the three snippets against their sources: setEspressoActions in ScreenshotScenarioRuleExampleTests.kt, HomeScreenTest.kt's @Before, and TestImageLoader.kt all match, and idling-concurrent is the right artifact. Docs build is clean. Three accuracy points remain.
1. The Glide executor claim
Glide and Picasso both accept a custom
ExecutorServicein the same way — Glide throughGlideBuilder.setSourceExecutor
Picasso.Builder.executor(ExecutorService) is right. GlideBuilder.setSourceExecutor takes a GlideExecutor, which is a final class built through GlideExecutor.newSourceBuilder(); it has no public way to wrap an arbitrary ExecutorService, so an IdlingThreadPoolExecutor can't be handed to it "in the same way". Nothing in this repo depends on Glide, so I'm going from the Glide 4 API rather than a compile — which is the same position the page is in, and the reason the last review asked for third-party claims to be backed by something compiled. Please either verify it against a real Glide build or cut Glide from that sentence and leave it under the listener fallback.
2. ScreenshotScenarioRule does not wait for "Nothing"
ScreenshotScenarioRule.afterInitializeView calls getInstrumentation().waitForIdleSync() (and again after closing the keyboard). So it waits for the main thread to go idle; what it doesn't do is consult Espresso, so idling resources are ignored. The table row and "captures immediately" in the tip should say that. The practical advice is unchanged, but "Nothing" is wrong as stated and I got it wrong the same way in the last review.
3. The Compose row merges two different rules
ComposableScreenshotRule extends ScreenshotRule, so it gets the Espresso idle wait as well as composeTestRule.waitForIdle(). ComposableScreenshotScenarioRule only gets the latter. One row for both loses the distinction the table was added to draw.
Cross-PR
#321 now links to this page by absolute URL, so the two need to merge in order (this one first).
|
All three are right, and I verified each against the source before changing it. 1. Glide cut from the executor sentenceConfirmed:
Glide now appears only under the listener fallback, where 2. "Nothing" was wrong
The table now says "the main thread, via 3. The Compose rules are split
The table also gained a third column, Honours idling resources, because that is the single fact the page needs a reader to take away and it was buried in prose. Every rule waits for the main thread; only the Cross-PRAgreed, and worth flagging that the dependency now runs both ways: #317 → #334 → #321. #321's link to this page is back to a relative one — the absolute URL I had used was a workaround for what turned out to be a stale Docs build clean with |
AndroidTestifyBot
left a comment
There was a problem hiding this comment.
Approved
All three points are resolved, and the extra column is an improvement.
- The Glide executor claim is gone. Glide now appears only under the listener fallback, and the replacement sentence is accurate.
ScreenshotScenarioRuleis described as waiting for the main thread viaInstrumentation.waitForIdleSync()without consulting Espresso. That matchesafterInitializeView(:451,:455), and thedecrement()tip now says "as soon as the main thread is idle".- The Compose rules are split correctly.
ComposableScreenshotRuleextendsScreenshotRule<ComposableTestActivity>andComposableScreenshotScenarioRuleextendsScreenshotScenarioRule, so they inherit opposite answers in the "Honours idling resources" column.
Docs build clean on this head. This one should merge first of the three: #321 links to it relatively and won't build without it.
What does this change accomplish?
The answers to #91 and #93 exist only in long issue threads. Docs searches for
async,wait,delayandanimationfind nothing that explains when Testify captures.Fixes #91
Fixes #93
How have you achieved it?
recipes/25-blank-or-incomplete-screenshots.md.Espresso.onIdle(), and that the Compose rules also wait for the composition to be idle.captureMethod = ::pixelCopyCapturewith a smallexactness, from Screens which inflate views at a later time are never captured #91.Scope of Impact and Testing instructions
Documentation only; no library or plugin changes, so no CHANGELOG entry. Built locally with
yarn buildfromdocs/. No broken links. Guidance comes from the #91 and #93 threads, updated to the currentconfigure { captureMethod = … }API.The commit includes
[skip ci]so CI doesn't run for this docs-only change.Changes since the review
All four review items are addressed.
1. The idle wait is now scoped per rule. "When Testify takes the screenshot" opens with a table
covering all three cases, and a new Idling with
ScreenshotScenarioRulesection explains thatidling is the test author's job there and why, with the
setEspressoActionspattern fromScreenshotScenarioRuleExampleTests.ktand a pointer to the full test.2. The image examples are now backed by code the repo compiles. The Glide 4
RequestListenerblock is gone — the review was right that its signatures drift across 4.x minors and nothing here
kept it honest. In its place are the two techniques the Flix sample actually uses:
Dispatchers.Unconfinedon the Coil loader, as inHomeScreenTest.kt— which landed in 326: Add RTL screenshot test sample #327 afterthis review was written and is exactly the compiled example it asked for.
IdlingThreadPoolExecutor, as inFlixLibrary'sTestImageLoader.kt, for keeping real threadingwhile staying visible to Espresso.
Glide and Picasso are covered by naming their equivalent executor hooks, with the per-request
CountingIdlingResourcekept only as the fallback for loaders whose executor you cannot reach. Noversion-specific callback signatures remain.
3. The
decrement()tip is scoped. It states that the hang only happens where something waits onEspresso, and what to do instead under
ScreenshotScenarioRule.4. Shadows and elevation: both pages were right. The drawing cache is drawn in software, so it
loses the same hardware-accelerated effects that
useSoftwareRenderer's KDoc warns about — one isopt-in, the other is the default capture method getting you there anyway. The page now says that
explicitly instead of leaving the two to contradict each other.
Rebased onto current
main. Verified withnpm run buildfromdocs/;onBrokenLinksisthrow,so the green build covers every internal link.
Notice
Warning
This change must keep
mainin a shippable state; it may be shipped without further notice.