Fix: Parallel testing should have a fast path for the 1-class-in-module case (#5040)

#4869

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

Author: arturaz

example/androidlib/java/1-hello-world/build.mill

@@ -107,7 +107,7 @@ object app extends AndroidAppModule {
 > ./mill show app.test
 ...compiling 2 Java source...
 
-> cat out/app/test/testForked.dest/worker-0/out.json
+> cat out/app/test/testForked.dest/out.json
 ["",[{"fullyQualifiedName":"com.helloworld.ExampleUnitTest.textSize_isCorrect","selector":"com.helloworld.ExampleUnitTest.textSize_isCorrect","duration":...,"status":"Success"}]]
 
 */

example/depth/sandbox/2-test/build.mill

@@ -47,13 +47,13 @@ object bar extends MyModule
 /** Usage
 
 > find . | grep generated.html
-.../out/foo/test/testForked.dest/worker-0/sandbox/generated.html
-.../out/bar/test/testForked.dest/worker-0/sandbox/generated.html
+.../out/foo/test/testForked.dest/sandbox/generated.html
+.../out/bar/test/testForked.dest/sandbox/generated.html
 
-> cat out/foo/test/testForked.dest/worker-0/sandbox/generated.html
+> cat out/foo/test/testForked.dest/sandbox/generated.html
 <h1>hello</h1>
 
-> cat out/bar/test/testForked.dest/worker-0/sandbox/generated.html
+> cat out/bar/test/testForked.dest/sandbox/generated.html
 <p>world</p>
 
 */

example/depth/sandbox/3-breaking/build.mill

@@ -36,9 +36,9 @@ object foo extends JavaModule {
 
 > find . | grep .html
 ...
-.../out/foo/test/testForked.dest/worker-0/sandbox/foo.html
+.../out/foo/test/testForked.dest/sandbox/foo.html
 
-> cat out/foo/test/testForked.dest/worker-0/sandbox/foo.html
+> cat out/foo/test/testForked.dest/sandbox/foo.html
 <h1>foo</h1>
 
 */

libs/scalalib/src/mill/scalalib/TestModuleUtil.scala

@@ -58,8 +58,10 @@ private final class TestModuleUtil(
         "\nRun discoveredTestClasses to see available tests"
     )
 
+    /** This is filtered by mill. */
     val filteredClassLists0 = testClassLists.map(_.filter(globFilter)).filter(_.nonEmpty)
 
+    /** This is filtered by the test framework. */
     val filteredClassLists =
       if (filteredClassLists0.size == 1 && !testParallelism) filteredClassLists0
       else {
@@ -102,7 +104,10 @@ private final class TestModuleUtil(
       }
     if (selectors.nonEmpty && filteredClassLists.isEmpty) throw doesNotMatchError
 
-    val result = if (testParallelism) {
+    /** If we only have a single group with one test there is no point in running things in parallel. */
+    val parallelismIsUseless =
+      filteredClassLists.sizeIs == 1 && filteredClassLists.forall(_.sizeIs == 1)
+    val result = if (testParallelism && !parallelismIsUseless) {
       runTestQueueScheduler(filteredClassLists)
     } else {
       runTestDefault(filteredClassLists)

libs/scalalib/test/resources/utestSingleTest/src/FooTests.scala

@@ -0,0 +1,11 @@
+package utestSingleTest.src
+
+import utest._
+
+object FooTests extends TestSuite {
+  val tests = Tests {
+    test("test") {
+      true
+    }
+  }
+}

libs/scalalib/test/src/mill/scalalib/TestRunnerParallelismTests.scala

@@ -0,0 +1,35 @@
+package mill.scalalib
+
+import mill.T
+import mill.define.Discover
+import mill.testkit.{TestBaseModule, UnitTester}
+import utest.*
+import mill.util.TokenReaders.*
+
+object TestRunnerParallelismTests extends TestSuite {
+  object utestSingleTest extends TestBaseModule with ScalaModule with TestModule.Utest {
+    override def scalaVersion: T[String] = sys.props.getOrElse("TEST_SCALA_2_13_VERSION", ???)
+
+    override def utestVersion = sys.props.getOrElse("TEST_UTEST_VERSION", ???)
+
+    override def testParallelism = true
+
+    lazy val millDiscover = Discover[this.type]
+  }
+
+  override def tests: Tests = Tests {
+    test("single group with single test") - UnitTester(
+      utestSingleTest,
+      os.Path(sys.env("MILL_TEST_RESOURCE_DIR")) / "utestSingleTest"
+    ).scoped { eval =>
+      val Right(result) = eval.apply(utestSingleTest.testForked()): @unchecked
+      val (doneMsg @ _, results) = result.value
+      // Only one test should have been run
+      assert(results.size == 1)
+      // If we are actually running in serial mode (because there was only 1 group and 1 test case, so we disabled
+      // parallelism) there should be no 'worker-X' directories and the 'sandbox' should be at the top level.
+      assert(os.exists(eval.outPath / "testForked.dest" / "sandbox"))
+    }
+  }
+
+}

libs/scalalib/test/src/mill/scalalib/TestRunnerScalatestTests.scala

@@ -29,25 +29,23 @@ object TestRunnerScalatestTests extends TestSuite {
 
       test("singleClass") - tester.testOnly(
         Seq("mill.scalalib.ScalaTestSpec"),
-        3,
-        Map(
-          // No test grouping is triggered because we only run one test class
-          testrunner.scalatest -> Set(
-            "out.json",
-            "result.log",
-            "sandbox",
-            "test-report.xml",
-            "testargs"
-          ),
-          testrunnerGrouping.scalatest -> Set(
+        3, {
+          val results = Set(
             "out.json",
             "result.log",
             "sandbox",
             "test-report.xml",
             "testargs"
-          ),
-          testrunnerWorkStealing.scalatest -> Set("worker-0", "test-classes", "test-report.xml")
-        )
+          )
+
+          Map(
+            testrunner.scalatest -> results,
+            // No test grouping is triggered because we only run one test class
+            testrunnerGrouping.scalatest -> results,
+            // No workers are triggered because we only run one test class
+            testrunnerWorkStealing.scalatest -> results
+          )
+        }
       )
 
       // Runs three test classes with 3 test cases each, and trigger test grouping

libs/scalalib/test/src/mill/scalalib/TestRunnerTests.scala

@@ -26,6 +26,7 @@ object TestRunnerTests extends TestSuite {
           junitReportIn(eval.outPath, "doneMessageFailure").shouldHave(1 -> Status.Failure)
         }
       }
+
       test("success") {
         val outStream = new ByteArrayOutputStream()
         UnitTester(

libs/scalalib/test/src/mill/scalalib/TestRunnerUtestTests.scala

@@ -32,25 +32,22 @@ object TestRunnerUtestTests extends TestSuite {
       test("prefix") - tester.testOnly(Seq("mill.scalalib.FooT*"), 1)
       test("exactly") - tester.testOnly(
         Seq("mill.scalalib.FooTests"),
-        1,
-        Map(
-          testrunner.utest -> Set(
-            "out.json",
-            "result.log",
-            "sandbox",
-            "test-report.xml",
-            "testargs"
-          ),
-          // When there is only one test group with test classes, we do not put it in a subfolder
-          testrunnerGrouping.utest -> Set(
+        1, {
+          val results = Set(
             "out.json",
             "result.log",
             "sandbox",
             "test-report.xml",
             "testargs"
-          ),
-          testrunnerWorkStealing.utest -> Set("worker-0", "test-classes", "test-report.xml")
-        )
+          )
+          Map(
+            testrunner.utest -> results,
+            // When there is only one test group with test classes, we do not put it in a subfolder
+            testrunnerGrouping.utest -> results,
+            // When there is only one test group with test classes, we do not run workers
+            testrunnerWorkStealing.utest -> results
+          )
+        }
       )
       test("multi") - tester.testOnly(
         Seq("*Bar*", "*bar*"),

testkit/src/mill/testkit/ExampleTester.scala

@@ -112,7 +112,10 @@ class ExampleTester(
       case s"curl $rest" => s"curl --retry 7 --retry-all-errors $rest"
       case s => s
     }
-    Console.err.println(s"$workspacePath> $commandStr")
+
+    /** The command we're about to execute */
+    val debugCommandStr = s"$workspacePath> $commandStr"
+    Console.err.println(debugCommandStr)
     Console.err.println(
       s"""--- Expected output ----------
          |${expectedSnippets.mkString("\n")}
@@ -142,14 +145,16 @@ class ExampleTester(
         fansi.Str(res.out.text(), errorMode = fansi.ErrorMode.Strip).plainText,
         fansi.Str(res.err.text(), errorMode = fansi.ErrorMode.Strip).plainText
       ),
-      check
+      check,
+      debugCommandStr
     )
   }
 
   def validateEval(
       expectedSnippets: Vector[String],
       evalResult: IntegrationTester.EvalResult,
-      check: Boolean = true
+      check: Boolean = true,
+      command: String = ""
   ): Unit = {
     if (check) {
       if (expectedSnippets.exists(_.startsWith("error: "))) assert(!evalResult.isSuccess)
@@ -181,7 +186,11 @@ class ExampleTester(
     for (expectedLine <- unwrappedExpected.linesIterator) {
       Predef.assert(
         filteredOut.linesIterator.exists(globMatches(expectedLine, _)),
-        s"==== filteredOut:\n$filteredOut\n==== Missing expectedLine: \n$expectedLine"
+        (if (command == "") "" else s"==== command:\n$command\n") +
+          s"""==== filteredOut:
+             |$filteredOut
+             |==== Missing expectedLine:
+             |$expectedLine""".stripMargin
       )
     }
   }