Skip to content

Commit

Permalink
Make TestModule#test run in T.dest folder by default (#3347)
Browse files Browse the repository at this point in the history
This PR makes it such that `TestModule#test` runs subprocesses inside
`T.dest / "sandbox"` by default, rather than `T.workspace`.

This change can be disabled with a flag `def testSandboxWorkingDir =
false`, but is a good default to encourage:

- Before, because tests run with the `pwd` in the repository root, there
are no guardrails preventing you from doing the "easy" thing and just
reading stuff off of disk rather than having explicit build
dependencies.
- Even though often people put stuff in `resources/` and read from it,
which did the right thing "accidentally" since tests depend on the
resource folder, it was easy for people to put stuff in other
`not-resources/` folders and have the dependency between the test files
and the test task missing.
- This also means things like `--watch` do not properly pick up changes,
and `testCached` does not properly get invalidated.
- It also means that it is easy to accidentally write over shared files,
which becomes problematic when parallelism is turned on (which becomes
the default for `testCached` in
https://github.com/com-lihaoyi/mill/pull/3265/files, and we probably
want to encourage more `testCached` going forward)

- With this change, tests run in empty `T.dest / "sandbox"` folders.
This means that it is very hard to "accidentally" do the wrong thing:
- Although it is still possible to read and write anywhere on disk, it
now becomes something you have to try hard to do, rather than something
that can be done by accident.
- Rather, you should prefer to pass files to tests via `forkEnv` or
`forkArgs`, which is a pattern we already do in Mill's own build (e.g.
[here](https://github.com/com-lihaoyi/mill/blob/33fa9530f165a5e5e1d367a5b51b291c2bb15d1f/build.sc#L684-L689)
and
[here](https://github.com/com-lihaoyi/mill/blob/33fa9530f165a5e5e1d367a5b51b291c2bb15d1f/build.sc#L1189-L1193)).
- This makes an dependencies between tests and their files explicit, so
`--watch` works and `testCached` properly invalidates
- To provide convenience for the common case of use cases where we read
stuff from the resource directories, I added a
`MILL_TEST_RESOURCE_FOLDER` environment variable that contains the
resource folders as a semicolon-separated string. In the common case,
this is a single path, and can be used in your test code to refer to the
resource folder as an alternative to the old practice reading directly
from `os.pwd / "src" / "test" / "resources"` or whatever

There is some design leeway here; we could follow bazel, and create
symlinks for all referenced `PathRef`s inside the test sandbox folder.
Or we could do nothing, and let people pass in the stuff they need
manually via `forkEnv` or `forkArgs`. I think the proposed approach of
providing `MILL_TEST_RESOURCE_FOLDER` by default, and letting people
provide other things on their own, is a reasonable compromise between
these approaches. A full design and implementation of a bazel-style
"Runfiles" system is probably more complexity than I am willing bear at
this moment. This change is nowhere near the sophistication of the bazel
sandbox, but it is a start

This is a binary compatible but semantically breaking change, and should
go into 0.12.0. It will also need extensive updates to the
documentation, which I'll postpone till later since we don't want those
changes to be visible to users until this change has been released
  • Loading branch information
lihaoyi authored Aug 11, 2024
1 parent fc35b16 commit 1cc6f91
Show file tree
Hide file tree
Showing 81 changed files with 276 additions and 30 deletions.
20 changes: 13 additions & 7 deletions build.sc
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ object Settings {
val docBranches = Seq()
// the exact tags containing a doc root
val legacyDocTags: Seq[String] = Seq(
"0.9.12",
"0.10.0",
"0.10.12",
"0.10.15",
"0.11.0-M7"
// "0.9.12",
// "0.10.0",
// "0.10.12",
// "0.10.15",
// "0.11.0-M7"
)
val docTags: Seq[String] = Seq(
"0.11.10",
"0.11.11"
// "0.11.10",
// "0.11.11"
)
val mimaBaseVersions: Seq[String] = 0.to(11).map("0.11." + _)
}
Expand Down Expand Up @@ -1898,6 +1898,12 @@ object docs extends Module {
s"You can browse the local pages at: ${(pages.path / "index.html").toNIO.toUri()}"
)
}
def fastPages = T {
val pages = generatePages(authorMode = true)().apply(Nil)
T.log.outputStream.println(
s"You can browse the local pages at: ${(pages.path / "index.html").toNIO.toUri()}"
)
}

def generatePages(authorMode: Boolean) = T.task { extraSources: Seq[os.Path] =>
T.log.errorStream.println("Creating Antora playbook ...")
Expand Down
24 changes: 14 additions & 10 deletions docs/modules/ROOT/pages/Java_Module_Config.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -30,30 +30,37 @@ include::example/javamodule/3-run-compile-deps.adoc[]

include::example/javamodule/4-test-deps.adoc[]

== Classpath and Filesystem Resources

include::example/javamodule/5-resources.adoc[]

== Annotation Processors

include::example/javamodule/6-annotation-processors.adoc[]

== Javadoc Config

include::example/javamodule/6-docjar.adoc[]
include::example/javamodule/7-docjar.adoc[]

== Unmanaged Jars

include::example/javamodule/7-unmanaged-jars.adoc[]
include::example/javamodule/8-unmanaged-jars.adoc[]

== Specifying the Main Class

include::example/javamodule/8-main-class.adoc[]
include::example/javamodule/9-main-class.adoc[]

== Downloading Non-Maven Jars

include::example/javamodule/9-downloading-non-maven-jars.adoc[]
include::example/javamodule/10-downloading-non-maven-jars.adoc[]

== Customizing the Assembly

include::example/javamodule/10-assembly-config.adoc[]
include::example/javamodule/11-assembly-config.adoc[]

== Repository Config

include::example/javamodule/11-repository-config.adoc[]
include::example/javamodule/12-repository-config.adoc[]

=== Maven Central: Blocked!

Expand Down Expand Up @@ -83,8 +90,5 @@ If you are using millw, a more permanent solution could be to set the environmen
== Native C Code with JNI
include::example/javamodule/12-jni.adoc[]
== Annotation Processors with Lombok
include::example/javamodule/13-jni.adoc[]
include::example/javamodule/13-annotation-processors-lombok.adoc[]
22 changes: 13 additions & 9 deletions docs/modules/ROOT/pages/Scala_Module_Config.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -32,33 +32,37 @@ include::example/scalamodule/3-run-compile-deps.adoc[]

include::example/scalamodule/4-test-deps.adoc[]

== Classpath and Filesystem Resources

include::example/scalamodule/5-resources.adoc[]

== Scala Compiler Plugins

include::example/scalamodule/5-scala-compiler-plugins.adoc[]
include::example/scalamodule/6-scala-compiler-plugins.adoc[]

== Scaladoc Config

include::example/scalamodule/6-docjar.adoc[]
include::example/scalamodule/7-docjar.adoc[]

== Unmanaged Jars

include::example/scalamodule/7-unmanaged-jars.adoc[]
include::example/scalamodule/8-unmanaged-jars.adoc[]

== Specifying the Main Class

include::example/scalamodule/8-main-class.adoc[]
include::example/scalamodule/9-main-class.adoc[]

== Downloading Non-Maven Jars

include::example/scalamodule/9-downloading-non-maven-jars.adoc[]
include::example/scalamodule/10-downloading-non-maven-jars.adoc[]

== Customizing the Assembly

include::example/scalamodule/10-assembly-config.adoc[]
include::example/scalamodule/11-assembly-config.adoc[]

== Repository Config

include::example/scalamodule/11-repository-config.adoc[]
include::example/scalamodule/12-repository-config.adoc[]

== Maven Central: Blocked!

Expand Down Expand Up @@ -86,11 +90,11 @@ If you are using millw, a more permanent solution could be to set the environmen
== Scoverage
include::example/scalamodule/12-contrib-scoverage.adoc[]
include::example/scalamodule/13-contrib-scoverage.adoc[]
== Unidoc
include::example/scalamodule/13-unidoc.adoc[]
include::example/scalamodule/14-unidoc.adoc[]
== Reformatting your code
Expand Down
File renamed without changes.
13 changes: 13 additions & 0 deletions example/javamodule/5-resources/build.sc
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//// SNIPPET:BUILD
import mill._, javalib._

object foo extends JavaModule {
object test extends JavaTests with TestModule.Junit4 {
def otherFiles = T.source(millSourcePath / "other-files")

def forkEnv = super.forkEnv() ++ Map(
"OTHER_FILES_FOLDER" -> otherFiles().path.toString
)
}
}

1 change: 1 addition & 0 deletions example/javamodule/5-resources/foo/resources/file.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Hello World Resource File
15 changes: 15 additions & 0 deletions example/javamodule/5-resources/foo/src/Foo.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package foo;

import java.io.IOException;
import java.io.InputStream;

public class Foo {

// Read `file.txt` from classpath
public static String classpathResourceText() throws IOException {
// Get the resource as an InputStream
try (InputStream inputStream = Foo.class.getClassLoader().getResourceAsStream("file.txt")) {
return new String(inputStream.readAllBytes());
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Other Hello World File
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test Hello World Resource File A
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test Hello World Resource File B
52 changes: 52 additions & 0 deletions example/javamodule/5-resources/foo/test/src/FooTests.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package foo;

import org.junit.Test;
import static org.junit.Assert.*;

import java.io.InputStream;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.nio.file.Path;
import java.util.List;
import java.util.ArrayList;

public class FooTests {

@Test
public void simple() throws IOException {
// Reference app module's `Foo` class which reads `file.txt` from classpath
String appClasspathResourceText = Foo.classpathResourceText();
assertEquals("Hello World Resource File", appClasspathResourceText);

// Read `test-file-a.txt` from classpath
String testClasspathResourceText;
try (InputStream inputStream = Foo.class.getClassLoader().getResourceAsStream("test-file-a.txt")) {
testClasspathResourceText = new String(inputStream.readAllBytes());
}
assertEquals("Test Hello World Resource File A", testClasspathResourceText);

// Use `MILL_TEST_RESOURCE_FOLDER` to read `test-file-b.txt` from filesystem
Path testFileResourceDir = Paths.get(System.getenv("MILL_TEST_RESOURCE_FOLDER"));
String testFileResourceText = new String(
Files.readAllBytes(testFileResourceDir.resolve("test-file-b.txt"))
);
assertEquals("Test Hello World Resource File B", testFileResourceText);

// Use `MILL_TEST_RESOURCE_FOLDER` to list files available in resource folder
List<Path> actualFiles = new ArrayList<>(Files.list(testFileResourceDir).toList());
actualFiles.sort(Path::compareTo);
List<Path> expectedFiles = List.of(
testFileResourceDir.resolve("test-file-a.txt"),
testFileResourceDir.resolve("test-file-b.txt")
);
assertEquals(expectedFiles, actualFiles);

// Use the `OTHER_FILES_FOLDER` configured in your build to access the
// files in `foo/test/other-files/`.
String otherFileText = new String(
Files.readAllBytes(Paths.get(System.getenv("OTHER_FILES_FOLDER"), "other-file.txt"))
);
assertEquals("Other Hello World File", otherFileText);
}
}
File renamed without changes.
File renamed without changes.
File renamed without changes.
1 change: 1 addition & 0 deletions example/scalamodule/4-test-deps/build.sc
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,4 @@ Using BazTestUtils.bazAssertEquals
...
*/

69 changes: 69 additions & 0 deletions example/scalamodule/5-resources/build.sc
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
//// SNIPPET:BUILD
import mill._, scalalib._

object foo extends ScalaModule {
def scalaVersion = "2.13.8"
def ivyDeps = Agg(
ivy"com.lihaoyi::os-lib:0.9.1"
)

object test extends ScalaTests {
def ivyDeps = Agg(ivy"com.lihaoyi::utest:0.8.4")
def testFramework = "utest.runner.Framework"

def otherFiles = T.source(millSourcePath / "other-files")

def forkEnv = super.forkEnv() ++ Map(
"OTHER_FILES_FOLDER" -> otherFiles().path.toString
)
}
}
//// SNIPPET:END

/** Usage
> ./mill foo.test
... foo.FooTests.simple ...
...
*/

// This section discusses how tests can depend on resources locally on disk.
// Mill provides two ways to do this: via the JVM classpath resources, and via
// the resource folder which is made available as the environment variable
// `TEST_MILL_RESOURCE_FOLDER`;
//
// * The *classpath resources* are useful when you want to fetch individual files,
// and are bundled with the application by the `.assembly` step when constructing
// an assembly jar for deployment. But they do not allow you to list folders
// or perform other filesystem operations.
//
// * The *resource folder*, available via `TEST_MILL_RESOURCE_FOLDER`, gives you
// access to the folder path of the resources on disk. This is useful in allowing
// you to list and otherwise manipulate the filesystem, which you cannot do with
// *classpath resources*. However, the `TEST_MILL_RESOURCE_FOLDER` only exists
// when running tests using Mill, and is not available when executing applications
// packaged for deployment via `.assembly`
//
// * Apart from `resources/`, you can provide additional folders to your test suite
// by defining a `T.source` (`otherFiles` above) and passing it to `forkEnv`. This
// provide the folder path as an environment variable that the test can make use of
//
// You can click the *browse* button in the above example to see an example of code
// the uses these three approaches to load files as part of a test module.
//
// Note that tests require that you pass in any files that they depend on explicitly.
// This is necessary so that Mill knows when a test needs to be re-run and when a
// previous result can be cached. This also ensures that tests reading and writing
// to the current working directory do not accidentally interfere with each others
// files, especially when running in parallel.
//
// The test process runs in a `sandbox/` folder, not in your project root folder, to
// prevent you from accidentally accessing files without explicitly passing them. If
// you have legacy tests that need to run in the project root folder to work, you
// can configure your test suite with `def testSandboxWorkingDir = false` to disable
// the sandbox and make the tests run in the project root.




1 change: 1 addition & 0 deletions example/scalamodule/5-resources/foo/resources/file.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Hello World Resource File
6 changes: 6 additions & 0 deletions example/scalamodule/5-resources/foo/src/Foo.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package foo

object Foo {
// Read `file.txt` from classpath
def classpathResourceText = os.read(os.resource / "file.txt")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Other Hello World File
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test Hello World Resource File A
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test Hello World Resource File B
31 changes: 31 additions & 0 deletions example/scalamodule/5-resources/foo/test/src/FooTests.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package foo
import utest._
object FooTests extends TestSuite {
def tests = Tests {
test("simple") {
// Reference app module's `Foo` class which reads `file.txt` from classpath
val appClasspathResourceText = Foo.classpathResourceText
assert(appClasspathResourceText == "Hello World Resource File")

// Read `test-file-a.txt` from classpath
val testClasspathResourceText = os.read(os.resource / "test-file-a.txt")
assert(testClasspathResourceText == "Test Hello World Resource File A")

// Use `MILL_TEST_RESOURCE_FOLDER` to read `test-file-b.txt` from filesystem
val testFileResourceDir = os.Path(sys.env("MILL_TEST_RESOURCE_FOLDER"))
val testFileResourceText = os.read(testFileResourceDir / "test-file-b.txt")
assert(testFileResourceText == "Test Hello World Resource File B")

// Use `MILL_TEST_RESOURCE_FOLDER` to list files available in resource folder
assert(
os.list(testFileResourceDir).sorted ==
Seq(testFileResourceDir / "test-file-a.txt", testFileResourceDir / "test-file-b.txt")
)

// Use the `OTHER_FILES_FOLDER` configured in your build to access the
// files in `foo/test/other-files/`.
val otherFileText = os.read(os.Path(sys.env("OTHER_FILES_FOLDER")) / "other-file.txt")
assert(otherFileText == "Other Hello World File")
}
}
}
File renamed without changes.
File renamed without changes.
1 change: 1 addition & 0 deletions example/thirdparty/commons-io/build.sc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ object commonsio extends RootModule with PublishModule with MavenModule {
)

object test extends MavenTests with TestModule.Junit5 with JmhModule{
def testSandboxWorkingDir = false
def jmhCoreVersion = "1.37"
def ivyDeps = super.ivyDeps() ++ Agg(
ivy"org.junit.jupiter:junit-jupiter:5.10.3",
Expand Down
2 changes: 1 addition & 1 deletion example/thirdparty/netty/build.sc
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ trait NettyBaseModule extends MavenModule{
def javacOptions = Seq("-source", "1.8", "-target", "1.8")
}
trait NettyBaseTestSuiteModule extends NettyBaseModule with TestModule.Junit5{

def testSandboxWorkingDir = false
def testFramework = "com.github.sbt.junit.jupiter.api.JupiterFramework"
def ivyDeps = Agg(
ivy"com.github.sbt.junit:jupiter-interface:0.11.2",
Expand Down
Loading

0 comments on commit 1cc6f91

Please sign in to comment.