Skip to content

fix: [ANDROAPP-7775] report the project's real test coverage - #5154

Open
andresmr wants to merge 6 commits into
developfrom
ANDROAPP-7775-report-real-test-coverage
Open

andresmr wants to merge 6 commits into
developfrom
ANDROAPP-7775-report-real-test-coverage

Conversation

@andresmr

Copy link
Copy Markdown
Collaborator

Coverage was reported for unit tests only, over roughly a fifth of the codebase. Two
independent causes, both silent — a missing coverage report is a warning, not an error,
and CI hid even that (if-no-files-found: warn, continue-on-error: true).

Cause 1 — instrumented coverage never left BrowserStack

The UI suites run on BrowserStack's devices, so their JaCoCo execution data was written
there and nothing brought it back. code-quality also ran alongside those jobs rather
than after them, so it could not have used the data even if it had arrived.

Cause 2 — the exclusion list removed 82.3% of the code

jacoco/jacoco.gradle.kts excluded 1803 of 2191 compiled classes in app. What the
report called the project's coverage was the coverage of the 18% that survived.

Measured per pattern on app's dhis2Debug output:

classes removed pattern what it actually removed
1046 **/*$* every nested class, lambda and coroutine state machine
664 **/*$*$*.*
332 **/*Module*.* anything with "Module" in the name, not just DI modules
197 **/*Activity*.*
153 **/databinding/*.* (legitimately generated)
127 **/ui/*.class including plain mappers that happen to live under ui/
110 **/*Dialog*.*

