Restructure `TestModule`, add `RunModule` #3064

lefou · 2024-03-05T22:56:48Z

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.

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:

Split up `JavaModule` #3076

This should it make easier to define multiple test modules with differnt test frameworks on the same test classpath.

lefou · 2024-03-18T06:58:42Z

scalalib/src/mill/scalalib/JavaModule.scala

+     */
+    def testClasspath: T[Seq[PathRef]] = T {
+      // bin-compat-shim: keep the super.call in the classfile
+      super.testClasspath


Calling super here but without calling apply, so even when not actual using the super-target here (we don't insert a real dependency to the super-target), we ensure the super-reference is generated in the byte-code. This allows us later to use the super-target without worrying about binary compatibility.

This is some new pattern I want to try out and establish. If it works out, we could automate it in the T-macros.

See for more details:

Mill module / target overriding vs Java binary compatibility #2987

lefou · 2024-03-20T08:18:47Z

@lihaoyi , @lolgab , do you have any concerns about this pull request?

lihaoyi · 2024-03-20T09:48:14Z

Looks good to me. I've needed this before: apart from the testFramework use case discussed earlier, I've needed to share the same compiled code but testing it with different flags or environment variables. It's possible to do today with a bunch of override hacks, but having TestModule be separated out is definitely a lot cleaner

lefou · 2024-03-20T09:51:07Z

I've needed this before: apart from the testFramework use case discussed earlier, I've needed to share the same compiled code but testing it with different flags or environment variables. It's possible to do today with a bunch of override hacks, but having TestModule be separated out is definitely a lot cleaner

That's probably even better covered by my other subsequent PR which is in the making:

Move run-targets into RunModule #3090

It's exactly for the case, when you want run with different flags, envs, or main classes or classpaths.

This PR moves the definition of the `run` targets from `JavaModule` into `RunModule`. Also some other targets where moved, e.g. `mainClass`, `finalMainClassOpt` and `finalMainClass`. The advantage of having them in `RunModule` is, that one can easily define and configure additional runners as sub-modules. Example: Define a `daemon` sub-module which acts as runner for the `cli.DaemonTool` class. ```scala import mill._, scalalib._ object foo extends RootModule with JavaModule { def mainClass = "cli.Main" object daemon extends RunModule { def runClasspath = foo.runClasspath def localRunClasspath = foo.localRunClasspath def mainClass = "cli.DaemonTool" } } ``` To preserve binary compatibility, all moved `def`s retain an override in `JavaModule` and forward to their `super`-`def` in `RunModule`. Therefore traits compiled against older versions of trait `JavaModule` should still be runnable with newer versions. Some `run`-targets (`run`, `runLocal` and `runBackground`) previously required the `compile` target, since that also provided some zinc analysis which included some main class discovery. Since the goal was to decouple the `run` targets from the concept of compilation, I implemented a different discovery logic for main classes which uses the Scala API of `scalap`. This is completely newly written code but it's only a couple of lines and all existing tests (which also include main class discovery) succeeded. The advantage of the new logic is, that it should work with any given classpath and also for non-Scala classes. The new code is located in the zinc worker, since this is already shared between all `RunModule`s, but there is no real need to have it there. To avoid increased complexity, I resisted to introduce a new shared worker just for the sake of technical independence, for now. The new `allLocalMainClasses` target can be used to find all main classes in the `localRunClasspath`. This is some useful information I needed now and then in projects, so it makes sense to have it in a dedicated target for better caching and easy `show`ing. I also introduced a new `localRunClasspath` target which abstracts away the classpath that is supposed to be produced by compilation. This is somewhat analogue to the `testClassapth` introdcued in PR #3064. Since both typically hold the same classpath, `testClasspath` by default uses the result of `localRunClasspath`. We probably could remove `testClasspath` altogether, but it's semantically bound to `TestModule` and isn't necessarily the same as `localRunClasspath`, so I think it's legit to keep it. For consistency, I also added a `localCompileClasspath` which resembles the part of the `localClasspath` which is feed into the compiler. All added classpath targets also have a version for BSP (named with prefix `bsp` and returning a `T[Agg[UnresolvedPath]]`) when appropriate. Pull request: #3090

chikei · 2024-04-03T10:50:07Z

in case someone else trying out similar setup, in IJ you also need override def bspBuildTarget = super.bspBuildTarget.copy(canCompile = false, canRun = false, languageIds = Seq.empty) in the additional test module to prevent BuildTarget{Scalac, Javac}Options bsp requests which mill will eventually calls the dummy compile method and cause unimplemented exception.

lefou · 2024-04-03T15:21:31Z

in case someone else trying out similar setup, in IJ you also need override def bspBuildTarget = super.bspBuildTarget.copy(canCompile = false, canRun = false, languageIds = Seq.empty) in the additional test module to prevent BuildTarget{Scalac, Javac}Options bsp requests which mill will eventually calls the dummy compile method and cause unimplemented exception.

Nice catch, @chikei ! That was probably an oversight. Can you show your complete example? I wonder why the canCompile is true to start with.

chikei · 2024-04-04T07:35:53Z

Honestly I am not really sure if tweaking canCompile and canRun matters, IJ's code seems only care language id, but IJ's bsp import UI flow is so annoy I just don't bother to reduce.

As for canCompile = true, that's due to I used some other common trait which extends SbtModuleTests instead of the minimal TestModule with TestModule.ScalaTest showed here.

lefou · 2024-04-04T07:45:20Z

Honestly I am not really sure if tweaking canCompile and canRun matters, IJ's code seems only care language id, but IJ's bsp import UI flow is so annoy I just don't bother to reduce.

We currently don't check the canCompile before running the compilation in each module, we just communicate it to the BSP client. I think the client is supposed to not request compilation for that module at all.

As for canCompile = true, that's due to I used some other common trait which extends SbtModuleTests instead of the minimal TestModule with TestModule.ScalaTest showed here.

Ok. So I think if you stay with the simple example from this PR, canCompile should already be set to false. If that fails in IJ too, it's an issue worth to be reported there.

If you extends from an real language module like SbtModuleTests, which is set up to compile files, you either need to tweak the BSP settings when you customize the compile target or you need to implement the compile in a more defensive way. The refactoring of this PR is exactly the solution to this issue.

chikei · 2024-04-08T07:20:12Z

So I tried defining additional test without an real language module, this line in mill will throw a MatchError as the additional test module does not inherent JavaModule.

lefou · 2024-04-08T10:59:07Z

So I tried defining additional test without an real language module, this line in mill will throw a MatchError as the additional test module does not inherent JavaModule.

This is in fact a bug. I opened #3110 to track it.

chikei · 2024-04-10T10:26:53Z

So with PR #3111, I can make IJ import without error, but build project will fail as mill return statusCode = ERROR.

Skimming through IJ's BSP code, my understanding is that IJ ignores BuildTargetCapabilities properties returned from WorkspaceBuildTargets call, therefore build project in IJ results a BuildTargetCompile call with all BuildTarget fromWorkspaceBuildTargets, and mill reports failure for non JavaModule.

lefou · 2024-04-10T13:17:46Z

@chikei Thanks for the feedback.

lefou changed the title ~~Introduce a TestModule.testClasspath to decouple from compilation~~ Restructure TestModule to make it independent of compilation Mar 6, 2024

lefou changed the title ~~Restructure TestModule to make it independent of compilation~~ Restructure TestModule, add RunModule Mar 11, 2024

lefou force-pushed the test-classpath branch from 6807808 to 19e122a Compare March 11, 2024 22:49

lefou added 14 commits March 17, 2024 23:58

Introduce a TestModule.testClasspath to decouple from compilation

148905a

This should it make easier to define multiple test modules with differnt test frameworks on the same test classpath.

Restructured TestModule, added RunBaseModule, added test case

c6809f7

Print each ran testcase in ExampleTestSuite

8df934b

docs

f9babf3

Removed TestModule.testRunModule again

ae80a34

Introduce a ZincWorkerAware trait

51aed91

Consolidate RunModule

2a96c65

.

3ca3e2c

.

e547a6f

.

2f08d5c

Formate expected output

3e3cf7e

Add MiMa ignore filter and ensure super-calls when we moved defs up

8296be4

.

5aced5c

.

d725414

lefou force-pushed the test-classpath branch from 19e122a to d725414 Compare March 17, 2024 22:59

lefou marked this pull request as ready for review March 18, 2024 06:36

docs

bfc49f0

lefou requested review from lihaoyi and lolgab March 18, 2024 07:02

lefou mentioned this pull request Mar 18, 2024

Move run-targets into RunModule #3090

Merged

1 task

lefou commented Mar 18, 2024

View reviewed changes

lefou merged commit 6e0de1c into main Mar 20, 2024
38 checks passed

lefou deleted the test-classpath branch March 20, 2024 09:52

lefou added this to the 0.11.8 milestone Mar 20, 2024

lefou mentioned this pull request Apr 8, 2024

BSP: MatchError when a TestModule is used in isolation #3110

Closed

lefou mentioned this pull request Apr 11, 2024

Isolate scoverage modules from their parent modules #3118

Merged

lefou mentioned this pull request May 3, 2024

Refactor assembly logic into a AssemblyModule #3141

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure `TestModule`, add `RunModule` #3064

Restructure `TestModule`, add `RunModule` #3064

lefou commented Mar 5, 2024 •

edited

Loading

lefou Mar 18, 2024

lefou commented Mar 20, 2024

lihaoyi commented Mar 20, 2024

lefou commented Mar 20, 2024 •

edited

Loading

chikei commented Apr 3, 2024 •

edited

Loading

lefou commented Apr 3, 2024 •

edited

Loading

chikei commented Apr 4, 2024

lefou commented Apr 4, 2024 •

edited

Loading

chikei commented Apr 8, 2024

lefou commented Apr 8, 2024

chikei commented Apr 10, 2024

lefou commented Apr 10, 2024

Restructure TestModule, add RunModule #3064

Restructure TestModule, add RunModule #3064

Conversation

lefou commented Mar 5, 2024 • edited Loading

lefou Mar 18, 2024

Choose a reason for hiding this comment

lefou commented Mar 20, 2024

lihaoyi commented Mar 20, 2024

lefou commented Mar 20, 2024 • edited Loading

chikei commented Apr 3, 2024 • edited Loading

lefou commented Apr 3, 2024 • edited Loading

chikei commented Apr 4, 2024

lefou commented Apr 4, 2024 • edited Loading

chikei commented Apr 8, 2024

lefou commented Apr 8, 2024

chikei commented Apr 10, 2024

lefou commented Apr 10, 2024

Restructure `TestModule`, add `RunModule` #3064

Restructure `TestModule`, add `RunModule` #3064

lefou commented Mar 5, 2024 •

edited

Loading

lefou commented Mar 20, 2024 •

edited

Loading

chikei commented Apr 3, 2024 •

edited

Loading

lefou commented Apr 3, 2024 •

edited

Loading

lefou commented Apr 4, 2024 •

edited

Loading