Code review

checkers/pmd/src/testFixtures/kotlin/net/twisterrob/gradle/pmd/test/PmdTestResources.kt

@@ -14,6 +14,9 @@ import org.gradle.util.GradleVersion
  * ```
  */
 class PmdTestResources(
+	/**
+	 * Reasons for conditionals, see embedded PMD versions in [net.twisterrob.gradle.quality.tasks.VersionsTaskTest].
+	 */
 	private val gradleVersion: () -> GradleVersion
 ) {
 
@@ -59,9 +62,6 @@ class PmdTestResources(
 		val message2: Regex
 			get() =
 				when {
-					gradleVersion() < GradleVersion.version("5.6.0") -> {
-						Regex(""".*src.main.java.pmd.PrintStack\.java:8:\tAvoid printStackTrace\(\); use a logger call instead.""")
-					}
 					gradleVersion() < GradleVersion.version("7.0.0") -> {
 						Regex(""".*src.main.java.pmd.PrintStack\.java:8:\tAvoid printStackTrace\(\); use a logger call instead.""")
 					}

quality/build.gradle.kts

@@ -29,18 +29,7 @@ dependencies {
 	implementation(projects.compat.agp)
 
 	compileOnly(libs.annotations.jetbrains)
-	compileOnly(libs.android.gradle) {
-		configurations.compileOnly {
-			// +--- com.android.tools.build:gradle:7.2.1
-			//|    +--- com.android.tools:sdk-common:30.2.1
-			//|    |    \--- xerces:xercesImpl:2.12.0
-			//|    |         \--- xml-apis:xml-apis:1.4.01
-			// Without this exclude, it brings in an incompatible version of javax.xml.stream.XMLOutputFactory
-			// which doesn't have a "newFactory(String, ClassLoader)" method.
-			// See net.twisterrob.gradle.quality.report.html.XMLStreamWriterDSLKt.bestXMLOutputFactory.
-			exclude("xml-apis", "xml-apis") // 1.4.01
-		}
-	}
+	compileOnly(libs.android.gradle)
 	// Need com.android.utils.FileUtils for HtmlReportTask.
 	compileOnly(libs.android.tools.common)
 //	compileOnly ("de.aaschmid:gradle-cpd-plugin:1.0")

quality/src/main/kotlin/net/twisterrob/gradle/quality/report/BestXMLSupport.kt

@@ -6,6 +6,7 @@ import javax.xml.transform.TransformerFactory
 import kotlin.reflect.full.declaredMembers
 import kotlin.reflect.full.staticFunctions
 import kotlin.reflect.jvm.isAccessible
+import kotlin.reflect.jvm.javaType
 
 /**
  * We're aiming to get the default Java writer:
@@ -14,7 +15,7 @@ import kotlin.reflect.jvm.isAccessible
  * but without hardcoding it.
  *
  * Strategy:
- *  * Call newDefaultFactory whenever available.
+ *  * Call [XMLOutputFactory.newDefaultFactory] whenever available.
  *  * If it doesn't exist, but the class has a declared default implementation, use that.
  *  * Fall back to hardcoding, which might fail anyway if the default is not there.
  *  * Give up, and use whatever is available.
@@ -28,14 +29,6 @@ internal fun bestXMLOutputFactory(): XMLOutputFactory {
 			.call() as XMLOutputFactory
 	}
 
-	fun String.isClassLoadable(): Boolean =
-		try {
-			Class.forName(this)
-			true
-		} catch (e: ClassNotFoundException) {
-			false
-		}
-
 	val defaultImpl =
 		XMLOutputFactory::class.declaredMembers
 			.singleOrNull { it.name == "DEFAULIMPL" }
@@ -44,12 +37,28 @@ internal fun bestXMLOutputFactory(): XMLOutputFactory {
 			?: "com.sun.xml.internal.stream.XMLOutputFactoryImpl"
 	if (defaultImpl.isClassLoadable()) {
 		// return XMLOutputFactory.newFactory(defaultImpl, null as ClassLoader?) is @since Java 6,
-		// BUT see build.gradle.kts too.
+		// BUT it cannot be compiled because of an old xml-apis:xml-apis dependency.
+		// +--- com.android.tools.build:gradle:7.2.1
+		// |    +--- com.android.tools:sdk-common:30.2.1
+		// |    |    \--- xerces:xercesImpl:2.12.0
+		// |    |         \--- xml-apis:xml-apis:1.4.01
+		// It has a very old incompatible version of javax.xml.stream.XMLOutputFactory
+		// which doesn't have a "newFactory(String, ClassLoader)" method.
+		// I tried to exclude with this but still didn't compile:
+		// configurations.compileOnly { exclude("xml-apis", "xml-apis") }
+		// So stuck with reflection.
 		val newFactory = XMLOutputFactory::class.staticFunctions
-			.firstOrNull { it.name == "newFactory" && it.parameters.size == 2 }
+			.firstOrNull {
+				it.name == "newFactory"
+						&& it.parameters.size == 2
+						&& it.parameters[0].type.javaType == String::class.java
+						&& it.parameters[1].type.javaType == ClassLoader::class.java
+			}
 		if (newFactory != null) {
 			/**
-			 * This is very silly, instead of providing the class name directly, we have to provide a System property.
+			 * This is a very silly (documented) API,
+			 * instead of providing the class name directly (like in other APIs in the same package),
+			 * we have to provide a System property.
 			 */
 			val factoryId = ::bestXMLOutputFactory.name + ".temporary.xml.output.factory"
 			System.setProperty(factoryId, defaultImpl)
@@ -97,6 +106,7 @@ private fun XMLOutputFactory.safeSetProperty(name: String, value: Any?) {
 		this.setProperty(name, value)
 	}
 }
+
 /**
  * We're aiming to get the default Java writer:
  * [com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl]
@@ -113,22 +123,22 @@ internal fun bestXMLTransformerFactory(): TransformerFactory {
 	if (ManagementFactory.getRuntimeMXBean().specVersion != "1.8") {
 		// return TransformerFactory.newDefaultInstance() // which is @since Java 9.
 		return TransformerFactory::class.staticFunctions
-			.first { it.name == "newDefaultInstance" && it.parameters.isEmpty() }
+			.single { it.name == "newDefaultInstance" && it.parameters.isEmpty() }
 			.call() as TransformerFactory
 	}
 
-	fun String.isClassLoadable(): Boolean =
-		try {
-			Class.forName(this)
-			true
-		} catch (e: ClassNotFoundException) {
-			false
-		}
-
 	val defaultImpl = "com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl"
 	if (defaultImpl.isClassLoadable()) {
 		return TransformerFactory.newInstance(defaultImpl, null as ClassLoader?)
 	}
 	// Useful link: https://stackoverflow.com/questions/11314604/how-to-set-saxon-as-the-xslt-processor-in-java
 	return TransformerFactory.newInstance()
 }
+
+private fun String.isClassLoadable(): Boolean =
+	try {
+		Class.forName(this)
+		/*@return*/ true
+	} catch (e: ClassNotFoundException) {
+		/*@return*/ false
+	}

quality/src/main/kotlin/net/twisterrob/gradle/quality/report/html/XMLStreamWriterDSL.kt

@@ -8,9 +8,7 @@ import javax.xml.stream.XMLStreamWriter
 
 fun Writer.xmlWriter(): XMLStreamWriter =
 	bestXMLOutputFactory()
-		.also { println("XMLAPI::xmlWriter.factory = ${it}") }
 		.createXMLStreamWriter(this)
-		.also { println("XMLAPI::xmlWriter() = ${it}") }
 
 fun XMLStreamWriter.use(block: (XMLStreamWriter) -> Unit) {
 	AutoCloseable {

quality/src/main/kotlin/net/twisterrob/gradle/quality/tasks/HtmlReportTask.kt

@@ -93,9 +93,7 @@ open class HtmlReportTask : BaseViolationsTask() {
 			// We're expecting to get [com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl]
 			// Creating [com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl].
 			bestXMLTransformerFactory()
-				.also { println("XMLAPI::transform.factory: ${it}") }
 				.newTransformer(StreamSource(xslOutputFile.reader()))
-				.also { println("XMLAPI::transform.transformer: ${it}") }
 				.transform(StreamSource(xmlFile.reader()), StreamResult(htmlFile))
 			println("Wrote HTML report to ${SdkUtils.fileToUrlString(htmlFile.absoluteFile)}")
 		} catch (ex: Throwable) {

quality/src/test/kotlin/net/twisterrob/gradle/quality/report/html/ViolationsRendererTest.kt

@@ -1,23 +1,18 @@
 package net.twisterrob.gradle.quality.report.html
 
 import org.intellij.lang.annotations.Language
-import org.junit.jupiter.api.Assertions.assertTrue
 import org.junit.jupiter.api.Test
 import java.io.StringWriter
 import kotlin.test.assertEquals
 
 class ViolationsRendererTest {
 
 	@Test fun `xmlWriter produces a supported writer for test`() {
-		val expected = setOf(
-			"com.sun.xml.internal.stream.writers.XMLStreamWriterImpl",
-			"com.ctc.wstx.sw.SimpleNsStreamWriter"
-		)
-
 		val writer = StringWriter().xmlWriter()
+
 		val actual = writer::class.qualifiedName
 
-		assertTrue(actual in expected) { "Expected one of ${expected}, but got ${actual}." }
+		assertEquals("com.sun.xml.internal.stream.writers.XMLStreamWriterImpl", actual)
 	}
 
 	@Test fun `renderXml writes preamble`() {

quality/src/test/kotlin/net/twisterrob/gradle/quality/tasks/HtmlReportTaskTest.kt

@@ -8,6 +8,7 @@ import net.twisterrob.gradle.test.GradleRunnerRuleExtension
 import net.twisterrob.gradle.test.projectFile
 import net.twisterrob.gradle.test.root
 import net.twisterrob.gradle.test.runBuild
+import net.twisterrob.test.testName
 import org.gradle.testkit.runner.TaskOutcome
 import org.hamcrest.MatcherAssert.assertThat
 import org.hamcrest.Matchers.greaterThan
@@ -380,17 +381,17 @@ class HtmlReportTaskTest : BaseIntgTest() {
 		)
 	}
 
-	private fun exposeViolationsInReport(test: TestInfo,everything: ViolationTestResources.Everything) {
-		val baseDir  = File("build/reports/tests/test/outputs").resolve(test.testName)
+	private fun exposeViolationsInReport(test: TestInfo, resources: ViolationTestResources.Everything) {
+		val baseDir = File("build/reports/tests/test/outputs").resolve(test.testName)
 		listOf(
 			gradle.violationsReport("xsl"),
 			gradle.violationsReport("xml"),
 			gradle.violationsReport("html"),
 		).forEach { file ->
 			file.copyTo(baseDir.resolve(file.name), overwrite = true)
 		}
-		baseDir.resolve("violations-expected.xml").writeText(everything.violationsXml)
-		baseDir.resolve("violations-expected.html").writeText(everything.violationsHtml)
+		baseDir.resolve("violations-expected.xml").writeText(resources.violationsXml)
+		baseDir.resolve("violations-expected.html").writeText(resources.violationsHtml)
 	}
 
 	companion object {
@@ -407,6 +408,3 @@ class HtmlReportTaskTest : BaseIntgTest() {
 			""".trimIndent()
 	}
 }
-
-val TestInfo.testName: String
-	get() = testClass.map { it.name }.orElse("") + "." + testMethod.map { it.name }.orElse("")

quality/src/testFixtures/kotlin/net/twisterrob/gradle/quality/tasks/ViolationTestResources.kt

@@ -20,6 +20,7 @@ class ViolationTestResources(
 	 *  1. Replace the project name (junit...) in this file with the new name.
 	 *  1. Run the test again to validate.
 	 *  1. Amend the commit to include changes in this file.
+	 *  1. Run the test with Java 8 and Java 11 set in `Settings | Build, Execution, Deployment | Build Tools | Gradle`.
 	 */
 	inner class Everything {
 		val checkstyleReport: String
@@ -138,6 +139,8 @@ class ViolationTestResources(
 				}
 				// The XSL transformation will produce system-specific separators
 				// (on CI/Unix this is different from the captured Windows line endings).
+				// Note: it is convenient that the capture was done on Windows, because it gives an easy target for replacement.
+				// If it was captured on Unix, then the replacement would be more complex.
 				.replace("\r\n", System.lineSeparator())
 	}
 

test/internal/src/main/kotlin/net/twisterrob/test/testName.kt

@@ -0,0 +1,6 @@
+package net.twisterrob.test
+
+import org.junit.jupiter.api.TestInfo
+
+val TestInfo.testName: String
+	get() = testClass.map { it.name }.orElse("") + "." + testMethod.map { it.name }.orElse("")