Issue about discovering test classes

[HollandDM]

Our company currently facing a problem while trying to migrate to mill 1.1.0 - RC3. This test will fail

test-mill.zip

After a little digging, we found out it was a combination of some decision across ZIO test and Mill that create this problem:

In TestModuleUtil.scala:

    /** This is filtered by the test framework. */
    val filteredClassLists = {
      // If test grouping is enabled and multiple test groups are detected, we need to
      // run test discovery via the test framework's own argument parsing and filtering
      // logic once before we potentially fork off multiple test groups that will
      // each do the same thing and then run tests. This duplication is necessary so we can
      // skip test groups that we know will be empty, which is important because even an empty
      // test group requires spawning a JVM which can take 1+ seconds to realize there are no
      // tests to run and shut down
      val discoveredTests = jvmWorker.apply(
        ZincOp.GetTestTasks(
          (runClasspath ++ testrunnerEntrypointClasspath).map(_.path),
          testClasspath.map(_.path),
          testFramework,
          selectors,
          args
        ),
        javaHome = javaHome
      ).toSet

      filteredClassLists0.map(_.filter(discoveredTests)).filter(_.nonEmpty)
    }

This code, AFAIK, is suppose to do the work of other test framework, by trying to trigger the loading test process of the framework, see which test is ignored, and then skip them before distribute them into other worker(s)

in ZIO test, we got

  private def loadSpec(taskDef: TaskDef): ZIOSpecAbstract = {
    val fqcn: String = TestRunner.moduleName(taskDef.fullyQualifiedName())
    // Creating the class from magic ether
    org.portablescala.reflect.Reflect
      .lookupLoadableModuleClass(fqcn, testClassLoader)
      .getOrElse(throw new ClassNotFoundException(s"failed to load object: $fqcn"))
      .loadModule()
      .asInstanceOf[ZIOSpecAbstract]
  }

While running, this code will try to load the companion object and initialize it right away (inside .loadModule())
This cause a problem for us, because we have some test defined in scala object, and there, while mill discover the test classes, it will trigger some init actions before we really run the test.

After some though, we do think that it's more correct to let the test framework ignoring the test case by themselves instead of filtering them out first, because frameworks will do their own thing, and we cannot control it (like this case).

If removed this is too much, I think we can have a flag that go with each test framework,, and base the logic on them

[HollandDM]

In the test-mill zip file, after running the test, you will see that "Init test class" and "Init val" get println before the test is running (it's printed in the discovery phase), and then printed again while test worker run the test

[HollandDM]

Also, in big codebase like our, I think that neither ignored tests are so many that it can skip the whole test lists, nor the time saved for not spawnning new jvm is noticable. So this optimization behavior is causing more harm than good.

[lihaoyi]

@HollandDM I think the current behavior is intentional; we use the test framework to run discovery/filtering, take the resultant classes, and then farm those out to the individual worker.

This is necessary because Mill doesn't understand all the flags that test frameworks use to filter test cases and classes, and it is a common scenario that the user selects a single test class/case to run and spawning NUM_CORES separate workers only to have NUM_CORES-1 on them immediately shut down is wasteful.

[HollandDM]

I understand this optimization, but it’s pretty clear that some test framework have to do more things to discover which test they will run (i suspect zio need to do it because they use Aspect to annotate the ignored case)
In that case, we should keep this filtering logic behind a flag. We can default some framework to do this if it’s safe, for other cases, we can set it to false. And user can decide this behavior too

[lihaoyi]

I think this is probably working as intended. Perhaps as a workaround you can move the stuff in your tests into lazy vals?

[lihaoyi]

Or wrap the side-effecting parts of your test suite objects into nested wrapper objects

[hoangmaihuy]

@lihaoyi For more context, we disable testParallelism and it works fine in mill 1.0.6. This issue only happens when we upgrade to mill 1.1.0-RC.

IMHO, if our tests run fine using previous stable version of Mill or other build tools, we expect them to continue to work when upgrading Mill version without updating the code, because investigating and fixing all tests is time-consuming for a large codebase.