Among the casualties of **/*$* were 197 named nested types carrying real logic:
sealed subclasses (ConvertTaskResult$Message), nested enums (OpenIdSession$LogOutReason),
$DefaultImpls — the actual bodies of interface default methods — and builders.

What changed

  1. Instrument debug buildsenableAndroidTestCoverage = true on the debug build
    type of app and form, so instrumented runs emit .ec data. Debug only; release
    builds and the shipped APK are unaffected.
  2. Retrieve the data — new scripts/fetchBrowserstackCoverage.sh reads
    .devices[].sessions[].id from GET /app-automate/espresso/v2/builds/<id> and
    downloads each session from .../sessions/<session>/coverage. "coverage": true
    added to the three build requests. The script never fails its caller — missing
    coverage should not turn a green test run red — and runs before the pass/fail exit so
    a failed suite still contributes what it covered.
  3. Merge before analysis — the BrowserStack jobs upload what they retrieved,
    unit-tests also uploads raw **/build/jacoco/*.exec, and code-quality now waits
    on all of them, copies the .ec files into each reporting module and regenerates the
    XML before Sonar runs.
  4. Exclude only generated code — the list now keeps R/BR/BuildConfig/Manifest, Data
    Binding, Dagger/Hilt, other annotation processors, $WhenMappings, $$serializer,
    ComposableSingletons and test classes, and nothing else. Android UI classes are no
    longer excluded on purpose: the instrumented suite exercises them, and hiding them
    understated the very tests that cover them.
  5. Unpin JaCoCo — required, not incidental. See below.
  6. verify-dependency-metadata.yml gets a concurrency block — see below.

Required: unpinning JaCoCo

Turning on instrumentation broke the build outright:

Execution failed for task ':form:mergeExtDexDebugAndroidTest'
  > Failed to transform mockito-kotlin-6.1.0.jar ... useJacocoTransformInstrumentation=true
    > Execution failed for JacocoTransform:
      .../byte-buddy/1.17.7/byte-buddy-1.17.7.jar

enableAndroidTestCoverage makes AGP run JacocoTransform over every dependency on
the androidTest runtime classpath. The root build pinned all org.jacoco artifacts to
0.8.10 through a resolutionStrategy, overriding whatever version AGP asked for, and
0.8.10 cannot instrument byte-buddy 1.17.7's bytecode. The pin predates instrumented
coverage and cannot survive it.

The pin is removed and the jacoco plugin's toolVersion is wired to
libs.versions.jacoco, so the catalog governs the report engine rather than Gradle's
bundled default. Modules that instrument (app, form) use AGP's own jacoco version for
the transform — the version AGP is tested against.

Verified locally that :form:mergeExtDexDebugAndroidTest, the task that failed, builds.

gradle/verification-metadata.xml gains the entries the newer artifacts require.

Result — verified on CI, every job green

instrumented coverage files retrieved: 3
job .ec retrieved
run-ui-landscape 1.3 MB
run-ui-portrait 1.3 MB
coverage lines measured lines covered
develop 10.6% 47,579 5,486
this branch 17.9% 61,224 12,298

Read the counts, not just the percentage. This measures 29% more code and credits
2.2× more covered lines than develop. Roughly 6,800 more lines of genuinely tested code are credited than on develop, and a
large body of untested UI code that was hidden from the denominator is now measured.

Exclusions: 82.3% → 25.4%. Classes analysed in app: 329 → 1074.

Also fixed: Verify Dependency Metadata pushed empty commits

That workflow had no concurrency block, so its runs stacked up. Each compares its
regenerated metadata against the commit it checked out rather than the branch tip, so the
slower of two overlapping runs still saw a difference after the faster one had already
committed the same fix, and pushed a commit with a byte-identical tree:

08:06:56  run A starts at ea6d2506c
08:23:47  run B starts at e2d3f3469
08:41:02  run A pushes 397a0a2da   (+18 lines -- legitimate)
08:46:10  run B pushes 04caca0c7   (identical tree -- empty)

Every such push cancelled the in-flight CI run. Four runs died this way while this work
was being verified. Serialising per ref means only the newest run survives, and it always
regenerates from the newest tip.

Note on the form suite

ANDROAPP-7748 (#5034) retired the form instrumentation CI job while this PR was open, and
this branch adopts that fully: the BrowserStack form script is deleted, form is out of
the coverage staging list, and form/build.gradle.kts is back to develop's version —
develop has no form/src/androidTest sources left, so instrumenting that module would
cost build time for data that cannot exist.

Instrumented coverage therefore comes from two device jobs rather than three. That is why
this figure moved from 19.1% when first measured to 17.9%: the denominator is unchanged at
61,224 — only device-sourced covered lines fell. The form module's replacement JVM tests
are counted through the normal unit-test path.

Trade-off

code-quality now waits on the device suites, so the Sonar result arrives later than it
used to. That is inherent to including their coverage.

Not in this PR

  • Making the metadata check compare against origin/<branch> rather than its own
    checkout. The concurrency block stops runs from overlapping, but the comparison is
    still against the wrong baseline if anything else pushes mid-run.
  • Deleting the stale SonarCloud required status check from the rulesets — nothing
    reports it, so it can never pass. Needs an admin. (SonarCloud Code Analysis and
    code-quality are both legitimate and should stay: one is the scan, the other the
    Quality Gate verdict.)

🤖 Generated with Claude Code

andresmr and others added 4 commits September 15, 2026 11:32
Coverage was reported for unit tests only, over a fifth of the codebase.

Two independent causes, both silent:

1. The UI suites run on BrowserStack's devices, so their JaCoCo execution data was
   written there and never came back. Nothing downloaded it, and code-quality ran
   alongside those jobs rather than after them, so it could not have used it anyway.

2. The exclusion list in jacoco/jacoco.gradle.kts removed 82.3% of the app module's
   compiled classes -- 1803 of 2191. What the report called 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*.*
   153  **/databinding/*.*
   127  **/ui/*.class     including plain mappers that happen to live under ui/
   110  **/*Dialog*.*

Changes:

- enableAndroidTestCoverage on the debug build type of app and form, so instrumented
  runs emit .ec data. Debug only; release builds and the shipped APK are unaffected.
- scripts/fetchBrowserstackCoverage.sh reads .devices[].sessions[].id from
  GET /app-automate/espresso/v2/builds/<id> and downloads each session's data from
  .../sessions/<session>/coverage. "coverage": true added to the three build requests.
  The script never fails its caller, and runs before the pass/fail exit so a failed
  suite still contributes what it covered.
