Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add check for right Tests traits in ScalaJS and Native #2874

Merged
merged 11 commits into from
Nov 15, 2023

Conversation

lolgab
Copy link
Member

@lolgab lolgab commented Nov 14, 2023

Motivation

People get confused when they do things like:

import mill._
import mill.scalalib._
import mill.scalajslib._

object root extends ScalaJSModule {
  def scalaVersion: T[String] = "3.3.1"
  def scalaJSVersion: T[String] = "1.14.0"
  object test extends ScalaTests with TestModule.Utest
}

which doesn't work since we need to extend ScalaJSTests instead of ScalaTests.
Now we crash with an exception:

[build.sc] [49/53] compile 
[info] compiling 1 Scala source to /Users/lorenzo/scala/repro/out/mill-build/compile.dest/classes ...
[info] done compiling
[build.sc] [53/53] methodCodeHashSignatures 
mill.api.MillException: root is a `ScalaJSModule`. root.test needs to extend `ScalaJSTests`.
    mill.scalalib.ScalaModule$ScalaTests.$init$(ScalaModule.scala:37)
    millbuild.build$root$test$.<init>(build.sc:8)
    millbuild.build$root$.test$lzycompute$1(build.sc:30)
    millbuild.build$root$.test(build.sc:30)
    java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.base/java.lang.reflect.Method.invoke(Method.java:566)
    mill.resolve.ResolveCore$.$anonfun$resolveDirectChildren0$10(ResolveCore.scala:272)

Pull Request: #2874

@lolgab lolgab marked this pull request as ready for review November 14, 2023 13:53
@lolgab lolgab requested review from lefou and lihaoyi November 14, 2023 13:53
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A great addition to fix some regularly seen user problems. See my remarks below.

scalalib/src/mill/scalalib/ScalaModule.scala Outdated Show resolved Hide resolved
scalalib/src/mill/scalalib/ScalaModule.scala Outdated Show resolved Hide resolved
scalalib/src/mill/scalalib/ScalaModule.scala Outdated Show resolved Hide resolved
scalalib/src/mill/scalalib/ScalaModule.scala Outdated Show resolved Hide resolved
@lolgab lolgab requested a review from lefou November 15, 2023 06:31
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@lefou
Copy link
Member

lefou commented Nov 15, 2023

@lolgab Just to be sure we don't overlook something. Are there definitely no valid use cases to have a ScalaTests module inside a ScalaJSModule? If we are not certain, we at least need some way to not fail the build.

@lolgab lolgab merged commit e93f6bd into com-lihaoyi:main Nov 15, 2023
37 checks passed
@lolgab lolgab deleted the check-right-module-js-native branch November 15, 2023 08:40
@lefou lefou added this to the 0.11.6 milestone Nov 15, 2023
@lolgab
Copy link
Member Author

lolgab commented Nov 15, 2023

@lolgab Just to be sure we don't overlook something. Are there definitely no valid use cases to have a ScalaTests module inside a ScalaJSModule? If we are not certain, we at least need some way to not fail the build.

Sorry, I had already merged the pull request.
Anyway, I don't think there are use cases for it.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants