Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Allow to run tests with a two-level classloader setup #638

Conversation

alexarchambault
Copy link
Contributor

@alexarchambault alexarchambault commented Jun 24, 2019

This PR allows to run tests via a two-level classloader setup (some JARs are loaded by a first classloader, and others by a second classloader, with the first one as parent).

This can be enabled by passing a Java property test.base.classpath to the JVM running the tests, with something like this in a test build definition:

def forkArgs = super.forkArgs() ++ Seq(
  "-Dtest.base.classpath=" + moduleInTopLoader()
    .runClasspath()
    .map(_.path.toIO.getAbsolutePath)
    .mkString(java.io.File.pathSeparator)
)

The list of JARs to load in the first classloader is passed via this Java property. The remaining test JARs are loaded by the second classloader.

Context for this is this Ammonite PR, that adds classloader isolation capabilities to Ammonite. During the Ammonite tests, things accessible from the test sessions code (Ammonite APIs, …) are loaded by the first classloader, the rest (core of Ammonite, test helpers, …) is loaded by the second loader.

@alexarchambault
Copy link
Contributor Author

I'm not too sure if / where tests should be added in this repository, for this feature.

@lefou
Copy link
Member

lefou commented Jun 25, 2019

I'm not exactly sure why that change is needed. IIUC, you changed ammonite so that it loads parts of its implementation classes from a second classloader. But how is this related to the general support in mill to run tests? Shouldn't it be specific to cases where we run/test some code that depends on ammonite.

@lefou
Copy link
Member

lefou commented Jun 25, 2019

Or to ask in other words: This PR hardcodes support for the test.base.classpath system property, whereas it could be simply supported out of the box by customizing the build.sc when needed. Or am I wrong?

@alexarchambault
Copy link
Contributor Author

@lefou Yeah, I was starting to feel the same, actually… I'm going to try with a custom test runner in Ammonite itself. (Will close this depending on how it goes.)

@lihaoyi
Copy link
Member

lihaoyi commented Jul 1, 2019

Seems to be superseded by the changes in Ammonite

@lihaoyi lihaoyi closed this Jul 1, 2019
@alexarchambault alexarchambault deleted the test-base-classpath branch July 1, 2019 13:10
@lefou lefou added the invalid This issue is invalid or lacks required information label Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This issue is invalid or lacks required information
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants