Consider targets used for test discovery in dependency checking

Before, we did test discovery at runtime, but we now do it at
compile-time, for speed and to avoid having to serialize the analysis
information returned by the compiler bridge.

This commits marks targets used during test discovery as used, thereby
subjecting them to depednency checking. In most cases, this won't make a
difference as the target containing the implementation of the
SBT test interface is the same one that the target being built would've
depended on anyway.

One important thing to note is that this will break a property of our
dependency checker that's been thus-far guaranteed: that it only allows
a dependency to be in `deps` or not in `deps`. When we do
test discovery, iterate through a list of test "frameworks". If we find
a test framework in the classpath, we look for tests that use that
framework; otherwise, we don't. This means that, for example, if specs2
isn't on the classpath, but the target contains specs2 tests, the tests
will silently fail to execute.

Author: jadenPete-lucid

src/main/scala/higherkindness/rules_scala/common/sbt-testing/Test.scala

@@ -1,5 +1,6 @@
 package higherkindness.rules_scala.common.sbt_testing
 
+import java.nio.file.{Path, Paths}
 import play.api.libs.json.{Format, Json}
 import sbt.testing.{Event, Framework, Logger, Runner, Status, Task, TaskDef, TestWildcardSelector}
 import scala.collection.mutable
@@ -12,10 +13,27 @@ object TestDefinition {
 }
 
 class TestFrameworkLoader(loader: ClassLoader) {
+  private val loadedJars = mutable.ArrayBuffer[Path]()
+  private def getClassJar(`class`: Class[?]): Option[Path] = for {
+    codeSource <- Option(`class`.getProtectionDomain().getCodeSource())
+    codeSourceUri = codeSource.getLocation().toURI()
+    path <-
+      try {
+        Some(Paths.get(codeSourceUri))
+      } catch {
+        case _: IllegalArgumentException => None
+      }
+  } yield path
+
+  def getLoadedJars(): Array[Path] = loadedJars.toArray
   def load(className: String) = {
     val framework =
       try {
-        Some(Class.forName(className, true, loader).getDeclaredConstructor().newInstance())
+        val `class` = Class.forName(className, true, loader)
+
+        getClassJar(`class`).foreach(loadedJars += _)
+
+        Some(`class`.getDeclaredConstructor().newInstance())
       } catch {
         case _: ClassNotFoundException => None
         case NonFatal(e)               => throw new Exception(s"Failed to load framework $className", e)

src/main/scala/higherkindness/rules_scala/workers/zinc/compile/ZincRunner.scala

@@ -195,19 +195,25 @@ object ZincRunner extends WorkerMain[Unit] {
     }
   }
 
-  private def maybeWriteTestsFile(parsedArguments: CommonArguments, analysis: AnnexAnalysis): Unit =
-    parsedArguments.testsFile.foreach { path =>
-      val classloader = ClassLoaders.sbtTestClassLoader(parsedArguments.classpath.map(_.toUri.toURL))
-      val frameworkLoader = new TestFrameworkLoader(classloader)
-      val testsFileData = TestsFileData(
-        parsedArguments.testFrameworks
-          .flatMap(frameworkName => frameworkLoader.load(frameworkName).map((frameworkName, _)))
-          .map { case (frameworkName, framework) => frameworkName -> new TestDiscovery(framework)(analysis.apis.toSet) }
-          .toMap,
-      )
-
-      Files.write(path, Json.stringify(Json.toJson(testsFileData)).getBytes(StandardCharsets.UTF_8))
-    }
+  private def maybeWriteTestsFile(parsedArguments: CommonArguments, analysis: AnnexAnalysis): Array[Path] =
+    parsedArguments.testsFile
+      .map { path =>
+        val classloader = ClassLoaders.sbtTestClassLoader(parsedArguments.classpath.map(_.toUri.toURL))
+        val frameworkLoader = new TestFrameworkLoader(classloader)
+        val testsFileData = TestsFileData(
+          parsedArguments.testFrameworks
+            .flatMap(frameworkName => frameworkLoader.load(frameworkName).map((frameworkName, _)))
+            .map { case (frameworkName, framework) =>
+              frameworkName -> new TestDiscovery(framework)(analysis.apis.toSet)
+            }
+            .toMap,
+        )
+
+        Files.write(path, Json.stringify(Json.toJson(testsFileData)).getBytes(StandardCharsets.UTF_8))
+
+        frameworkLoader.getLoadedJars()
+      }
+      .getOrElse(Array.empty)
 
   protected def init(args: Option[Array[String]]): Unit = ()
 
@@ -282,7 +288,7 @@ object ZincRunner extends WorkerMain[Unit] {
 
     InterruptUtil.throwIfInterrupted(task.isCancelled)
 
-    maybeWriteTestsFile(workRequest, analysis)
+    val usedDepsDuringTestDiscovery = maybeWriteTestsFile(workRequest, analysis)
 
     InterruptUtil.throwIfInterrupted(task.isCancelled)
 
@@ -294,7 +300,7 @@ object ZincRunner extends WorkerMain[Unit] {
     val usedDeps = workRequest.classpath.view
       .map(_.normalize().toAbsolutePath)
       .toSet
-      .intersect(analysis.usedJars.toSet)
+      .intersect((analysis.usedJars.view ++ usedDepsDuringTestDiscovery.view).toSet)
       // Filter out the Scala standard library as they should always be implicitly available we shouldn't be
       // bookkeeping them
       .view

tests/test-frameworks/mixed/BUILD

@@ -3,17 +3,12 @@ load("@rules_scala_annex//rules:scala.bzl", "scala_test")
 scala_test(
     name = "mixed_2_13",
     srcs = glob(["*.scala"]),
-    deps_used_whitelist = [
-        "@annex_test//:org_hamcrest_hamcrest_core",
-        "@annex_test//:com_novocode_junit_interface",
-    ],
     scala_toolchain_name = "test_zinc_2_13",
     shard_count = 2,
     tags = ["manual"],
     deps = [
         "@annex_test//:com_novocode_junit_interface",
         "@annex_test//:junit_junit",
-        "@annex_test//:org_hamcrest_hamcrest_core",
         "@annex_test//:org_scalacheck_scalacheck_2_13",
         "@annex_test//:org_scalactic_scalactic_2_13",
         "@annex_test//:org_scalatest_scalatest_compatible",
@@ -31,11 +26,8 @@ scala_test(
     scala_toolchain_name = "test_zinc_2_12",
     shard_count = 2,
     tags = ["manual"],
-    runtime_deps = [
-        "@annex_test//:com_novocode_junit_interface",
-        "@annex_test//:org_hamcrest_hamcrest_core",
-    ],
     deps = [
+        "@annex_test//:com_novocode_junit_interface",
         "@annex_test//:junit_junit",
         "@annex_test_2_12//:org_scalacheck_scalacheck_2_12",
         "@annex_test_2_12//:org_scalactic_scalactic_2_12",