- ci.yml: the BrowserStack jobs upload what they retrieved, unit-tests also uploads raw
  **/build/jacoco/*.exec, and code-quality now waits on all of them, copies the .ec
  files into each reporting module and regenerates the XML before Sonar runs.
- The exclusion list keeps only genuinely generated code: R/BR/BuildConfig/Manifest,
  Data Binding, Dagger/Hilt, other annotation processors, $WhenMappings, $$serializer,
  ComposableSingletons and test classes. Exclusions drop from 82.3% to 25.4%, and
  classes analysed in app from 329 to 1074. Android UI classes are no longer excluded
  on purpose: the instrumented suite exercises them, and hiding them understated it.
- verify-dependency-metadata.yml gets a concurrency block. Without one its runs stacked
  up, and each compares its regenerated metadata against the commit it checked out
  rather than the branch tip, so the slower of two overlapping runs pushed a commit with
  a byte-identical tree. Each such push cancelled the in-flight CI run; four runs died
  that way while this work was being verified.

Verified on CI, every job green:

  instrumented coverage files retrieved: 3   (294 KB + 1.3 MB + 1.3 MB)

                                  coverage   lines measured   lines covered
  develop today                       10.5%           47,500          ~4,988
  this branch                         19.1%           61,145          13,080

28% more code measured, 2.6x more covered lines credited.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
build-test-apks failed on this branch:

  Execution failed for task ':form:mergeExtDexDebugAndroidTest'
    > Failed to transform mockito-kotlin-6.1.0.jar ... useJacocoTransformInstrumentation=true
      > Execution failed for JacocoTransform:
        .../byte-buddy/1.17.7/byte-buddy-1.17.7.jar

enableAndroidTestCoverage makes AGP run JacocoTransform over every dependency on the
androidTest runtime classpath. The root build pinned every org.jacoco artifact to
0.8.10 through a resolutionStrategy, overriding the version AGP asked for, and 0.8.10
cannot instrument byte-buddy 1.17.7's bytecode. The pin predates instrumented coverage
and cannot survive it.

Removes the pin and wires the jacoco plugin's toolVersion to libs.versions.jacoco, so
the catalog governs the report engine instead of Gradle's bundled default. Modules that
instrument (app, form) take AGP's own jacoco version for the transform, which is the
version AGP is tested against.

Verified locally: :form:mergeExtDexDebugAndroidTest, the task that failed, now builds.

This change was originally in the harness PR. It belongs here: instrumented coverage is
what makes the old pin fatal, so the two cannot land separately.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removed the failure history and trade-off commentary: why the 0.8.10 pin broke
instrumentation, what the old exclusion list hid, how the metadata workflow raced itself,
and the note about Sonar turnaround. That belongs in the PR and the ticket, not beside
the code.

What is left says what each piece does where that is not obvious: which consumer the
uploaded .exec is for, why every module gets a copy of the same .ec, what toolVersion
governs, what each exclusion group covers, and the fetch script's contract.

Comment-only; no behaviour change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
andresmr and others added 2 commits September 15, 2026 12:19
Gating it on the UI jobs succeeding threw away the coverage they had already
retrieved: fetchBrowserstackCoverage runs before the pass/fail exit, so the .ec files
exist whether or not a test failed, but a skipped code-quality never merges them and
Sonar reports nothing.

!cancelled() keeps the ordering - the analysis still runs after the device jobs - while
letting it consume whatever was collected.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The list had already drifted: form applies jacoco and produces a jacocoReport, but was
dropped from the hardcoded list when its instrumentation job was retired. One .ec covers
the whole app process, including form's classes, so form was losing instrumented
coverage.

Deriving it from the modules that apply jacoco.gradle.kts keeps the two in step - 8
modules today, and the list cannot go stale when one is added or removed. The step logs
what it staged into.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants