Switched to single test framework per TestModule #1268

lefou · 2021-04-10T20:35:15Z

Introduced new methods and targets where appropriate.
Deprecated all methods that support multiple frameworks.

See also discussion in #170

lolgab · 2021-04-12T10:56:54Z

scalajslib/src/ScalaJSModule.scala

@@ -218,13 +218,13 @@ trait TestScalaJSModule extends ScalaJSModule with TestModule {
    val (close, framework) = mill.scalajslib.ScalaJSWorkerApi.scalaJSWorker().getFramework(
      toolsClasspath().map(_.path),
      jsEnvConfig(),
-      testFrameworks().head,


scalalib/src/TestModule.scala

lolgab

It seems strange to have mill-contrib-testng artifact with an hardcoded _2.13.
I guess it's to be used with Java projects.
If you mix it with other Scala dependencies not on Scala 2.13 it silently breaks the binary compatibility.
Maybe we should rewrite that module in pure java if possible?
Other than this it looks good to me!

lefou · 2021-04-12T11:16:43Z

It seems strange to have mill-contrib-testng artifact with an hardcoded _2.13.
I guess it's to be used with Java projects.
If you mix it with other Scala dependencies not on Scala 2.13 it silently breaks the binary compatibility.
Maybe we should rewrite that module in pure java if possible?
Other than this it looks good to me!

Oh, our testng framework is already a pure Java implementation. I think the Scala binary suffix is an oversight due to current consistent packaging setup.

lolgab · 2021-04-12T11:23:02Z

It seems strange to have mill-contrib-testng artifact with an hardcoded _2.13.
I guess it's to be used with Java projects.
If you mix it with other Scala dependencies not on Scala 2.13 it silently breaks the binary compatibility.
Maybe we should rewrite that module in pure java if possible?
Other than this it looks good to me!

Oh, our testng framework is already a pure Java implementation. I think the Scala binary suffix is an oversight due to current consistent packaging setup.

I think it was a side effect of this change
If you look at Maven Central at some point ( a few commits after this ) the artifact was renamed with the extra _2.13. This because MillModule depends on Scala where MillPublishModule depends on Java only.

EDIT: Actually it started to depend on scalalib so something happened in that PR that needs to be addressed.

lolgab · 2021-04-12T11:27:23Z

From my side the issue with TestNG can be fixed later with a separate PR.
Just add a TODO where the dependency is added with the _2.13 and create a companion issue to link in the TODO

build.sc

lefou · 2021-04-12T16:08:43Z

It seems strange to have mill-contrib-testng artifact with an hardcoded _2.13.
I guess it's to be used with Java projects.
If you mix it with other Scala dependencies not on Scala 2.13 it silently breaks the binary compatibility.
Maybe we should rewrite that module in pure java if possible?
Other than this it looks good to me!

Oh, our testng framework is already a pure Java implementation. I think the Scala binary suffix is an oversight due to current consistent packaging setup.

I think it was a side effect of this change
If you look at Maven Central at some point ( a few commits after this ) the artifact was renamed with the extra _2.13. This because MillModule depends on Scala where MillPublishModule depends on Java only.

EDIT: Actually it started to depend on scalalib so something happened in that PR that needs to be addressed.

We added Integration tests which obviously need mill. ;-) I now changed this dependency to be only a dependency of the actual test module, not the outer testng framework implementation.

lolgab · 2021-04-12T16:44:24Z

bsp/src/mill/bsp/Utils.scala

          }
        )
-        classFingerprint.map(classF => classF._1.getName.stripSuffix("$"))
+        Seq.from(classFingerprint.map(classF => classF._1.getName.stripSuffix("$")))


Just curiosity: why not .toSeq?

.toSeq gives a deprecation warning, and any fix is more verbose and results in the same call.

lolgab · 2021-04-12T16:49:07Z

scalalib/src/TestModule.scala

+        "predefined TestModules: TestNg, Junit, Scalatest, ..."
+    if (frameworks.size != 1) {
+      Result.Failure(
+        "Since mill after-0.9.6 only one test framework per TestModule is supported. " ++ msg)


First time I see string concatenation with ++ :) Is it to not confuse it with the sum?

I have no idea, haha. Will fix it...

lolgab

Good job on this one! Thank you!

sake92 · 2021-04-12T19:16:24Z

Looks great really, seems like a great simplification!
Just wondering if having ivyDeps for some frameworks and not for others is a bit inconsistent?
Also, MUnit? 😄

sake92 · 2021-04-12T19:19:21Z

scalalib/src/TestModule.scala

