108: Fix the report's baseline path and duplicate image keys - #330
Conversation
AndroidTestifyBot
left a comment
There was a problem hiding this comment.
Changes requested
Both code changes are correct. What's missing is around them.
1. The documented report example now shows the wrong format
baseline_image changes shape for every test, but the two places that document report.yml still show the old one:
docs/docs/get-started/8-use-gradle-plugin.md:532—baseline_image: assets/screenshots/29-1080x2220@440dp-en_US/withFocusOnBackground.pngPlugins/Gradle/README.md:469— same line
The committed report.yml at the repo root has it too (it's the file the PR description quotes as evidence). Please update the two docs examples to <Class>_<method>.png, and either regenerate or drop the root file.
2. CI is red
Legacy Sample failed and Library / Plugin are pending. This PR changes the reporter and the Legacy sample is the module with testify-reporter enabled, so it can't be waved through. For what it's worth I could not reproduce it — LegacySample:screenshotTest on this branch gives OK (88 tests) locally, and #329 failed the same way in the same window — so I expect a re-run to pass. It needs to actually be green.
3. The description mischaracterises #108
delineation between test methods is correct, and always was
The report in #108 shows two different methods (default, usingLayoutResName) under a single - test:. That is not the multiple-assertSame() case; it is a dropped - test: line between methods, and that header has no skipped: row, so it predates the current headerLineCount. It is fixed on main today, but "always was" is contradicted by the issue itself. I ran the issue's exact repro on this branch:
./gradlew LegacySample:screenshotTest -PtestClass=dev.testify.sample.clients.details.ClientDetailsViewScreenshotTest
./gradlew LegacySample:reportPull
- test:
name: default
baseline_image: assets/screenshots/37-1080x2220@440dp-en_US/ClientDetailsViewScreenshotTest_default.png
status: PASS
- test:
name: usingLayoutResName
baseline_image: assets/screenshots/37-1080x2220@440dp-en_US/ClientDetailsViewScreenshotTest_usingLayoutResName.png
status: PASSSo closing #108 is justified, and the new baseline_image values name files that exist. Please correct the description so the issue isn't closed with the wrong explanation attached.
Verified correct
getBaselinePath()now matchesloadBaselineBitmapForComparison()'sdescription.name.- The
hasCapturedOutputguard loses nothing: both assertions in one method resolve the same output file name, so the dropped pair would have been an exact duplicate. Library:ktlintCheck Library:testDebugUnitTestpass locally.- CHANGELOG hunk conflicts with the other five open code PRs.
|
Agreed on all three, and point 3 is a fair hit — I overclaimed. 1. Documented examples updated
Both were stale in a second way you did not mention: neither carried the On the root 2. The #108 characterisation — you are right, and I was wrongThe report in #108 shows The corroborating detail is the one you spotted: that header has no Description rewritten to say the cross-method bug was real and is already fixed on 3. CIAgreed it has to be actually green rather than argued green. Nothing in this PR touches the Legacy sample, and your local CHANGELOG conflictsNoted, and expected: all six code PRs insert at the same |
AndroidTestifyBot
left a comment
There was a problem hiding this comment.
Approved
All three points are resolved.
- Both documented
report.ymlexamples now show<Class>_<method>.png, and adding the missingskipped:row was a good catch on top of what I asked for. - You're right about the root
report.yml: it's gitignored and untracked. My mistake for calling it committed. - The description now says the cross-method bug in #108 was real and is already fixed on
main, and that this PR's guard is for multiple assertions in one method. That's accurate, and it matches the repro I ran on this branch earlier (two correctly separated entries forClientDetailsViewScreenshotTest). - CI is green on the new head, including
Legacy Sample,LibraryandPlugin. The docs build is clean.
Merge-order note only: the CHANGELOG hunk will still conflict with whichever of the other code PRs lands first.
What does this change accomplish?
Fixes #108
Two defects in
report.yml. The second is the one filed; the first was found while verifying it andis arguably worse.
1.
baseline_imagenames a file that does not exist.Reporter.getBaselinePath()built the path from the test method name, but baselines are stored as<Class>_<method>.png— that is whatloadBaselineBitmapForComparison()resolves, viaTestDescription.name. The committedreport.ymlin this repo shows the two side by side in a singleentry:
test_imageis right;baseline_imageis missing the class prefix. Everybaseline_imagelineTestify has ever written points at nothing, so any tool following that path gets a missing file.
2. Duplicate keys when a test asserts more than once.
startTest()runs once per test method, butcaptureOutput()runs from everyassertSame(). Asecond assertion appended another
baseline_image:/test_image:pair inside the same- test:entry, which is duplicate keys and therefore invalid YAML.
Worth being precise about the scope, because the issue title is broader than what is left to fix.
The cross-method bug #108 actually reports — two methods collapsed under one
- test:— was real,and is already fixed on
main. The report pasted into that issue has noskipped:row, so itpredates the current
insertHeader()/headerLineCount, and the issue's own repro now produces twoproperly separated entries. What remains broken is the multiple-assertions-per-method case, which is
what the guard below addresses.
How have you achieved it?
getBaselinePath()now usestestDescription.nameinstead oftestDescription.methodName, so itmatches the loader exactly.
captureOutput()records the image paths once per test, guarded by ahasCapturedOutputflag thatstartTest()resets.The guard is deliberately the conservative half of the fix. Emitting one report entry per assertion —
a nested
captures:list — is the shape the report probably wants long term, but it changes thereport contract that the Bitrise step and
reportShowconsume, and it is really part of #85(
assertSame(name)). This keeps the YAML valid today without pre-empting that design.Scope of Impact and Testing instructions
baseline_imagevalues change for every test, from<method>.pngto<Class>_<method>.png. That isthe fix, but anything parsing
report.ymland resolving that path should be aware.test_image,status,causeanddescriptionare untouched.The two places that document a
report.ymlsample are updated to match —docs/docs/get-started/8-use-gradle-plugin.mdandPlugins/Gradle/README.md. Both were stale in asecond way as well: neither showed the
skipped:row thatinsertSessionInfo()has emitted sincethe header rework, which is the same staleness that produced the report in #108.
CHANGELOG entries added under Unreleased for both.
Two existing assertions in
ReporterTestencoded the old, wrong path and are updated. Three testsadded:
getBaselinePath() matches the file name the baseline is loaded from— asserts againstTestDescription.nameso it fails if the two paths diverge again.captureOutput() called twice in one test records the image paths once.startTest() allows the next test to record its own image paths— guards against the flag stickingacross tests.
Verified locally: