Conversation
andresmr
force-pushed
the
claude/harness-wave-1-status-c5ff38
branch
2 times, most recently
from
August 25, 2026 09:44
bc3e6a8 to
cdc8a2e
Compare
Partial work, interrupted by a spend limit. Done here: - three assertThrows call sites in DataSetTableViewModelTest moved to kotlin.test.assertFailsWith - libs.junit.jupiter removed from the androidHostTest sets of login, sync, tracker and aggregates; unused koin-test-junit5 removed; catalog aliases dropped (koin-test-junit4 kept -- GetDataValueInputTest genuinely uses KoinTestRule) - .github/agents/testing.agent.md reduced to a pointer stub Still to do: the org.junit.jupiter exclusion guard in root build.gradle.kts, verifyAll + mustRunAfter, the jacoco 0.8.10 unpin, AGENTS.md, the skill split, and the Task 1 coverage investigation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Task 6 of the harness wave. The single 441-line SKILL.md became: - SKILL.md (122 lines) — a router: a "what am I testing" decision table mapping kind -> module type -> source set -> Gradle task, an explicit negative list of the four things that do NOT exist here (androidUnitTest/, androidInstrumentedTest/, src/desktopTest/, testAndroidDebugUnitTest), 10 invariants, a "confirm your test actually ran" check, and a Keeping this true rule. - references/instrumented-testing.md — verbatim move of the instrumented content; only edit is androidInstrumentedTest/ -> src/androidTest/, plus a note that commonskmm/login/sync declare androidDeviceTest with no tests yet. - references/unit-testing.md — new. 11 sections, each citing a real file in this repo rather than a synthetic skeleton. Facts verified against the tree rather than taken from the brief: - Turbine per module: login/aggregates declare it in commonTest, tracker in both (redundant), sync only in androidHostTest, commonskmm nowhere. - DomainError is `sealed class DomainError : Throwable()`, so mockito's doThrow rejects it and `catch (e: Exception)` does not catch it. - Dispatcher is a 3-field data class (io/main/default). - tracker is KMP; dhis2-mobile-program-rules is a plain com.android.library. The three worked examples from the old file were dropped in favour of pointers to canonical tests: the old Repository example used `whenever(...).thenThrow(d2Error)`, which is exactly the pattern that fails with "Checked exception is invalid for this method". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tasks 2 (remainder), 3 and 4 of the harness wave. - Exclude org.junit.jupiter from every configuration whose name contains "test". useJUnitPlatform() is set nowhere in this build, so a Jupiter @test was not run, it was silently never collected: the class compiled, the build went green and no result file was written. Verified by re-adding junit-jupiter to login's androidHostTest with a scratch Jupiter test -- the exclusion strips it and the build fails with "Unresolved reference 'jupiter'". Scratch reverted. - Register a verifyAll lifecycle task (ktlintCheck, testDebugUnitTest, testDhis2DebugUnitTest, testAndroidHostTest, jacocoReport across every project) and point run_tests.sh at it. ci.yml is deliberately untouched: its unit-tests job already needs the separate lint-check job and would run ktlint twice. The first version of this task resolved to ZERO dependencies and still reported BUILD SUCCESSFUL -- org.gradle.configureondemand=true means the subprojects are not configured while the root script runs, so eager findByName found nothing. Now resolved through a Provider at task-graph time, with a check() so an empty resolution fails instead of passing vacuously. - Drop the jacoco useVersion("0.8.10") override and wire the plugin's toolVersion to libs.versions.jacoco. Two claims in the brief did not survive checking: - jacocoReport does NOT need mustRunAfter. Its dependsOn loop in jacoco/jacoco.gradle.kts runs inside the task's registration action, which Gradle realises after AGP has created the unit-test tasks, so findByName resolves them even though apply(from=) precedes the android { } block. `./gradlew :app:jacocoReport --dry-run` puts testDhis2DebugUnitTest ahead of jacocoReport. - Removing the version override does not make the catalog's 0.8.15 apply. libs.jacoco is only on the buildscript classpath; the report engine follows the jacoco plugin's toolVersion, which falls through to Gradle's bundled 0.8.14. Hence the explicit wiring above; probed to confirm all 8 modules now report 0.8.15. Verified: ./gradlew ktlintFormat && ./run_tests.sh -> BUILD SUCCESSFUL, 1570 tests run, 1570 passed, 0 failed, across all 13 modules. All 8 coverage reports still parse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Task 5 of the harness wave.
AGENTS.md:
- Gradle task naming corrected: app is testDhis2DebugUnitTest, other AGP modules
testDebugUnitTest, KMP modules testAndroidHostTest for BOTH commonTest and
androidHostTest. States that testAndroidDebugUnitTest is not a task here and
that no module has a desktopTest source set.
- Project structure: tracker marked KMP, dhis2-mobile-program-rules marked a plain
com.android.library, and the three missing modules added (stock-usecase,
dhis_android_analytics, dhis2_android_maps). 13 modules, 5 KMP / 8 AGP.
- Source set listing: androidUnitTest -> androidHostTest, plus the note that
androidHostTest depends on commonTest so test deps need not be declared twice.
- Repository pattern: the "required imports" snippet replaced with
withDomainErrors { } / withDomainErrorsAsResult { } and the reason -- the SDK's
blocking RxJava operators rewrap the checked D2Error in a RuntimeException, so an
inline catch (d2Error: D2Error) misses every blocking call. Noted as landing with
ANDROAPP-7733.
- launchUseCase scoped: required in KMP ViewModels and any ViewModel with
instrumented coverage; the 140 legacy viewModelScope.launch sites are not being
migrated (count verified).
- Testing section reduced to five invariants plus a pointer to the skill.
The UseCase example was teaching the bug: it caught Exception, but DomainError is
`sealed class DomainError : Throwable()`, so that catch does not match and the error
escapes the try. Example now catches Throwable and rethrows CancellationException,
with a note explaining why.
Stale tokens cleared from the remaining files:
- .claude/skills/sentry-fix/SKILL.md (placement list + run commands)
- .claude/skills/sentry-triage/references/repo-map.md (task list; and line 127,
which correctly described the DESIGN-SYSTEM repo where Paparazzi tests really do
live in that source set -- reworded to stay true while dropping the literal token)
- .claude/agents/test-flow-architect.md
- .claude/skills/test-flow-planner/SKILL.md
A fourth brief claim did not survive checking: UseCase<R,T> is not confined to
login/sync/commonskmm. It is the dominant pattern with 33 implementations, and app
has the most (14), ahead of sync (9), login (6), tracker (3), commonskmm (1).
The grep gate as briefed is self-contradictory: it demands zero occurrences of the
three names while the skill is required to carry an explicit list saying those names
do not exist. The negative list is kept -- naming the wrong token is what makes the
warning usable -- and the gate is scoped to allow denials, documented in SKILL.md:
grep -rn "androidUnitTest\|androidInstrumentedTest\|testAndroidDebugUnitTest" \
--include="*.md" --include="*.kts" --include="*.yml" . \
| grep -v "/build/" | grep -v "does not exist\|not a task"
That returns nothing: 0 prescriptive uses remain across the repo.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The documented command contains the three names, so the gate flagged itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Documenting the command inline in SKILL.md made the gate match its own source: the patterns sat in a .md file, which is one of the file types it scans. The script is .sh, which the scan does not cover, so the gate stops flagging itself and there is one runnable definition instead of a command to copy by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jacoco/jacoco.gradle.kts writes to build/coverage-report/jacocoTestReport.xml, a non-default location, and nothing ever set sonar.coverage.jacoco.xmlReportPaths. The scanner therefore looked only at its own default path, found nothing, and reported no coverage for any module -- silently, because a missing coverage report is a warning, not an error, and CI hides even that (upload uses if-no-files-found: warn, download uses continue-on-error: true). Set on every project via the sonar extension, relative to each module's projectBaseDir. That matches the layout the CI artifact restores: the unit-tests job uploads '**/build/coverage-report/**', so download-artifact into '.' recreates <module>/build/coverage-report/jacocoTestReport.xml. Verified with an init-script probe that dumps the scanner's own computed property map before it connects: before: SONARPROP_COUNT_COVERAGE = 0 after: SONARPROP_COUNT_COVERAGE = 14 (13 modules + root) so this is the scanner's resolved configuration, not an inference from the source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ANDROAPP-7733 merged (#5071), so DomainErrorMapperExtensions.kt is on develop. Removes the 'lands with ANDROAPP-7733' notes from AGENTS.md and unit-testing.md, and corrects the call form: they are extensions on DomainErrorMapper, so the call is domainErrorMapper.withDomainErrors { }, not a bare withDomainErrors { }. Cites SyncDataSetRepositoryImpl as the worked example. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The relative path was accepted by the scanner but resolved to nothing in CI: the sensor logged "No coverage report can be found with sonar.coverage.jacoco.xmlReportPaths='build/coverage-report/jacocoTestReport.xml'" for every module, even though the artifact restores the reports at exactly <module>/build/coverage-report/jacocoTestReport.xml in the workspace root. Two candidates: the relative path is resolved against the wrong base dir, or the files are present but something removes them between the artifact download and the sensor. Absolute paths rule out the first. The COVERAGE_PROBE line reports whether the file exists when Gradle configures, which separates the two: exists=true with the sensor still reporting nothing means the files were deleted mid-build. The probe is temporary and comes out once the cause is confirmed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The premise was wrong. SonarCloud has been importing coverage all along: develop reports 10.5% today, with continuous history (18.8% through 2024, 9.3% from April 2025, 10.5% now). The assertion that it imported 0% for every module does not survive contact with the API. Two things misled me. The 'No coverage report can be found' warnings in CI are emitted only by the modules that genuinely have no report -- root and the 5 KMP modules, six in total -- not by all thirteen; with a relative path they all printed the same string so they looked identical. And the init-script probe read SonarTask.properties from a doFirst, which is before the scanner's own property computer has populated the map, so '0 coverage properties' was an artefact of reading too early rather than a finding. The property is kept as explicit configuration, not sold as a fix, and the temporary COVERAGE_PROBE logging is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drops sonar.coverage.jacoco.xmlReportPaths entirely. The premise for it was wrong -- SonarCloud has been importing coverage all along (develop reads 10.5%) -- so the property was redundant configuration justified by a bug that did not exist. Enables enableAndroidTestCoverage on the debug build type of app and form. Debug only, so release builds and the shipped APK are untouched. Confirmed it takes effect: createDhis2DebugCoverageReport tasks now exist. This does not move the coverage number on its own. jacocoReport reads instrumented data from build/outputs/code_coverage/**/*.ec, the UI tests run on BrowserStack, and nothing pulls those files back into the workspace. Instrumentation is the first of three steps; the remaining two are recorded in the PR description. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Coverage reported only unit tests. The UI suites run on BrowserStack's devices, so their JaCoCo execution data was written there and never came back, and the analysis ran alongside those jobs anyway rather than after them. Three parts: 1. scripts/fetchBrowserstackCoverage.sh (new). After a build finishes it reads .devices[].sessions[].id from GET /app-automate/espresso/v2/builds/<id>, then downloads each session's data from .../builds/<id>/sessions/<session>/coverage into browserstack-coverage/<label>/. It never fails its caller: missing coverage should not turn a green test run red. Runs before the pass/fail exit so a failed suite still contributes what it covered. 2. "coverage": true added to the three BrowserStack build requests, via a new browserstack_coverage setting. Confirmed jq emits it as a JSON boolean, not the string "true". 3. ci.yml: the three BrowserStack jobs upload what they retrieved; unit-tests now also uploads the raw **/build/jacoco/*.exec; code-quality waits on all of them, copies the .ec files into each reporting module, and regenerates the XML with `jacocoReport -x testDebugUnitTest -x testDhis2DebugUnitTest` before Sonar runs. One .ec covers the whole app process, so every module gets a copy; jacocoReport ignores classes outside its own classDirectories, so the duplication is harmless. Verified locally that jacocoReport picks up a .ec placed in build/outputs/code_coverage/browserstack/ and still produces a valid report. The BrowserStack half needs credentials and cannot be exercised from a workstation; the staging step logs the retrieved file count so an empty result is visible in the log instead of silently reverting to unit-tests-only. Cost: the Sonar result now arrives after the device suites rather than beside them. Also fixes scripts/config_jenkins.init, which had no trailing newline -- appending to it would have merged the new setting into browserstack_deviceOrientation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mmits The workflow had no concurrency block, unlike ci.yml, so its runs stacked up. Each takes several minutes, which is long enough for two to overlap, and each compares its regenerated metadata against the commit it checked out rather than against the branch tip. The slower run therefore still sees a difference after the faster one has already committed the same fix, and pushes a commit whose tree is byte-identical to the tip. Observed on this PR: 08:06:56 run A starts at ea6d250 08:23:47 run B starts at e2d3f34 08:41:02 run A pushes 397a0a2 (+18 lines -- legitimate) 08:46:10 run B pushes 04caca0 (identical tree -- empty) Each of those pushes cancels the in-flight CI run, because ci.yml sets cancel-in-progress, so code-quality never survived long enough to finish. Four runs were cancelled this way. Serialising per ref means only the newest run survives, and it always regenerates from the newest tip, so the comparison it makes is the right one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The exclusion list removed 82.3% of the app module's compiled classes: 1803 of 2191.
What the report described as the project's coverage was the coverage of the 18% that
survived the filter.
Measured per pattern on app's dhis2Debug output:
1046 **/*$* every nested class, lambda and coroutine state machine
664 **/*$*$*.*
332 **/*Module*.* anything with "Module" in the name, not just DI modules
197 **/*Activity*.*
188 **/*_Provide*Factory*.*
153 **/databinding/*.*
127 **/ui/*.class including plain mappers that happen to live under ui/
110 **/*Dialog*.*
96 **/*Component*.*
Only the generated ones belong there. The list now keeps R/BR/BuildConfig/Manifest,
Data Binding, Dagger/Hilt, other annotation processors, $WhenMappings, $$serializer,
ComposableSingletons, and test classes -- and nothing else. That takes exclusions from
82.3% to 25.4%, and classes analysed in app from 329 to 1074.
Android UI classes are no longer excluded on purpose: instrumented tests now contribute
their execution data, so those classes are genuinely exercised and hiding them
understates the very suite that covers them.
Expect the headline percentage to fall even so. On unit tests alone, app moves from
9.50% to 6.75% -- covered lines rise 1753 -> 2351, but the denominator grows far more,
because a large amount of previously hidden UI code is only reached by the instrumented
suite. The CI figure, which merges both, is the one to read.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The coverage fixes are self-contained and ready to merge, while the harness and guidance work is still a draft. Split so the first is not held up by the second. Moved to fix/coverage-measures-real-code (#5087): instrumented coverage retrieval from BrowserStack, the jacoco exclusion list, enableAndroidTestCoverage, the ci.yml coverage wiring, and the verify-dependency-metadata concurrency block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 0.8.10 pin is fatal once enableAndroidTestCoverage is on -- AGP runs JacocoTransform over every androidTest dependency and 0.8.10 cannot instrument byte-buddy 1.17.7 -- so the unpin has to travel with the instrumentation rather than with the harness work. Moved to fix/coverage-measures-real-code (#5087), along with the verification-metadata entries it produced. This branch keeps the pin as develop has it. verifyAll still lists jacocoReport; only the version handling moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
andresmr
force-pushed
the
claude/harness-wave-1-status-c5ff38
branch
from
August 31, 2026 12:17
c541939 to
d26d575
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Why
The test harness failed silently in several places, and its testing guidance existed
in three copies that disagreed with the repo.
useJUnitPlatform()is set nowhere, so a JUnit Jupiter@Testwas not "run andfailed" — it was never collected. The class compiled, the build went green, and no
result file was written.
androidUnitTest/,androidInstrumentedTest/andtestAndroidDebugUnitTest— none of which exist in this repository. A test placedin one of them is never compiled and never run, and nothing says so.
What changed
One test framework: JUnit4 +
kotlin.testorg.junit.jupiter.api.assertThrowscall sites inDataSetTableViewModelTest→kotlin.test.assertFailsWith.libs.junit.jupiterremoved from theandroidHostTestsets oflogin,sync,tracker,aggregates; unusedkoin-test-junit5removed; both aliases dropped fromthe catalog.
koin-test-junit4kept —GetDataValueInputTestgenuinely usesKoinTestRulewith@get:Rule.org.junit.jupiteris excluded from every configuration whose namecontains "test".
Verified by making it fail:
junit-jupiterwas re-added tologin'sandroidHostTestwith a scratch Jupiter test. The exclusion strips it and the build fails with
Unresolved reference 'jupiter'. Scratch reverted.One command for humans and CI
New
verifyAlllifecycle task coveringktlintCheck,testDebugUnitTest,testDhis2DebugUnitTest,testAndroidHostTestandjacocoReportacross every project.run_tests.shcalls it.ci.ymlis deliberately untouched — its unit-tests job alreadyneedsthe separatelint-check job, so swapping the step would run ktlint twice.
JaCoCo unpin
useVersion("0.8.10")removed, and the plugin'stoolVersionwired tolibs.versions.jacoco. All 8 modules verified on 0.8.15.Guidance collapsed into one source of truth
.github/agents/testing.agent.md→ a 30-line pointer stub (was 337 lines).AGENTS.md: task names corrected (app→testDhis2DebugUnitTest, other AGP →testDebugUnitTest, KMP →testAndroidHostTestfor both source sets);trackermarked KMP and
dhis2-mobile-program-rulesa plaincom.android.library; the threemissing modules added;
androidUnitTest→androidHostTest; the Repository "requiredimports" snippet replaced with
withDomainErrors { };launchUseCasescoped with the140 legacy
viewModelScope.launchsites explicitly excluded; Testing section reducedto five invariants plus a pointer.
.claude/skills/android-testing/— one 441-lineSKILL.mdbecame a 122-line routerplus
references/unit-testing.md(new, 11 sections, each citing a real file in thisrepo) and
references/instrumented-testing.md(verbatim move).sentry-fix,sentry-triage/repo-map.md,test-flow-architect.mdandtest-flow-planner.The grep gate is self-contradictory, and how that was resolved
The plan wanted zero occurrences of the three dead names repo-wide, while the skill has
to carry an explicit list stating they do not exist — and you cannot write
"
androidUnitTestdoes not exist" without writingandroidUnitTest.The negative list is kept, because naming the wrong token is what makes the warning
usable. The gate is scoped to allow denials and lives in
.claude/skills/android-testing/check-stale-tokens.sh— deliberately a.sh, becausethe scan covers
.md/.kts/.yml, so keeping the patterns out of a scanned file stopsthe gate matching its own source. (The first version documented the command inline in
SKILL.mdand promptly flagged itself.)Four claims in the original plan that did not survive checking
jacocoReportdoes not needmustRunAfter. ItsdependsOnloop runs inside thetask's registration action, which Gradle realises after AGP has created the
unit-test tasks, so
findByNameresolves them even thoughapply(from=)precedes theandroid { }block.--dry-runputstestDhis2DebugUnitTestahead ofjacocoReport.libs.jacocois only on the buildscript classpath;the report engine follows the plugin's
toolVersion, which falls through to Gradle9.5.1's bundled 0.8.14. Hence the explicit wiring.
loginandaggregatesdeclare it incommonTestdirectly, not "byinheritance". The real gaps are
sync(onlyandroidHostTest) andcommonskmm(nowhere).
UseCase<R,T>is not confined tologin/sync/commonskmm. 33 implementations,and
apphas the most (14).A correction worth recording
An earlier revision of this PR claimed SonarCloud imported 0% coverage, stated as
"confirmed, not suspected". That was wrong and has been withdrawn. Coverage was being
imported all along. Two bad inferences caused it: reading
SonarTask.propertiesfrom adoFirst, which is before the scanner's own property computer fills the map; and readingsix identical warning strings as "all thirteen modules failed" when only the six modules
without a report emit them. The real coverage problems were different, and are fixed in
#5087.
Verification
1570 vs the 1267 of a coverage-only run:
verifyAllpulls in the five KMP modules'testAndroidHostTest, whichjacocoReportalone never triggered.State
Everything above is complete, verified and pushed. Nothing here is half-finished. The
coverage work that was originally in this branch is now #5087 and is independent.
Next — for whoever picks this up
Roughly in order of value:
ForgotPinUseCaseandSavePinUseCase(login) catchException, which does notmatch
DomainError : Throwable(), so a mapped SDK error escapes uncaught instead ofbecoming
Result.failure.AGENTS.mdnow documents the correct shape while these twostill violate it. Note mockito's
doThrowrejectsDomainError— stub withthenAnswer { throw error }.verifyAllinci.yml, restructuring the lint/unit-test jobs so ktlint isnot run twice. Deliberately out of scope here.
sync'scommonTestand tocommonskmm, which has it nowhere.the 8 AGP modules only. Kover is the usual answer.
continue-on-error: true/if-no-files-found: warnfromci.yml, nowthat coverage genuinely works (fix: [ANDROAPP-7775] report the project's real test coverage #5087). They currently hide exactly the failure this
whole effort was about.
check-stale-tokens.sh, so the guidancecannot drift back.
:testingmodule for shared fixtures, test-bundle collapse, a new-codecoverage gate,
build-logic.Working agreement for this branch: if a test problem costs more than ~30 minutes, add
a row to the troubleshooting table in
references/unit-testing.mdin the same PR. Thatrule is written into the skill; it is the mechanism that keeps this from rotting again.
🤖 Generated with Claude Code