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, we 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.

This means that now, having that a dependency in `deps` or not in `deps`
are sometimes both valid states to be in. Prior to this change, that
wasn't the case; there was a single, correct list of dependencies.

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,17 +13,39 @@ object TestDefinition {
 }
 
 class TestFrameworkLoader(loader: ClassLoader) {
+  private val loadedJarsByFramework = mutable.Map.empty[String, 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 getLoadedJarsByFramework(): Map[String, Array[Path]] =
+    loadedJarsByFramework.view.map { case (frameworkName, loadedJars) => frameworkName -> loadedJars.toArray }.toMap
+
   def load(className: String) = {
-    val framework =
+    val (framework, loadedJar) =
       try {
-        Some(Class.forName(className, true, loader).getDeclaredConstructor().newInstance())
+        val `class` = Class.forName(className, true, loader)
+        val loadedJar = getClassJar(`class`)
+
+        (Some(`class`.getDeclaredConstructor().newInstance()), loadedJar)
       } catch {
-        case _: ClassNotFoundException => None
+        case _: ClassNotFoundException => (None, None)
         case NonFatal(e)               => throw new Exception(s"Failed to load framework $className", e)
       }
     framework.map {
-      case framework: Framework => framework
-      case _                    => throw new Exception(s"$className does not implement ${classOf[Framework].getName}")
+      case framework: Framework =>
+        loadedJar.foreach(loadedJarsByFramework.getOrElseUpdate(className, mutable.ArrayBuffer.empty) += _)
+
+        framework
+
+      case _ => throw new Exception(s"$className does not implement ${classOf[Framework].getName}")
     }
 
   }

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

@@ -211,19 +211,33 @@ 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,
-      )
+  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))
-    }
+        val loadedJarsByFramework = frameworkLoader.getLoadedJarsByFramework()
+        val loadedJars = testsFileData.testsByFramework.view
+          .filter { case (_, tests) => tests.nonEmpty }
+          .keys
+          .flatMap(loadedJarsByFramework.get)
+          .flatten
+          .toArray
+
+        Files.write(path, Json.stringify(Json.toJson(testsFileData)).getBytes(StandardCharsets.UTF_8))
+
+        loadedJars
+      }
+      .getOrElse(Array.empty)
 
   protected def init(args: Option[Array[String]]): Unit = ()
 
@@ -298,7 +312,16 @@ object ZincRunner extends WorkerMain[Unit] {
 
     InterruptUtil.throwIfInterrupted(task.isCancelled)
 
-    maybeWriteTestsFile(workRequest, analysis)
+    /*
+     * JARs on the classpath used during test discovery, but not used by compiler won't show up in `analysis.usedJars`, so
+     * we need to consider them in addition to those used by the compiler.
+     *
+     * Test discovery works by iterating through the list of test frameworks provided via `workRequest.testFrameworks`,
+     * attempting to classload each framework, and then using that framework to discover tests. If a given framework
+     * successfully loads and we found a test with it we take note of the JAR it was loaded from and consider that JAR
+     * (and therefore the target to which it belongs) used.
+     */
+    val usedDepsDuringTestDiscovery = maybeWriteTestsFile(workRequest, analysis)
 
     InterruptUtil.throwIfInterrupted(task.isCancelled)
 
@@ -310,7 +333,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",