+   * TestModule that uses Scalatest Framework to run tests.
+   * You need to provide the scalatest dependencies yourself.
+   */
+  trait Scalatest extends TestModule {


ScalaTest is more precise I think.

lefou · 2021-04-12T19:24:41Z

Looks great really, seems like a great simplification!
Just wondering if having ivyDeps for some frameworks and not for others is a bit inconsistent?

It's mainly because those which come with ivyDeps don't provide a Sbt Test Interface. The side effect of course is, that they can be used without any addional ivyDeps.

Also, the most Scala-based test frameworks are more complex in regard of their dependencies, many have extra options for e.g. ScalaCheck, or shapeless

Also, MUnit? smile

Oh, yeah. I never used it or saw it in action. But if you say, it works as smooth as all the others in mill, I can add a trait for it, too.

lefou · 2021-04-12T19:26:35Z

Looks like latest CI runs fail because org.tpolecat:tut-core is no longer available. Probably an issue with bintray sunsetting?

sake92 · 2021-04-12T19:36:38Z

Thx for adding MUnit, it's really awesome framework! :)
Yeah, probably fails bcoz Bintray stuff. Tut seems to be deprecated/abandoned anyways?

lefou · 2021-04-12T20:55:41Z

Looks like latest CI runs fail because org.tpolecat:tut-core is no longer available. Probably an issue with bintray sunsetting?

See #1269

Introduced new methods and targets where appropriate. Deprecated all methods that support multiple frameworks.

lefou · 2021-04-12T21:04:56Z

Thank you, @lolgab and @sake92, for you valuable feedback.

lefou marked this pull request as ready for review April 12, 2021 10:02

lolgab reviewed Apr 12, 2021

View reviewed changes

scalalib/src/TestModule.scala Outdated Show resolved Hide resolved

lolgab requested changes Apr 12, 2021

View reviewed changes

lolgab reviewed Apr 12, 2021

View reviewed changes

build.sc Show resolved Hide resolved

lefou requested a review from lolgab April 12, 2021 16:05

lolgab reviewed Apr 12, 2021

View reviewed changes

lolgab approved these changes Apr 12, 2021

View reviewed changes

sake92 reviewed Apr 12, 2021

View reviewed changes

lefou added 11 commits April 12, 2021 22:56

Switched to single test framework per TestModule

7b83c06

Introduced new methods and targets where appropriate. Deprecated all methods that support multiple frameworks.

Fixed error message

a2ef4be

Adapted play-json build.sc to one-framework-per-test-module

583abcb

Moved TestModule into own file, added more test framework traits

ed27f84

Fixed some prepareOffline command invocations

2225e4d

Added Utest to predefined test frameworks

670824d

Replaced some test framework configurations with predefined traits

ba89502

Removed explicit framework dependencies and versions

7ce8930

Fix inconsistencies with mill-contrib-testng artifact name

99bac48

Moved mill dependencies of testng module into test module

dfd625f

Made testng module completely scala free

305bc2e

lefou added 4 commits April 12, 2021 22:57

Fixed docJar target (without scala deps)

9e1a961

Mill version bump

3d209c1

Fixed string concatenation

f385090

Added Munit trait and renamed ScalaTest trait

dd2ec71

lefou force-pushed the testframework branch from e8a5732 to dd2ec71 Compare April 12, 2021 20:59

lefou merged commit b261b5f into com-lihaoyi:master Apr 12, 2021

lefou deleted the testframework branch April 12, 2021 21:03

lefou added this to the after 0.9.6 milestone Apr 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switched to single test framework per TestModule #1268

Switched to single test framework per TestModule #1268

lefou commented Apr 10, 2021 •

edited

Loading

lolgab Apr 12, 2021

lolgab left a comment

lefou commented Apr 12, 2021

lolgab commented Apr 12, 2021 •

edited

Loading

lolgab commented Apr 12, 2021

lefou commented Apr 12, 2021

lolgab Apr 12, 2021

lefou Apr 12, 2021

lolgab Apr 12, 2021

lefou Apr 12, 2021

lolgab left a comment

sake92 commented Apr 12, 2021

sake92 Apr 12, 2021

lefou commented Apr 12, 2021 •

edited

Loading

lefou commented Apr 12, 2021

sake92 commented Apr 12, 2021

lefou commented Apr 12, 2021

lefou commented Apr 12, 2021

Switched to single test framework per TestModule #1268

Switched to single test framework per TestModule #1268

Conversation

lefou commented Apr 10, 2021 • edited Loading

lolgab Apr 12, 2021

Choose a reason for hiding this comment

lolgab left a comment

Choose a reason for hiding this comment

lefou commented Apr 12, 2021

lolgab commented Apr 12, 2021 • edited Loading

lolgab commented Apr 12, 2021

lefou commented Apr 12, 2021

lolgab Apr 12, 2021

Choose a reason for hiding this comment

lefou Apr 12, 2021

Choose a reason for hiding this comment

lolgab Apr 12, 2021

Choose a reason for hiding this comment

lefou Apr 12, 2021

Choose a reason for hiding this comment

lolgab left a comment

Choose a reason for hiding this comment

sake92 commented Apr 12, 2021

sake92 Apr 12, 2021

Choose a reason for hiding this comment

lefou commented Apr 12, 2021 • edited Loading

lefou commented Apr 12, 2021

sake92 commented Apr 12, 2021

lefou commented Apr 12, 2021

lefou commented Apr 12, 2021

lefou commented Apr 10, 2021 •

edited

Loading

lolgab commented Apr 12, 2021 •

edited

Loading

lefou commented Apr 12, 2021 •

edited

Loading