Restructure TestModule, add RunModule (#3064)

This idea of this change is to restructure the `TestModule` to decouple
it from the concept of compilation. After this change, only a
`runClasspath` and a `testClasspath` is needed to run tests. This makes
adding test modules for the sake of using a different test framework a
breeze.

See the following example: the `test2` module is an additional test
framework on the same classes of `test`.

```scala
import mill._
import mill.scalalib._
import mill.scalalib.api.CompilationResult

object foo extends RootModule with ScalaModule {
  def scalaVersion = "2.13.11"
  def ivyDeps = Agg(
    ivy"com.lihaoyi::scalatags:0.12.0",
    ivy"com.lihaoyi::mainargs:0.6.2"
  )

  object test extends ScalaTests {
    def ivyDeps = Agg(
      ivy"com.lihaoyi::utest:0.7.11",
      ivy"org.scalatest::scalatest-freespec:3.2.18"
    )
    def testFramework = "utest.runner.Framework"
  }
  object test2 extends TestModule with TestModule.ScalaTest {
    override def compile: T[CompilationResult] = ???
    override def runClasspath: T[Seq[PathRef]] = foo.test.runClasspath()
    override def testClasspath = foo.test.testClasspath()
  }
}
```

Please note the `compile` target is a legacy to our binary-compatibility
promise. The target is not used directly in `TestModule`.

This pull request additionally contains the following changes:

* Introduces a new `RunModule` and moved some `run`-related task
previously in `TestModule` up.
* Extend `RunModule` in `JavaModule` to share run-releated targets and
resolve super-hierarchy
* Introduces a `WithZincWorker` as a shared base trait to resolve
super-hierarchies for using and overriding a common worker.

I plan to move more run-releated target from `JavaModule` to `RunModule`
in a subsequent PR. (See #3090)

See also the following discussion:
* #3076

Pull request: #3064

Author: lefou

bsp/worker/src/mill/bsp/worker/MillScalaBuildServer.scala

@@ -112,12 +112,12 @@ private trait MillScalaBuildServer extends ScalaBuildServer { this: MillBuildSer
       targetIds = _ => p.getTargets.asScala.toSeq,
       tasks = {
         case m: TestModule =>
-          T.task(Some((m.runClasspath(), m.testFramework(), m.compile())))
+          T.task(Some((m.runClasspath(), m.testFramework(), m.testClasspath())))
         case _ =>
           T.task(None)
       }
     ) {
-      case (ev, state, id, m: TestModule, Some((classpath, testFramework, compResult))) =>
+      case (ev, state, id, m: TestModule, Some((classpath, testFramework, testClasspath))) =>
         val (frameworkName, classFingerprint): (String, Agg[(Class[_], Fingerprint)]) =
           Jvm.inprocess(
             classpath.map(_.path),
@@ -129,7 +129,7 @@ private trait MillScalaBuildServer extends ScalaBuildServer { this: MillBuildSer
               val discoveredTests = TestRunnerUtils.discoverTests(
                 cl,
                 framework,
-                Agg(compResult.classes.path)
+                Agg.from(testClasspath.map(_.path))
               )
               (framework.name(), discoveredTests)
             }

build.sc

@@ -428,28 +428,48 @@ trait MillPublishScalaModule extends MillScalaModule with MillPublishJavaModule
 trait MillStableScalaModule extends MillPublishScalaModule with Mima {
   import com.github.lolgab.mill.mima._
   override def mimaBinaryIssueFilters: T[Seq[ProblemFilter]] = Seq(
-    // MIMA doesn't properly ignore things which are nested inside other private things
+    // (5x) MIMA doesn't properly ignore things which are nested inside other private things
     // so we have to put explicit ignores here (https://github.com/lightbend/mima/issues/771)
     ProblemFilter.exclude[Problem]("mill.eval.ProfileLogger*"),
     ProblemFilter.exclude[Problem]("mill.eval.GroupEvaluator*"),
     ProblemFilter.exclude[Problem]("mill.eval.Tarjans*"),
     ProblemFilter.exclude[Problem]("mill.define.Ctx#Impl*"),
     ProblemFilter.exclude[Problem]("mill.resolve.ResolveNotFoundHandler*"),
-    // See https://github.com/com-lihaoyi/mill/pull/2739
+    // (4x) See https://github.com/com-lihaoyi/mill/pull/2739
     ProblemFilter.exclude[ReversedMissingMethodProblem](
       "mill.scalajslib.ScalaJSModule.mill$scalajslib$ScalaJSModule$$super$scalaLibraryIvyDeps"
     ),
-    // See https://github.com/com-lihaoyi/mill/pull/3072
     ProblemFilter.exclude[ReversedMissingMethodProblem](
       "mill.scalalib.ScalaModule.mill$scalalib$ScalaModule$$super$zincAuxiliaryClassFileExtensions"
     ),
-    // See https://github.com/com-lihaoyi/mill/pull/3072
     ProblemFilter.exclude[ReversedMissingMethodProblem](
       "mill.scalajslib.ScalaJSModule.mill$scalajslib$ScalaJSModule$$super$zincAuxiliaryClassFileExtensions"
     ),
-    // See https://github.com/com-lihaoyi/mill/pull/3072
     ProblemFilter.exclude[ReversedMissingMethodProblem](
       "mill.scalanativelib.ScalaNativeModule.mill$scalanativelib$ScalaNativeModule$$super$zincAuxiliaryClassFileExtensions"
+    ),
+    // (7x) See https://github.com/com-lihaoyi/mill/pull/3064
+    // Moved targets up in trait hierarchy, but also call them via super, which I think is safe
+    ProblemFilter.exclude[ReversedMissingMethodProblem](
+      "mill.scalalib.JavaModule.mill$scalalib$JavaModule$$super$zincWorker"
+    ),
+    ProblemFilter.exclude[ReversedMissingMethodProblem](
+      "mill.scalalib.JavaModule.mill$scalalib$JavaModule$$super$runClasspath"
+    ),
+    ProblemFilter.exclude[ReversedMissingMethodProblem](
+      "mill.scalalib.JavaModule.mill$scalalib$JavaModule$$super$runUseArgsFile"
+    ),
+    ProblemFilter.exclude[ReversedMissingMethodProblem](
+      "mill.scalalib.JavaModule#JavaModuleTests.mill$scalalib$JavaModule$JavaModuleTests$$super$testClasspath"
+    ),
+    ProblemFilter.exclude[ReversedMissingMethodProblem](
+      "mill.scalalib.JavaModule.mill$scalalib$JavaModule$$super$forkArgs"
+    ),
+    ProblemFilter.exclude[ReversedMissingMethodProblem](
+      "mill.scalalib.JavaModule.mill$scalalib$JavaModule$$super$forkEnv"
+    ),
+    ProblemFilter.exclude[ReversedMissingMethodProblem](
+      "mill.scalalib.JavaModule.mill$scalalib$JavaModule$$super$forkWorkingDir"
     )
   )
   def mimaPreviousVersions: T[Seq[String]] = Settings.mimaBaseVersions
@@ -975,7 +995,8 @@ object contrib extends Module {
 
 object scalanativelib extends MillStableScalaModule {
   def moduleDeps = Seq(scalalib, scalanativelib.`worker-api`)
-  def testTransitiveDeps = super.testTransitiveDeps() ++ Seq(worker("0.4").testDep(), worker("0.5").testDep())
+  def testTransitiveDeps =
+    super.testTransitiveDeps() ++ Seq(worker("0.4").testDep(), worker("0.5").testDep())
 
   object `worker-api` extends MillPublishScalaModule {
     def ivyDeps = Agg(Deps.sbtTestInterface)

example/basic/5-multiple-test-frameworks/build.sc

@@ -0,0 +1,51 @@
+import mill._
+import mill.scalalib._
+import mill.scalalib.api.CompilationResult
+
+object foo extends RootModule with ScalaModule {
+  def scalaVersion = "2.13.11"
+  def ivyDeps = Agg(
+    ivy"com.lihaoyi::scalatags:0.12.0",
+    ivy"com.lihaoyi::mainargs:0.6.2"
+  )
+
+  object test extends ScalaTests {
+    def ivyDeps = Agg(
+      ivy"com.lihaoyi::utest:0.7.11",
+      ivy"org.scalatest::scalatest-freespec:3.2.18"
+    )
+    def testFramework = "utest.runner.Framework"
+  }
+  object test2 extends TestModule with TestModule.ScalaTest {
+    override def compile: T[CompilationResult] = ???
+    override def runClasspath: T[Seq[PathRef]] = foo.test.runClasspath()
+    override def testClasspath = foo.test.testClasspath()
+  }
+}
+
+// format: off
+/** Usage
+
+> mill resolve __:TestModule.test
+...
+test.test
+test2.test
+
+> mill test
+...
++ foo.FooTests.simple ...  <h1>hello</h1>
++ foo.FooTests.escaping ...  <h1>&lt;hello&gt;</h1>
+Tests: 2, Passed: 2, Failed: 0
+> mill test2
+...
+FooScalaTests:
+Foo
+- simple
+- escaping
+...
+Total number of tests run: 2
+Suites: completed 1, aborted 0
+Tests: succeeded 2, failed 0, canceled 0, ignored 0, pending 0
+
+*/
+// format: on

example/basic/5-multiple-test-frameworks/src/Foo.scala

@@ -0,0 +1,16 @@
+package foo
+import scalatags.Text.all._
+import mainargs.{main, ParserForMethods}
+
+object Foo {
+  def generateHtml(text: String) = {
+    h1(text).toString
+  }
+
+  @main
+  def main(text: String) = {
+    println(generateHtml(text))
+  }
+
+  def main(args: Array[String]): Unit = ParserForMethods(this).runOrExit(args)
+}

example/basic/5-multiple-test-frameworks/test/src/FooScalaTests.scala

@@ -0,0 +1,18 @@
+package foo
+
+import org.scalatest.freespec._
+
+class FooScalaTests extends AnyFreeSpec {
+  "Foo" - {
+    "simple" in {
+      val result = Foo.generateHtml("hello")
+      assert(result == "<h1>hello</h1>")
+      result
+    }
+    "escaping" in {
+      val result = Foo.generateHtml("<hello>")
+      assert(result == "<h1>&lt;hello&gt;</h1>")
+      result
+    }
+  }
+}

example/basic/5-multiple-test-frameworks/test/src/FooTests.scala

@@ -0,0 +1,16 @@
+package foo
+import utest._
+object FooTests extends TestSuite {
+  def tests = Tests {
+    test("simple") {
+      val result = Foo.generateHtml("hello")
+      assert(result == "<h1>hello</h1>")
+      result
+    }
+    test("escaping") {
+      val result = Foo.generateHtml("<hello>")
+      assert(result == "<h1>&lt;hello&gt;</h1>")
+      result
+    }
+  }
+}

example/src/mill/integration/ExampleTestSuite.scala

@@ -3,6 +3,8 @@ import mill.integration.{BashTokenizer, IntegrationTestSuite}
 import utest._
 import mill.util.Util
 
+import scala.util.chaining.scalaUtilChainingOps
+
 /**
  * Shared implementation for the tests in `example/`.
  *
@@ -85,7 +87,15 @@ object ExampleTestSuite extends IntegrationTestSuite {
       expectedSnippets: Vector[String],
       commandStr: String
   ): Unit = {
-    BashTokenizer.tokenize(commandStr) match {
+    BashTokenizer.tokenize(commandStr)
+      .tap { cmd =>
+        Console.err.println(
+          s"""$workspaceRoot> ${cmd.mkString("'", "' '", "'")}
+             |--- Expected output --------
+             |${expectedSnippets.mkString("\n")}
+             |----------------------------""".stripMargin
+        )
+      } match {
       case Seq("cp", "-r", from, to) =>
         os.copy(os.Path(from, workspaceRoot), os.Path(to, workspaceRoot))
 

scalalib/src/mill/scalalib/JavaModule.scala

@@ -23,15 +23,17 @@ import os.{Path, ProcessOutput}
  */
 trait JavaModule
     extends mill.Module
+    with WithZincWorker
+    with TestModule.JavaModuleBase
     with TaskModule
+    with RunModule
     with GenIdeaModule
     with CoursierModule
     with OfflineSupportModule
     with BspModule
-    with SemanticDbJavaModule
-    with TestModule.JavaModuleBase { outer =>
+    with SemanticDbJavaModule { outer =>
 
-  def zincWorker: ModuleRef[ZincWorkerModule] = ModuleRef(mill.scalalib.ZincWorkerModule)
+  def zincWorker: ModuleRef[ZincWorkerModule] = super.zincWorker
 
   trait JavaModuleTests extends JavaModule with TestModule {
     // Run some consistence checks
@@ -51,6 +53,15 @@ trait JavaModule
       }
     }
 
+    /**
+     * The classpath containing the tests. This defaults to the compilation output.
+     */
+    def testClasspath: T[Seq[PathRef]] = T {
+      // bin-compat-shim: keep the super.call in the classfile
+      super.testClasspath
+      Seq(compile().classes)
+    }
+
     /**
      * JavaModule and its derivates define inner test modules.
      * To avoid unexpected misbehavior due to the use of the wrong inner test trait
@@ -155,11 +166,17 @@ trait JavaModule
 
   /**
    *  The direct dependencies of this module.
+   *  This is meant to be overridden to add dependencies.
+   *  To read the value, you should use [[moduleDepsChecked]] instead,
+   *  which uses a cached result which is also checked to be free of cycle.
    *  @see [[moduleDepschecked]]
    */
   def moduleDeps: Seq[JavaModule] = Seq.empty
 
-  /** Same as [[moduleDeps]] but checked to not contain cycles. */
+  /**
+   * Same as [[moduleDeps]] but checked to not contain cycles.
+   * Prefer this over using [[moduleDeps]] directly.
+   */
   final def moduleDepsChecked: Seq[JavaModule] = {
     // trigger initialization to check for cycles
     recModuleDeps
@@ -517,7 +534,10 @@ trait JavaModule
    * necessary to run this module's code after compilation
    */
   def runClasspath: T[Seq[PathRef]] = T {
-    resolvedRunIvyDeps().toSeq ++ transitiveLocalClasspath() ++ localClasspath()
+    super.runClasspath() ++
+      resolvedRunIvyDeps().toSeq ++
+      transitiveLocalClasspath() ++
+      localClasspath()
   }
 
   /**
@@ -668,13 +688,19 @@ trait JavaModule
    * Any command-line parameters you want to pass to the forked JVM under `run`,
    * `test` or `repl`
    */
-  def forkArgs: T[Seq[String]] = T { Seq.empty[String] }
+  def forkArgs: T[Seq[String]] = T {
+    // overridden here for binary compatibility (0.11.x)
+    super.forkArgs()
+  }
 
   /**
    * Any environment variables you want to pass to the forked JVM under `run`,
    * `test` or `repl`
    */
-  def forkEnv: T[Map[String, String]] = T.input { T.env }
+  def forkEnv: T[Map[String, String]] = T {
+    // overridden here for binary compatibility (0.11.x)
+    super.forkEnv()
+  }
 
   /**
    * Builds a command-line "launcher" file that can be used to run this module's
@@ -789,8 +815,10 @@ trait JavaModule
     }
   }
 
-  /** Control whether `run*`-targets should use an args file to pass command line args, if possible. */
-  def runUseArgsFile: T[Boolean] = T { scala.util.Properties.isWin }
+  def runUseArgsFile: T[Boolean] = T {
+    // overridden here for binary compatibility (0.11.x)
+    super.runUseArgsFile()
+  }
 
   /**
    * Runs this module's code in-process within an isolated classloader. This is
@@ -1004,7 +1032,10 @@ trait JavaModule
    */
   def artifactSuffix: T[String] = platformSuffix()
 
-  def forkWorkingDir: T[Path] = T { T.workspace }
+  def forkWorkingDir: T[Path] = T {
+    // overridden here for binary compatibility (0.11.x)
+    super.forkWorkingDir()
+  }
 
   /**
    * Files extensions that need to be managed by Zinc together with class files.

scalalib/src/mill/scalalib/RunModule.scala

@@ -0,0 +1,35 @@
+package mill.scalalib
+
+import mill.T
+import mill.define.Module
+import mill.api.JsonFormatters.pathReadWrite
+import mill.api.PathRef
+
+trait RunModule extends Module {
+
+  /**
+   * Any command-line parameters you want to pass to the forked JVM.
+   */
+  def forkArgs: T[Seq[String]] = T { Seq.empty[String] }
+
+  /**
+   * Any environment variables you want to pass to the forked JVM.
+   */
+  def forkEnv: T[Map[String, String]] = T.input { T.env }
+
+  def forkWorkingDir: T[os.Path] = T { T.workspace }
+
+  /**
+   * All classfiles and resources including upstream modules and dependencies
+   * necessary to run this module's code.
+   */
+  def runClasspath: T[Seq[PathRef]] = T { Seq.empty[PathRef] }
+
+  /**
+   * Control whether `run*`-targets should use an args file to pass command line args, if possible.
+   */
+  def runUseArgsFile: T[Boolean] = T { scala.util.Properties.isWin }
+
+//  def zincWorker: ModuleRef[ZincWorkerModule] = ModuleRef(mill.scalalib.ZincWorkerModule)
+
+}