-
-
Notifications
You must be signed in to change notification settings - Fork 366
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
Test suite overhaul #3398
Conversation
I tried to break up the |
There's some residual classloader leakage in the test suite; |
@lefou this should be ready to look at if you have some time. The most important stuff to review here is the code in 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 |
There was a problem hiding this 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.
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") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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. |
* 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
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
, andTestEvaluator
have been moved intomill.testkit
TestEvaluator
has been renamedUnitTester
, andExampleTestSuite
andIntegrationTestSuite
have hadExampleTester
andIntegrationTester
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.There are now
IntegrationTesterTests
andExampleTesterTests
to go along withUnitTesterTests
(previouslyUnitTestTests
), illustrating how to use all these helpers in standalone scenariosUnitTester
ChangesConsolidated most of the
def workspaceTest
boilerplate functions into theUnitTester
classA lot of test code and test data needed to be slightly tweaked to accommodate the now standardized
UnitTester
folder setupUnitTester
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 anos.temp.dir()
without/
as a sub-folderDropped 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 withtest("foo"){
UnitTester#apply
now returns aUnitTester.Result
object, allowing us to fetch fields via.value
and.evalCount
rather than fiddling with tuplesMillTestKit.scala
has been removed, withUnitTester
andTestBaseModule
broken out into their own source files, and all the helper methods to generate nice folder paths are no longer used and have been removedIntegrationTester
ChangesThe various
eval*
methods have been consolidated into a singleval
method that follows OS-Lib's newos.call
APIThe various
meta*
methods have been consolidated into a single.meta("...").*
APIinitWorkspace
no longer returns the workspace path, since it is already available throughIntegrationTester#workspacePath
ExampleTester
ChangesMoved the logic for parsing example test
build.sc
files intomill-build/src/ExampleParser.scala
, so we can share it between Mill's ownbuild.sc
as well as the application code inmill.testkit
via the meta-buildExampleTester
can now be used standalone vianew 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 andExampleTester
can be used as part of any vanilla test suite.