Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ We use [semantic versioning](http://semver.org/):
- PATCH version when you make backwards compatible bug fixes.

# Next version
- [fix] _teamscale-gradle-plugin_, _teamscale-maven-plugin_: Testwise coverage reports no longer drop a class directory that contains an archive (JAR, WAR, EAR, ZIP) that contains a JAR.

# 39.0.0
- [breaking] _agent_: The deprecated Docker image `cqse/teamscale-jacoco-agent` is no longer published. Use `cqse/teamscale-java-profiler` instead, which has replaced it since version 36.5.0.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ import org.jacoco.core.internal.analysis.StringPool
import org.jacoco.core.internal.data.CRC64
import org.jacoco.core.internal.flow.ClassProbesAdapter
import org.jacoco.core.internal.instr.InstrSupport
import java.io.File
import java.io.IOException
import java.io.InputStream
import java.nio.file.Files
import java.nio.file.Paths

/**
* An [AnalyzerCache] instance processes a set of Java class/jar/war/... files and builds a cache for each of the classes.
Expand Down Expand Up @@ -55,10 +55,38 @@ class AnalyzerCache(

/**
* Adds caching for jar files to the analyze jar functionality.
*
* The jar ID of a jar on disk is computed from the file, so the jar is only held in memory while the ID is
* computed. The jar itself is then streamed from [input] while it is analyzed.
*/
@Throws(IOException::class)
override fun analyzeJar(input: InputStream, location: String): Int {
val jarId = CRC64.classId(Files.readAllBytes(Paths.get(location)))
val file = File(location)
// Checks the file system instead of looking for '@' in the location, because real paths may contain '@' as
// well (e.g. the Jenkins workspace `workspace@2`). Such jars must still be streamed instead of buffered.
if (!file.isFile) {
return analyzeJarInsideArchive(input, location)
}
return analyzeJarWithCache(CRC64.classId(Files.readAllBytes(file.toPath())), input, location)
}

/**
* Analyzes a jar that is an entry of another archive (JAR, WAR, EAR, ZIP, ...). Such a jar has a location like
* `app.war@WEB-INF/lib/inner.jar`, which is not a file on disk. Its bytes are only available from [input], so
* they are buffered to compute the jar ID and to analyze the jar.
*/
@Throws(IOException::class)
private fun analyzeJarInsideArchive(input: InputStream, location: String): Int {
val bytes = try {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the special case to be documented inline. Maybe have a separate method for that case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude: Agreed. Right now the reason is only in the method's KDoc (lines 59–61). Someone reading lines 65–74 sees file.isFile with no hint why a .jar location might not be a file. A separate method would let its name and doc say it, for example:

override fun analyzeJar(input: InputStream, location: String): Int {
	val file = File(location)
	if (!file.isFile) {
		return analyzeJarInsideArchive(input, location)
	}
	return analyzeJarWithCache(CRC64.classId(Files.readAllBytes(file.toPath())), input, location)
}

/** A jar inside an archive has a location like `app.war@WEB-INF/lib/inner.jar`, which is not a file on disk. ... */
private fun analyzeJarInsideArchive(input: InputStream, location: String): Int

Two things the author should keep:

  • Not only jars in jars. The case also covers a jar inside a WAR, an EAR or a ZIP, so a name with "nested jar" in it is a bit narrow (see the proposed thread on CHANGELOG.md).
  • Keep isFile as the check. It may be tempting to switch to '@' in location, but @ shows up in real paths. Jenkins, for example, names the workspaces of concurrent builds workspace@2. With that check, every jar on disk in such a workspace would take the in-memory path and stay buffered in full while it is analyzed, instead of being streamed. That is the extra memory use that commit 7bb9cc5 removed. A comment on the check could say this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude: Done. The reply above is now the single merged reply, with the corrected point about the @ check. I removed my correction, your request and my note about the blocked edit. Say so if this comment and yours should go too.

input.readBytes()
} catch (e: IOException) {
throw analyzerError(location, e)
}
return analyzeJarWithCache(CRC64.classId(bytes), bytes.inputStream(), location)
}

/** Analyzes the given jar unless a jar with the same [jarId] has been analyzed before. */
private fun analyzeJarWithCache(jarId: Long, input: InputStream, location: String): Int {
val probesCountForJarId = probesCache.countForJarId(jarId)
if (probesCountForJarId != 0) {
return probesCountForJarId
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package com.teamscale.report.testwise.jacoco.cache

import com.teamscale.report.EDuplicateClassFileBehavior
import com.teamscale.report.util.ClasspathWildcardIncludeFilter
import com.teamscale.report.util.ILogger
import org.assertj.core.api.Assertions.assertThat
import org.assertj.core.api.Assertions.assertThatThrownBy
import org.jacoco.core.internal.data.CRC64
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.io.TempDir
import org.junit.jupiter.params.ParameterizedTest
import org.junit.jupiter.params.provider.CsvSource
import org.mockito.Mockito
import java.io.ByteArrayOutputStream
import java.io.File
import java.io.IOException
import java.util.zip.ZipEntry
import java.util.zip.ZipOutputStream

/** Tests for the [AnalyzerCache] class. */
class AnalyzerCacheTest {

@TempDir
private lateinit var tempDir: File

private val classBytes = javaClass.getResourceAsStream("AnalyzerCacheTest.class")!!.use { it.readBytes() }
private val innerJar = zip("com/teamscale/report/testwise/jacoco/cache/AnalyzerCacheTest.class" to classBytes)

private val logger = Mockito.mock<ILogger>()
private val probesCache = ProbesCache(logger, EDuplicateClassFileBehavior.IGNORE)
private val analyzer = AnalyzerCache(probesCache, ClasspathWildcardIncludeFilter(null, null), logger)

/**
* An archive may contain JARs, e.g. a WAR contains its libraries and some JARs contain other JARs (e.g. SVNKit).
* The analyzer addresses such a JAR as `app.war@WEB-INF/lib/inner.jar`, which is not a file on disk, so the cache
* must not try to read it from there.
*/
@ParameterizedTest
@CsvSource("outer.jar, lib/inner.jar", "app.war, WEB-INF/lib/inner.jar")
fun analyzesClassesInJarsInsideArchives(archiveName: String, jarPath: String) {
tempDir.resolve(archiveName).writeBytes(zip(jarPath to innerJar))

assertThat(analyzer.analyzeAll(tempDir)).isEqualTo(1)
assertThat(probesCache.containsClassId(CRC64.classId(classBytes))).isTrue()
}

/** A nested JAR that cannot be read must be reported with its location, so that users can find it. */
@Test
fun reportsLocationOfUnreadableNestedJar() {
val outerJar = zip("lib/inner.jar" to innerJar)
// Cuts the outer JAR in the middle of the nested JAR's data
tempDir.resolve("outer.jar").writeBytes(outerJar.copyOf(outerJar.size / 2))

assertThatThrownBy { analyzer.analyzeAll(tempDir) }
.isInstanceOf(IOException::class.java)
.hasMessageContaining("outer.jar@lib/inner.jar")
}

private fun zip(vararg entries: Pair<String, ByteArray>): ByteArray {
val output = ByteArrayOutputStream()
ZipOutputStream(output).use { zip ->
entries.forEach { (name, content) ->
zip.putNextEntry(ZipEntry(name))
zip.write(content)
zip.closeEntry()
}
}
return output.toByteArray()
}
}
Loading