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

Test suite overhaul #3398

Merged
merged 78 commits into from
Aug 23, 2024
Merged

Test suite overhaul #3398

merged 78 commits into from
Aug 23, 2024

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Aug 19, 2024

Mill's own test fixtures are pretty decent, and could definitely benefit external plugin authors. But the implementation, API, and usage within Mill's own test suite has always been a mess. This PR takes the first step at centralizing all of them in mill.testkit and tries to clean them up as much as possible. This improves our own experience working in the Mill codebase, and ensures the test fixtures are in decent shape to be published and used by downstream plugin authors.

We traditionally never guaranteed binary compatibility for mill.testkit, probably for good reason since it was never as polished or designed as the rest of Mill's APIs, and this PR should get us much closer to the point at which we can start giving compatibility guarantees.

This PR is definitely too big to properly review in detail, but the test suite passing should give us some confidence I didn't break everything. It only also only touches test code, and should not affect the packaged Mill distribution at all

Major Changes

  • ExampleTestSuite, IntegrationTestSuite, and TestEvaluator have been moved into mill.testkit

  • TestEvaluator has been renamed UnitTester, and ExampleTestSuite and IntegrationTestSuite have had ExampleTester and IntegrationTester classes broken out of them. These *Tester classes contain all the test-framework-agnostic functionality, so it can easily be used independently of Mill or uTest.

    • Tried to improve and document the APIs here so they can be used externally
  • There are now IntegrationTesterTests and ExampleTesterTests to go along with UnitTesterTests (previously UnitTestTests), illustrating how to use all these helpers in standalone scenarios

UnitTester Changes

  • Consolidated most of the def workspaceTest boilerplate functions into the UnitTester class

  • A lot of test code and test data needed to be slightly tweaked to accommodate the now standardized UnitTester folder setup

  • UnitTester no longer tries to pick two nicely-named folders on disk to work with, one for the project source code and one for the out folder, and instead just does all its work inside an os.temp.dir() with out/ as a sub-folder

  • Dropped support for Scala 2.11, 3.0, 3.1 in the test suite, as well as Scala-Native 0.4, following the rest of the com-lihaoyi projects.

  • Replaced the long-deprecated utest "foo" - { syntax with test("foo"){

  • UnitTester#apply now returns a UnitTester.Result object, allowing us to fetch fields via .value and .evalCount rather than fiddling with tuples

  • MillTestKit.scala has been removed, with UnitTester and TestBaseModule broken out into their own source files, and all the helper methods to generate nice folder paths are no longer used and have been removed

IntegrationTester Changes

  • The various eval* methods have been consolidated into a singl eval method that follows OS-Lib's new os.call API

  • The various meta* methods have been consolidated into a single .meta("...").* API

  • initWorkspace no longer returns the workspace path, since it is already available through IntegrationTester#workspacePath

ExampleTester Changes

  • Moved the logic for parsing example test build.sc files into mill-build/src/ExampleParser.scala, so we can share it between Mill's own build.sc as well as the application code in mill.testkit via the meta-build

  • ExampleTester can now be used standalone via new ExampleTester(clientServerMode: Boolean, workspaceSourcePath: os.Path, millExecutable: os.Path).run(), without needing any special integration with the Mill build. Downstream users can still do build-level stuff if they wish, and Mill's own build does so, but it is no longer mandatory and ExampleTester can be used as part of any vanilla test suite.

@lihaoyi
Copy link
Member Author

lihaoyi commented Aug 22, 2024

I tried to break up the testkit into testkit.{unit,integration,example} submodules. Unfortunately there still is some code in integration tests that depend on Mill: it uses mill.eval.Evaluator.Cached and mill.resolve.ParseArgs.apply. These can probably be refactored out at some point if we want to, but probably going to leave that out of this PR.

@lihaoyi
Copy link
Member Author

lihaoyi commented Aug 22, 2024

There's some residual classloader leakage in the test suite; TestEvaluator/UnitTester does not clean up its workers after each test, which seems to cause the JIT to run out of memory and stop JITing after a while. That is an existing problem and not new in this PR

@lihaoyi lihaoyi requested review from lefou and lolgab and removed request for lefou August 22, 2024 06:08
@lihaoyi
Copy link
Member Author

lihaoyi commented Aug 22, 2024

@lefou this should be ready to look at if you have some time.

The most important stuff to review here is the code in testkit/: this contains the standardization and cleanup of the testing infrastructure, and the rest of the code changes in this PR is just adapting to this small set of standardized APIs. These are also the testing APIs that I want to publish for downstream consumption by plugin authors.

I'm planning on adding some examples/documentation for what the "happy path" for someone writing a Mill plugin looks like, using the new APIs. But that can come as a follow up as this PR is big enough already

@lihaoyi lihaoyi marked this pull request as ready for review August 22, 2024 08:57
@lihaoyi lihaoyi requested a review from lefou August 22, 2024 09:01
@lihaoyi lihaoyi added this to the 0.12.0 milestone Aug 22, 2024
@lihaoyi lihaoyi merged commit 8d5f4dd into com-lihaoyi:main Aug 23, 2024
31 checks passed
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.

Is this PR completely dropping support to compile for Scala 2.11, or is this just the test coverage we drop?

Also, one change request below.

Comment on lines -63 to +62
val sv = sys.props.getOrElse("TEST_SCALA_2_13_VERSION", ???)
val sjsv = sys.props.getOrElse("TEST_SCALAJS_VERSION", ???)
val sv = sys.props("TEST_SCALA_2_13_VERSION")
val sjsv = sys.props("TEST_SCALAJS_VERSION")
Copy link
Member

Choose a reason for hiding this comment

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

This change means, we silently go with a null, making any test failure hard to trace back to a missing sysprop. The explicit ??? ensured we'd see the cause, the missing sysprop, early. Sysprops are an easy way to configure the test suite from the build tool, but they can be easily broken, too. Can you revert to the sys.prop.getOrElse notation in all places?

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh I didn't realize sys.props("doesntexist") returns null. I thought it would throw since sys.props is a Map, but I guess there's a default value. I can revert those changes

@lihaoyi
Copy link
Member Author

lihaoyi commented Aug 23, 2024

I just removed testing for scala 2.11 native 0.4. The code didn't change, but those two significantly slowed down the test suite by bloating the test matrix, so I wanted to trim it down where possible. The code may or may not work, but by removing the tests we are stopping the guarantee that it should

@lefou
Copy link
Member

lefou commented Aug 23, 2024

I just removed testing for scala 2.11 native 0.4. The code didn't change, but those two significantly slowed down the test suite by bloating the test matrix, so I wanted to trim it down where possible. The code may or may not work, but by removing the tests we are stopping the guarantee that it should.

We could have just disabled the Scala 2.11 test in the test matrix, but run it occasionally before a release. It's a bit unfortunate to reduce coverage. I always found it a (unique) selling point for Mill, that all our main build chains are covered by tests. As a consequence, we should mark Scala 2.11 as (officially) unsupported, since we no longer have easy ways to detect or reproduce issues.

lihaoyi added a commit that referenced this pull request Sep 14, 2024
* Added `Kotlin_Intro_to_Mill.adoc`,
`Kotlin_Installation_IDE_Support.adoc`, `Kotlin_Builtin_Commands.adoc`
pages
* Fixed an issue with inherited example adoc pages not rendering
correctly since #3398
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.

2 participants