Exception when running tests using ScalaTest #2595
Comments
|
It seems that there is a problem resolving transitive dependencies. |
|
Side note (and probably expected), I get the exact same issue with a project running zio-test |
|
Thanks for your report! This looks like a bug in Mill to me. We changed the way how we run tests and test frameworks. Instead of loading the test into a mostly isolated classloader hierarchy below the test runner, we now load the test runner into the tests class path. I guess, we might have overlooked something and need some more tweaking. |
|
Yeah, that change aimed to run the tests in as clean a classpath as possible. Maybe we need to add |
|
I was trying to investigate why this was note the case, here is the And the same with Scalatest: No signs of test-interface in the list. As, as far as I could remember, mill is using coursier to resolve the dependencies, a quick check: No results for So somehow it seems that it's not detected as a dependency by coursier. And this could actually make sense, according to https://github.com/scalatest/scalatest/blob/d5f38074ea9057a0f56045f1bfd1f6eb2e24a782/project/scalatest.scala#L147 as it's marked as Optional. Should we add it by default to the module that require it ? As far as I could test its needed for ZioTest and ScalaTest |
|
As we clearly depend on test-interface, we should always add this dependency to the run classpath, when resolving the test runner. |
|
I don't think I will have the time to contribute before the end of the week, however where would be the best place to include that ? |
|
Yeah, looks like the right location. As we work with already resolved classpaths, it's probably the good idea to pre-resolve the sbt-testing jars in a separate target and add it's result to the end. That way, we make sure test-interface is present, but if the runClasspath already contains one (which might be even newer) than that one is used. |
|
If it's the Mill code in TestRunnetMain0 that depends on test-interface, shouldnt we just add the dependency to thay module in Line 511 in e5a3990 |
|
We should! (I was not sure whether our module loading mechanism can do that now.) |
|
It should just work I think. Looking at the stack trace, it seems that the code path being exercised is within the classloader, which would contain the classpath from the mill Unrelated, I wonder why our tests didn't catch this. Maybe Scalatest/ZIOtest exercise different code paths than uTest does |
|
Probably, because the dependency is marked as optional. We should have a test case for each supported test framework. |
|
So, a bit of news, I got some time to (try) adding tests for that, see #2609 Also, regarding @lihaoyi comments about the The jar is already in the class path for this Do the tests looks good ? Should I continue with other tests framework ? I have added the libraries in the main Edit: |
|
@ex0ns interesting. If the class is actually on the classpath, then the problem may well be on this line where we try and splice the classpaths together https://github.com/com-lihaoyi/mill/blob/main/testrunner/entrypoint/src/mill/testrunner/entrypoint/TestRunnerMain.java#L29 What that lines tries to do is to ensure that the classes in Two options include:
Option (1) is probably the better one. We want to keep the parent JVM classloader as clean as possible, so as to ensure that the user test code is running in as realistic a classloading environment as possible. Option (2) is available as a fallback, but only if Option (1) doesn't work. Option (1) should work |
|
I am not sure that I fully understand, are you talking about something like that: if (name.startsWith("sbt.testing")) {
try {
return TestRunnerMain.class.getClassLoader().loadClass(name);
catch (ClassNotFoundException e) {
return super.findClass(name);
}
} else {If yes, then we still have an issue, the stack trace went from: Failing in With this try/catch, the stack trace become: Which is a failure inside val framework = frameworkInstances(cl)Another question I had, is that maybe we should change the class loader that we provide to the following code: |
|
So with the try catch, even though the immediate failure goes away, we still have a failure later on. This is because the child classloader uses the parent classloader when trying to load the I think we have no choice but to either add the |
I'd prefer to add it to |
|
Also you can use scalatest by simply setting the |
|
|
|
Looks like an additional artifact |
|
oh you are correct, and this is documented on ZIO's side it seems: https://zio.dev/reference/test/installation so I guess we don't have to do anything specific about that (except what you did to fix the tests in the PR) |
|
I triggered a manual CI run (https://github.com/com-lihaoyi/mill/actions/runs/5290282018) to publish a new snapshot release |
|
I still see the same issue although I use the latest Mill snapshot release containing the fix. https://github.com/lefou/mill-kotlin/actions/runs/5298466073/jobs/9590834819?pr=80 What am I missing? This is a scoverage enabled test module, so it could be relevant. |
|
You are probably missing |
|
I don't think that's the issue, as we always add the entrypoint classpath. |
|
But I currently wonder, whether there is a difference regarding the runtime-dependency handling from within Mill tests vs. when running standalone. |
|
Yeah, looks like that makes the difference. |
When upgrading to mill 0.11.0 I got the following stack trace when trying to run my tests using scalatest
The minimized
build.sc:Adding
test-interfaceexplicitly fixes the issue, but I think that there must be something else going on hereThe text was updated successfully, but these errors were encountered: