From aa61182e84cb7d3e92881c3e9f79b2600c206315 Mon Sep 17 00:00:00 2001 From: Florian Dreier Date: Tue, 1 Sep 2026 09:47:48 +0200 Subject: [PATCH] TS-47712 Record the coverage of the JVMs our tests spawn Most of what this repository ships is executed during the build, but not in a JVM that anyone instruments, so Teamscale reports it as untested. The Mojos of the Maven plugin, the report generator they call into and the whole Gradle plugin run in the Maven and Gradle builds that the system tests and the TestKit tests start as child processes. The JaCoCo agent that com.teamscale.coverage attaches reaches only the test JVM itself, which does little more than run a mock Teamscale server: the jacocoTestReport.xml of every system test was an empty stub, because a system test has no production code of its own to fill the report's class directories with either. The new com.teamscale.spawned-jvm-coverage convention plugin attaches a plain JaCoCo agent to those child processes and folds what it records into the project's jacocoTestReport, which is the report CI uploads. It also lets the consuming build script name the projects whose classes run in there, so that the report can be mapped onto their sources; they are resolved through a configuration rather than read from the projects, which is what keeps this compatible with project isolation. The agent reaches Maven through MAVEN_OPTS, whose JVM is the one the Mojos run in, and Gradle through org.gradle.jvmargs, since its build logic runs in the daemon rather than in the launcher that GRADLE_OPTS would reach. JAVA_TOOL_OPTIONS would have covered both plus the forked test JVMs, but it announces itself on the stderr of every JVM, which sut-uses-logback-test asserts is empty, and it would put a second agent into the JVMs that the profiler already instruments. The include pattern deliberately does not match the shadow.com.teamscale.* classes of the profiler, which AgentOptions.DEFAULT_EXCLUDES keeps the profiler itself from instrumenting, so the two agents record a disjoint set of classes and cannot interfere with each other. Coverage of the classes involved, as Teamscale sees it across all uploaded reports: the Gradle plugin's own report goes from 2 of 284 methods to 188, the Maven plugin from 6 of 102 to 81, the report generator from 182 of 284 to 235 and the client from 54 of 198 to 96. Branch and pull request builds now upload their coverage as well. They ran every test but uploaded nothing, so until a change reached master all of its methods looked untested. On a pull request the checkout is the synthetic merge commit, which does not exist in the repository, so the head commit is named explicitly rather than left to the commit auto detection. Builds of pull requests from forks are still skipped, since they get no secrets. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/actions.yml | 22 +++- ....teamscale.spawned-jvm-coverage.gradle.kts | 118 ++++++++++++++++++ .../teamscale/test/commons/ProcessUtils.kt | 83 +++++++++++- .../cucumber-maven-tia/build.gradle.kts | 8 +- system-tests/gradle-cucumber/build.gradle.kts | 8 +- .../gradle-multi-module/build.gradle.kts | 8 +- .../build.gradle.kts | 6 + system-tests/tia-maven/build.gradle.kts | 9 +- teamscale-gradle-plugin/build.gradle.kts | 2 +- .../com/teamscale/TeamscalePluginTestBase.kt | 15 +++ 10 files changed, 266 insertions(+), 13 deletions(-) create mode 100644 buildSrc/src/main/kotlin/com.teamscale.spawned-jvm-coverage.gradle.kts diff --git a/.github/workflows/actions.yml b/.github/workflows/actions.yml index 743ac38e0..6774edb30 100644 --- a/.github/workflows/actions.yml +++ b/.github/workflows/actions.yml @@ -71,7 +71,11 @@ jobs: ORG_GRADLE_PROJECT_dockerHubUsername: ${{ secrets.DOCKERHUB_USER }} ORG_GRADLE_PROJECT_dockerHubPassword: ${{ secrets.DOCKERHUB_TOKEN }} - name: Upload coverage to Teamscale - if: always() && github.event_name == 'push' + # Fork pull requests get no secrets, so the upload can only run for pushes and for pull requests + # from branches in this repository. github.event.pull_request is null on push events. + if: >- + always() && (github.event_name == 'push' + || github.event.pull_request.head.repo.full_name == github.repository) uses: cqse/teamscale-upload-action@v2.10.0 with: server: 'https://cqse.teamscale.io' @@ -81,6 +85,11 @@ jobs: partition: 'Coverage' format: 'JACOCO' message: 'Linux Coverage' + # On pull_request, actions/checkout checks out the synthetic refs/pull/N/merge commit and + # GitHub sets GITHUB_SHA to it, which is what teamscale-upload's commit auto-detection picks + # up. That commit is not reachable from any branch, so Teamscale never fetches it and cannot + # resolve the revision. Name the pull request's head commit instead. + revision: ${{ github.event.pull_request.head.sha || github.sha }} files: '**/jacocoTestReport.xml' test-windows: @@ -96,7 +105,11 @@ jobs: - name: Build with Gradle run: ./gradlew build --max-workers=2 - name: Upload coverage to Teamscale - if: always() && github.event_name == 'push' + # Fork pull requests get no secrets, so the upload can only run for pushes and for pull requests + # from branches in this repository. github.event.pull_request is null on push events. + if: >- + always() && (github.event_name == 'push' + || github.event.pull_request.head.repo.full_name == github.repository) uses: cqse/teamscale-upload-action@v2.10.0 with: server: 'https://cqse.teamscale.io' @@ -106,4 +119,9 @@ jobs: partition: 'Coverage Windows' format: 'JACOCO' message: 'Coverage Windows' + # On pull_request, actions/checkout checks out the synthetic refs/pull/N/merge commit and + # GitHub sets GITHUB_SHA to it, which is what teamscale-upload's commit auto-detection picks + # up. That commit is not reachable from any branch, so Teamscale never fetches it and cannot + # resolve the revision. Name the pull request's head commit instead. + revision: ${{ github.event.pull_request.head.sha || github.sha }} files: '**/jacocoTestReport.xml' diff --git a/buildSrc/src/main/kotlin/com.teamscale.spawned-jvm-coverage.gradle.kts b/buildSrc/src/main/kotlin/com.teamscale.spawned-jvm-coverage.gradle.kts new file mode 100644 index 000000000..b74e42606 --- /dev/null +++ b/buildSrc/src/main/kotlin/com.teamscale.spawned-jvm-coverage.gradle.kts @@ -0,0 +1,118 @@ +import org.gradle.api.artifacts.component.ProjectComponentIdentifier +import java.io.Serializable + +// Records the coverage of our own production classes inside the JVMs that this project's tests spawn. +// +// Those tests do their real work in the Maven and Gradle builds they start as child processes, so the JaCoCo +// agent that com.teamscale.coverage attaches to the test JVM never sees the classes that matter: the Mojos of +// the Maven plugin, the tasks of the Gradle plugin and the report generator they call into. This plugin +// attaches a second, plain JaCoCo agent to those child processes and folds what it records into the project's +// jacocoTestReport, which is the report that CI uploads. + +plugins { + id("com.teamscale.coverage") +} + +// The version catalog accessors are not available in precompiled script plugins, cf. com.teamscale.java-convention. +val catalogs = extensions.getByType() +val libs = catalogs.named("libs") + +/** + * The plain JaCoCo agent that instruments our classes in the spawned JVMs. Not the profiler, cf. the note above. + * Deliberately not named jacocoAgent: inside dependencies {} that name binds to the accessor of the JaCoCo + * plugin's own configuration of that name rather than to this variable, which would silently leave this one empty. + */ +val spawnedJvmJacocoAgent = configurations.dependencyScope("spawnedJvmJacocoAgent") +val jacocoAgentJar = configurations.resolvable("spawnedJvmJacocoAgentPath") { + extendsFrom(spawnedJvmJacocoAgent.get()) +} + +/** + * The projects whose production classes run inside the spawned JVMs, declared by the consuming build script. + * Their class and source directories are resolved through this configuration instead of being read from the + * projects directly, which is what keeps the build compatible with project isolation. + */ +val spawnedJvmCode = configurations.dependencyScope("spawnedJvmCode") +val spawnedJvmCodePath = configurations.resolvable("spawnedJvmCodePath") { + extendsFrom(spawnedJvmCode.get()) + // Every project that runs in a spawned JVM is named explicitly, so that the report does not also list the + // classes that such a project merely depends on. Those would show up as entirely uncovered, because they + // either do not run at all or, like the profiler, only run under their relocated names. + isTransitive = false + // The attributes of a runtime classpath. Asking for Bundling.EXTERNAL is also what selects the plain + // variant of the projects that publish a shaded jar, whose classes we could not map back to the sources. + attributes { + attribute(Usage.USAGE_ATTRIBUTE, objects.named(Usage.JAVA_RUNTIME)) + attribute(Category.CATEGORY_ATTRIBUTE, objects.named(Category.LIBRARY)) + attribute(LibraryElements.LIBRARY_ELEMENTS_ATTRIBUTE, objects.named(LibraryElements.JAR)) + attribute(Bundling.BUNDLING_ATTRIBUTE, objects.named(Bundling.EXTERNAL)) + attribute( + TargetJvmEnvironment.TARGET_JVM_ENVIRONMENT_ATTRIBUTE, + objects.named(TargetJvmEnvironment.STANDARD_JVM) + ) + } +} + +dependencies { + spawnedJvmJacocoAgent("org.jacoco:org.jacoco.agent:${libs.findVersion("jacoco").get().requiredVersion}:runtime") +} + +/** + * The file that every JVM spawned by this project's tests appends its coverage to. One file for all of them is + * safe: JaCoCo takes an exclusive lock on the destination file before writing it and reads the resulting + * concatenation back as consecutive sessions. + */ +val spawnedJvmExecutionData = layout.buildDirectory.file("jacoco/spawnedJvms.exec") + +tasks.test { + // Resolved eagerly so that the option is a plain string, cf. multiple-agents-test, which attaches a + // foreign JaCoCo agent to its own test JVM the same way. + val agentJar = jacocoAgentJar.get().singleFile + val destination = spawnedJvmExecutionData.get().asFile + outputs.file(spawnedJvmExecutionData) + // The include pattern matches the VM names of the classes, so it deliberately does not match the + // shadow.com.teamscale.* classes of the profiler, which is attached to some of the same JVMs. Both + // agents therefore instrument a disjoint set of classes and cannot interfere with each other. + // The name of the property is repeated in ProcessUtils, which reads it. + systemProperty( + "systemTestCoverageAgent", + "-javaagent:$agentJar=destfile=$destination,append=true,output=file,dumponexit=true,jmx=false," + + "includes=com.teamscale.*" + ) + doFirst("deleteSpawnedJvmCoverage", DeleteFile(destination)) +} + +tasks.jacocoTestReport { + executionData(spawnedJvmExecutionData) + // A system test has no production code of its own, so without these its report stays empty no matter how + // much coverage was recorded. Projects that do have production code, like the Gradle plugin, declare + // nothing here and keep the class directories that the jacoco plugin derives from their own source set. + classDirectories.from(spawnedJvmCodePath.get().incoming.artifactView { + componentFilter { it is ProjectComponentIdentifier } + attributes.attribute( + LibraryElements.LIBRARY_ELEMENTS_ATTRIBUTE, objects.named(LibraryElements.CLASSES) + ) + }.files) + // Only needed for the HTML report; Teamscale maps the XML report onto the sources by package and file + // name. Resolved leniently so that a project without a sources variant cannot fail the build over it. + sourceDirectories.from(spawnedJvmCodePath.get().incoming.artifactView { + withVariantReselection() + lenient(true) + componentFilter { it is ProjectComponentIdentifier } + attributes { + attribute(Category.CATEGORY_ATTRIBUTE, objects.named(Category.VERIFICATION)) + attribute(Bundling.BUNDLING_ATTRIBUTE, objects.named(Bundling.EXTERNAL)) + attribute( + VerificationType.VERIFICATION_TYPE_ATTRIBUTE, + objects.named(VerificationType.MAIN_SOURCES) + ) + } + }.files) +} + +/** Deletes the given file before the task runs, so that a run never reports the coverage of the previous one. */ +class DeleteFile(private val file: File) : Action, Serializable { + override fun execute(t: Task) { + file.delete() + } +} diff --git a/common-system-test/src/main/kotlin/com/teamscale/test/commons/ProcessUtils.kt b/common-system-test/src/main/kotlin/com/teamscale/test/commons/ProcessUtils.kt index 649be4514..e1e299e85 100644 --- a/common-system-test/src/main/kotlin/com/teamscale/test/commons/ProcessUtils.kt +++ b/common-system-test/src/main/kotlin/com/teamscale/test/commons/ProcessUtils.kt @@ -101,6 +101,7 @@ object ProcessUtils { val exitCode = executeWithoutConcurrencyLimit( builder, input, -1, consoleCharset, stdoutConsumer, stderrConsumer ) + awaitCoverageOfSingleUseDaemon(commands) return ProcessResult( stdout = stdoutConsumer.content, stderr = stderrConsumer.content, @@ -117,11 +118,14 @@ object ProcessUtils { * * @return ProcessBuilder configured with commands and working directory */ - fun build(): ProcessBuilder = ProcessBuilder(commands.withDebuggerArgumentIfRequested()).apply { - workingDirectory?.let { directory(it) } - environmentVariables?.let { environment().putAll(it) } - removeEnvironmentVariables.forEach { environment().remove(it) } - } + fun build(): ProcessBuilder = + ProcessBuilder(commands.withDebuggerArgumentIfRequested().withCoverageArgumentIfRequested()).apply { + workingDirectory?.let { directory(it) } + // Merged before the caller's variables, so that a test that sets MAVEN_OPTS itself still wins. + addCoverageEnvironment(commands) + environmentVariables?.let { environment().putAll(it) } + removeEnvironmentVariables.forEach { environment().remove(it) } + } } /** @@ -148,6 +152,75 @@ object ProcessUtils { private fun String.isJavaExecutable() = substringAfterLast('/').substringAfterLast('\\').removeSuffix(".exe") == "java" + /** + * The `-javaagent` option that records the coverage of our own classes in the JVMs a system test spawns, set + * by the com.teamscale.spawned-jvm-coverage convention plugin. Absent unless that plugin is applied. + * + * Both `MAVEN_OPTS` and the Gradle command line split the option on whitespace, so a path containing a space + * would produce a broken JVM command line. Recording coverage is a nice to have, the system tests are not, + * so we rather record nothing in that case. + */ + private val COVERAGE_AGENT_ARGUMENT: String? = + System.getProperty("systemTestCoverageAgent")?.takeIf { it.isNotEmpty() && it.none(Char::isWhitespace) } + + /** + * Attaches [COVERAGE_AGENT_ARGUMENT] to a Gradle invocation, whose build logic runs in the daemon rather than + * in the launcher that `GRADLE_OPTS` would reach. `--no-daemon` makes that daemon exit with the build, which + * is when JaCoCo writes what it recorded. + */ + private fun List.withCoverageArgumentIfRequested(): List { + val agent = COVERAGE_AGENT_ARGUMENT?.takeIf { isLauncherFor("gradlew") } ?: return this + return this + listOf("-Dorg.gradle.jvmargs=$agent", "--no-daemon") + } + + /** + * Attaches [COVERAGE_AGENT_ARGUMENT] to a Maven invocation. The wrapper boots Maven in the JVM it starts, so + * `MAVEN_OPTS` reaches exactly the JVM the Mojos run in and leaves the forked test JVMs alone. Those already + * have the profiler attached, and a second agent in them would only duplicate what the profiler records. + */ + private fun ProcessBuilder.addCoverageEnvironment(commands: List) { + val agent = COVERAGE_AGENT_ARGUMENT?.takeIf { commands.isLauncherFor("mvnw") } ?: return + environment().merge("MAVEN_OPTS", agent) { inherited, added -> "$inherited $added" } + } + + /** + * Whether this command line invokes the given wrapper script, with or without a path and the Windows file + * extension. On Windows the script is invoked through `cmd /c`, so it is not necessarily the first argument. + */ + private fun List.isLauncherFor(wrapperScript: String) = any { + it.substringAfterLast('/').substringAfterLast('\\') + .removeSuffix(".cmd").removeSuffix(".bat") == wrapperScript + } + + /** + * Waits until the single-use daemon of a Gradle build we recorded coverage of has written what it recorded. + * + * `--no-daemon` runs the build in a daemon that the launcher forks and that shuts itself down once the build + * finished, so it outlives the process we just waited for. JaCoCo writes its data from a shutdown hook of + * that daemon, which without this would race the jacocoTestReport task and cost us most of the coverage. + * Maven needs none of this: it runs the build in the very JVM that we started. + */ + private fun awaitCoverageOfSingleUseDaemon(commands: List) { + val destination = COVERAGE_AGENT_ARGUMENT + ?.takeIf { commands.isLauncherFor("gradlew") } + ?.substringAfter("destfile=")?.substringBefore(',') + ?.let { File(it) } ?: return + var previousLength = -1L + repeat(COVERAGE_FLUSH_POLLS) { + val length = if (destination.isFile) destination.length() else 0 + // Two polls in a row that saw the same non-empty file. JaCoCo holds an exclusive lock while it + // writes, so a length that stopped growing means the daemon is done rather than merely slow. + if (length > 0 && length == previousLength) return + previousLength = length + Thread.sleep(COVERAGE_FLUSH_POLL_INTERVAL_MILLIS) + } + } + + private const val COVERAGE_FLUSH_POLL_INTERVAL_MILLIS = 100L + + /** Polls for at most five seconds, which is far more than the daemon has ever needed to shut down. */ + private const val COVERAGE_FLUSH_POLLS = 50 + /** * Immutable result of process execution. */ diff --git a/system-tests/cucumber-maven-tia/build.gradle.kts b/system-tests/cucumber-maven-tia/build.gradle.kts index 36400e900..be75224bf 100644 --- a/system-tests/cucumber-maven-tia/build.gradle.kts +++ b/system-tests/cucumber-maven-tia/build.gradle.kts @@ -1,7 +1,13 @@ plugins { com.teamscale.`kotlin-convention` com.teamscale.`system-test-convention` - com.teamscale.coverage + com.teamscale.`spawned-jvm-coverage` +} + +dependencies { + spawnedJvmCode(project(":teamscale-maven-plugin")) + spawnedJvmCode(project(":report-generator")) + spawnedJvmCode(project(":teamscale-client")) } tasks.test { diff --git a/system-tests/gradle-cucumber/build.gradle.kts b/system-tests/gradle-cucumber/build.gradle.kts index 4edcc7677..3f11094ec 100644 --- a/system-tests/gradle-cucumber/build.gradle.kts +++ b/system-tests/gradle-cucumber/build.gradle.kts @@ -1,7 +1,13 @@ plugins { com.teamscale.`kotlin-convention` com.teamscale.`system-test-convention` - com.teamscale.coverage + com.teamscale.`spawned-jvm-coverage` +} + +dependencies { + spawnedJvmCode(project(":teamscale-gradle-plugin")) + spawnedJvmCode(project(":report-generator")) + spawnedJvmCode(project(":teamscale-client")) } tasks.test { diff --git a/system-tests/gradle-multi-module/build.gradle.kts b/system-tests/gradle-multi-module/build.gradle.kts index ac31019c7..9223c3634 100644 --- a/system-tests/gradle-multi-module/build.gradle.kts +++ b/system-tests/gradle-multi-module/build.gradle.kts @@ -1,7 +1,13 @@ plugins { com.teamscale.`kotlin-convention` com.teamscale.`system-test-convention` - com.teamscale.coverage + com.teamscale.`spawned-jvm-coverage` +} + +dependencies { + spawnedJvmCode(project(":teamscale-gradle-plugin")) + spawnedJvmCode(project(":report-generator")) + spawnedJvmCode(project(":teamscale-client")) } tasks.test { diff --git a/system-tests/maven-external-upload-test/build.gradle.kts b/system-tests/maven-external-upload-test/build.gradle.kts index 508a88b91..1b16d0889 100644 --- a/system-tests/maven-external-upload-test/build.gradle.kts +++ b/system-tests/maven-external-upload-test/build.gradle.kts @@ -1,6 +1,12 @@ plugins { com.teamscale.`kotlin-convention` com.teamscale.`system-test-convention` + com.teamscale.`spawned-jvm-coverage` +} + +dependencies { + spawnedJvmCode(project(":teamscale-maven-plugin")) + spawnedJvmCode(project(":teamscale-client")) } tasks.test { diff --git a/system-tests/tia-maven/build.gradle.kts b/system-tests/tia-maven/build.gradle.kts index 95042d297..be75224bf 100644 --- a/system-tests/tia-maven/build.gradle.kts +++ b/system-tests/tia-maven/build.gradle.kts @@ -1,11 +1,16 @@ plugins { com.teamscale.`kotlin-convention` com.teamscale.`system-test-convention` - com.teamscale.coverage + com.teamscale.`spawned-jvm-coverage` +} + +dependencies { + spawnedJvmCode(project(":teamscale-maven-plugin")) + spawnedJvmCode(project(":report-generator")) + spawnedJvmCode(project(":teamscale-client")) } tasks.test { // install dependencies needed by the Maven test project dependsOn(":publishToMavenLocal") } - diff --git a/teamscale-gradle-plugin/build.gradle.kts b/teamscale-gradle-plugin/build.gradle.kts index 285b8e9e6..00981093d 100644 --- a/teamscale-gradle-plugin/build.gradle.kts +++ b/teamscale-gradle-plugin/build.gradle.kts @@ -6,7 +6,7 @@ plugins { `java-gradle-plugin` `kotlin-dsl` com.teamscale.`kotlin-convention` - com.teamscale.coverage + com.teamscale.`spawned-jvm-coverage` com.teamscale.publish alias(libs.plugins.pluginPublish) } diff --git a/teamscale-gradle-plugin/src/test/kotlin/com/teamscale/TeamscalePluginTestBase.kt b/teamscale-gradle-plugin/src/test/kotlin/com/teamscale/TeamscalePluginTestBase.kt index 67f179d2c..41e9a7e12 100644 --- a/teamscale-gradle-plugin/src/test/kotlin/com/teamscale/TeamscalePluginTestBase.kt +++ b/teamscale-gradle-plugin/src/test/kotlin/com/teamscale/TeamscalePluginTestBase.kt @@ -17,6 +17,20 @@ import java.lang.management.ManagementFactory */ abstract class TeamscalePluginTestBase { + companion object { + /** + * The `-javaagent` option that records the coverage of this plugin, supplied by the + * com.teamscale.spawned-jvm-coverage convention plugin. Absent unless that plugin is applied. + * + * TestKit runs every build below in a daemon of its own, so without this the plugin would be entirely + * uncovered even though these tests exercise all of it. `GRADLE_OPTS` would only reach the launcher, + * which is why the daemon is asked for the agent through `org.gradle.jvmargs`. The option is dropped + * when it contains whitespace, which would break the command line, cf. ProcessUtils. + */ + private val coverageAgent: String? = + System.getProperty("systemTestCoverageAgent")?.takeIf { it.none(Char::isWhitespace) } + } + /** Teamscale mock server to be used during the tests. */ protected lateinit var teamscaleMockServer: TeamscaleMockServer @@ -56,6 +70,7 @@ abstract class TeamscalePluginTestBase { val runner = GradleRunner.create() runner.forwardOutput() runnerArgs.add("--stacktrace") + coverageAgent?.let { runnerArgs.add("-Dorg.gradle.jvmargs=$it") } if (ManagementFactory.getRuntimeMXBean().inputArguments.toString() .contains("-agentlib:jdwp")