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

Update thirdparty tests, make subprocess testrunner run user code in root classloader #2561

Merged
merged 13 commits into from
Jun 4, 2023

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented May 31, 2023

  1. Move thirdparty from integration/ to example/, and simplify them down to acyclic, fansi, and jimfs. We can add more complex builds later if we want, but for now just having some realistic builds that are not bitrotted and old is already an improvement over what we have now.

  2. Flip the sub-process TestRunner classloaders on its head: rather than runner TestRunner.scala in the boot classloader and user code in a sub-classloader, we run user code in the boot classloader and TestRunner.scala in a sub classloader. This is necessary to make JimFS work, and would probably fix Tests fail due to class loading issue - GraalVM cannot be used. #716 and related issues.

  3. Broke up and cleaned up TestRunner.scala:

    1. Separated TestRunner and TestRunnerMain0, the two entrypoints, from TestRunnerUtils
    2. Replaced all the complicated logic for converting logic to/from CLI flags with JSON written to a file by TestModule and read from the file by TestRunner
    3. Replaced ClosableIterator with geny.Generator, which also provides guaranteed closing after iteration
  4. Marked most of testrunner as @internal. Most of these could be private[mill], but I think we don't have a clear idea of how this stuff is meant to be used yet, and it's plausible that other people may want to interact with the testrunner in their own custom test suites. I think @internal is better suited for now until we either (a) know better what a stable API looks like or (b) decide that this stuff should not be used externally.

We have much more control over TestRunner.scala than we do over user code, so we can make sure that TestRunner.scala works when running in a semi-isolated classloader, whereas for user code we can offer no such guarantees. We can't do anything for .testLocal that runs in-process, but at least for .test we can try to offer them a clean classloader environment.

The classloader for user code is not 100% clean: we still have a single class mill.testrunner.entrypoint.TestRunnerMain added within it. That's probably OK, since no matter what we do w.r.t. shading and stuff we still need somewhere to put our own main method to bootstrap the mill.testrunner classloader.

The testrunner changes are covered by existing unit and integration/example tests, and the new "user code runs in clean classloader" logic is covered by example.thirdparty[jimfs].local.

@lihaoyi lihaoyi changed the title [WIP] Overhaul thirdparty tests [WIP] Update thirdparty tests, overhaul subprocess testrunner May 31, 2023
@lihaoyi lihaoyi changed the title [WIP] Update thirdparty tests, overhaul subprocess testrunner [WIP] Update thirdparty tests, make subprocess testrunner run user code in root classloader May 31, 2023
Comment on lines -143 to -164
private def copy(
framework: String = framework,
classpath: Seq[String] = classpath,
arguments: Seq[String] = arguments,
sysProps: Map[String, String] = sysProps,
outputPath: String = outputPath,
colored: Boolean = colored,
testCp: String = testCp,
homeStr: String = homeStr,
globSelectors: Seq[String] = globSelectors
): TestArgs = new TestArgs(
framework,
classpath,
arguments,
sysProps,
outputPath,
colored,
testCp,
homeStr,
globSelectors
)

Copy link
Member

Choose a reason for hiding this comment

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

We made this one private and some other tricks to keep the option to evolve this class in a binary compatible way. What's the intention of changing that?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is still WIP, but I want to try to adjust the code path so we don't need to use a case class here. That would make maintaining binary compatibility much simpler

@lihaoyi lihaoyi marked this pull request as ready for review June 1, 2023 05:08
@lihaoyi lihaoyi requested a review from lolgab June 1, 2023 05:08
@lihaoyi
Copy link
Member Author

lihaoyi commented Jun 1, 2023

Should be ready for review I think

@lihaoyi lihaoyi changed the title [WIP] Update thirdparty tests, make subprocess testrunner run user code in root classloader Update thirdparty tests, make subprocess testrunner run user code in root classloader Jun 1, 2023
@lihaoyi lihaoyi requested a review from lefou June 1, 2023 14:00
@lihaoyi lihaoyi merged commit f944ceb into com-lihaoyi:main Jun 4, 2023
@lefou lefou added this to the 0.11.0-M11 milestone Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests fail due to class loading issue - GraalVM cannot be used.
2 participants