Add CodeCov - #28
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
| informational: true | ||
| patch: | ||
| default: | ||
| target: 90% |
There was a problem hiding this comment.
Must fix — the patch target is not met by this PR itself.
codecov/patch on this head commit reports 69.07% of diff hit (target 90.00%), so the check this file introduces is red on the very change that adds it, and it will stay red for anyone rebasing on it.
Reproduced locally with ./gradlew --no-configuration-cache :gradle-plugin:jacocoTestReport. The uncovered new lines are all error-handling wrappers plus one synthetic overload:
InstallationDirectory.kt66–69, 142–147, 182–185 — theIOException→GradleExceptionwrappers inprepare(),createDirectoriesSafely(), andisRedirectingFileSystemEntry().Checksum.kt191–192 — theIOExceptioncatch inreadChecksumMetadata.Checksum.kt152, 155 — the synthetic default-argument overload ofreadGitHubReleaseMetadata(see the separate comment there).
Smallest correction: either cover the reachable ones (a read-only installation root exercises the prepare() wrapper; a closed loopback socket exercises the readChecksumMetadata catch) and drop the default argument, or set a target this repository can actually sustain and raise it later. Merging a permanently failing status check is the outcome to avoid.
| internal fun readGitHubReleaseMetadata( | ||
| source: URI, | ||
| githubToken: String?, | ||
| readMetadata: (URI, String?) -> String = ::readChecksumMetadata, |
There was a problem hiding this comment.
Should fix — the defaulted test seam leaves the production call path uncovered and deviates from the neighbouring function.
resolveExpectedAssetSha256 (line 211 in this same file) takes readMetadata: (URI) -> String as a required parameter, and InstallEmbedCodeTask passes the reader explicitly. This new function adds a default instead, which has two consequences:
- Kotlin emits a synthetic
readGitHubReleaseMetadata$defaultfor the two-argument production call.ChecksumSpeconly ever calls the three-argument form, and the TestKit builds configuresha256explicitly so the metadata reader is never reached — lines 152 and 155 are fully uncovered in the generated JaCoCo report, and they count against the 90% patch target added incodecov.yml. - The reader that production actually uses is no longer visible at the call site.
Smallest correction: drop = ::readChecksumMetadata and pass ::readChecksumMetadata from InstallEmbedCodeTask, matching resolveExpectedAssetSha256.
Separately, the KDoc above does not mention readMetadata at all. Per the KDoc policy, an internal declaration needs KDoc where it explains a non-obvious contract — an injected reader qualifies.
| uses: codecov/codecov-action@v7 | ||
| with: | ||
| files: ./gradle-plugin/build/reports/jacoco/test/jacocoTestReport.xml | ||
| fail_ci_if_error: true |
There was a problem hiding this comment.
Should fix — fail_ci_if_error: true combined with a repository secret makes fork pull requests fail.
The workflow triggers on pull_request. GitHub does not expose secrets.* to workflow runs from forked repositories, so token resolves to an empty string there. codecov-action then falls back to a tokenless upload, which is rate-limited, and fail_ci_if_error: true turns any upload hiccup into a red check on a contribution the author cannot fix.
Smallest correction: either drop fail_ci_if_error: true (coverage reporting is advisory here — project is already informational: true), or gate the upload step with if: github.event.pull_request.head.repo.full_name == github.repository.
| } | ||
|
|
||
| @Test | ||
| fun `reject coverage collection with the configuration cache`() { |
There was a problem hiding this comment.
Should fix — this test verifies the spec's own helper, not the plugin.
assertThrows(IllegalArgumentException::class.java) { runner(...) } asserts on the require(...) at line 1200 of this same file. No Gradle build runs, no plugin code executes, and the assertion breaks the moment the helper's message is reworded.
This is a TestKit consumer-build suite; per the test-engineer skill each spec should prove a consuming-build contract. The invariant here is a private precondition — the two callers that need it already pass collectCoverage = false explicitly, and a misuse fails loudly at development time anyway.
Smallest correction: delete the test and keep the require.
| /** | ||
| * Verifies that [directory] resolves below the real installation root. | ||
| */ | ||
| internal fun requireRealPathInside(directory: Path) { |
There was a problem hiding this comment.
Should fix — visibility widened purely for test access.
requireRealPathInside was private before the move out of InstallEmbedCodeTask; it is now internal so that InstallationDirectorySpec can call it directly. Reaching it through the public entry point is impossible by design: createDirectoriesSafely already rejects every redirecting component before this final check runs, which is why the spec has to bypass it. Widening the boundary to test an unreachable defence-in-depth branch trades a real invariant (this method is an internal postcondition of createDirectoriesSafely) for a coverage line.
Smallest correction: restore private and drop the second test in InstallationDirectorySpec, or keep the test and note in KDoc why the method is separately reachable.
Minor, same file: every member is marked internal inside an already-internal class, so the modifier adds nothing. The original nested class relied on the enclosing visibility.
| private companion object { | ||
| const val TEST_KIT_COVERAGE_JVM_ARGUMENT_PROPERTY = | ||
| "io.spine.embedcode.gradle.testkit.coverage.jvm-argument" | ||
| const val DEFAULT_GRADLE_DAEMON_JVM_ARGUMENTS = |
There was a problem hiding this comment.
Nit — the hardcoded daemon arguments silently restate Gradle's defaults.
-Dorg.gradle.jvmargs=... replaces the daemon JVM arguments rather than appending to them, which is why the JaCoCo agent argument has to be prefixed with these values. That reason is not recorded anywhere, and -Xmx512m -XX:MaxMetaspaceSize=384m pins today's Gradle defaults: if a future Gradle raises them, coverageFunctionalTest will quietly run its daemons with less memory than functionalTest does, and the two suites will diverge under load.
Smallest correction: add a short comment stating that the constant exists because org.gradle.jvmargs is replaced, not appended.
| import java.net.HttpURLConnection | ||
| import java.net.URI | ||
| import java.net.URLEncoder | ||
| import java.net.URLConnection |
There was a problem hiding this comment.
Nit — import out of order.
java.net.URLConnection sorts before java.net.URLEncoder (C < E), but it is added after it. Every other import block in this file is lexicographic.
| import java.util.HexFormat | ||
|
|
||
| private const val SHA256_LENGTH = 64 | ||
| private const val CHECKSUM_METADATA_CONNECT_TIMEOUT_MILLIS = 30_000 |
There was a problem hiding this comment.
Nit — duplicated timeout values.
These two constants carry exactly the values of InstallEmbedCodeTask.CONNECT_TIMEOUT_MILLIS and READ_TIMEOUT_MILLIS (still present and still used for the asset download at InstallEmbedCodeTask.kt:520-521). The move split one HTTP timeout policy into two independent sources of truth that will drift.
Smallest correction: keep one pair of constants and reference it from both call sites.
| threshold: 0% | ||
| only_pulls: true | ||
|
|
||
| ignore: |
There was a problem hiding this comment.
Nit — these ignore entries have no effect.
The uploaded report is gradle-plugin/build/reports/jacoco/test/jacocoTestReport.xml, whose classDirectories come from the plugin module's main source set only. Neither buildSrc/** (a separate build, never in this report) nor scripts/** (Python) can appear in it, so Codecov has nothing to ignore.
Harmless, but it reads as if buildSrc coverage were being deliberately suppressed. Worth either removing, or keeping with a comment that it guards against a future report that does include them.
Oleg-Melnik
left a comment
There was a problem hiding this comment.
@Vladyslav-Kuksiuk LGTM with comments to address.
This PR adds CodeCov to verify the test coverage.