Tweak Maven build gen tests #4079
Conversation
| @@ -125,15 +131,78 @@ object BuildGenTests extends TestSuite { | |||
| .map(PathRef(_)) | |||
| .toSeq | |||
|
|
|||
| def checkFiles(actualFiles: Seq[PathRef], expectedRoot: os.Path): Boolean = { | |||
| val expectedFiles = buildFiles(expectedRoot) | |||
| def checkFiles(actualFiles: Seq[os.SubPath], root: os.Path, expectedRoot: os.Path): Boolean = { | |||
There was a problem hiding this comment.
Could we use git diff --no-index to perform this comparison? That comes with a nice diff viewer for free for people to understand what the difference between the two folders is, and means a lot less comparison code for us to maintain
There was a problem hiding this comment.
Trying that. That output is mainly meant for CI IMO. Locally, one can set updateSnapshots to true, and run git diff manually to check the diff.
| val differentContent = shared.filter { subPath => | ||
| val actual = os.read.lines(root / subPath) | ||
| val expected = os.read.lines(expectedRoot / subPath) | ||
| actual != expected | ||
| } | ||
|
|
||
| val valid = missing.isEmpty && extra.isEmpty && differentContent.isEmpty | ||
|
|
||
| if (!valid) | ||
| if (updateSnapshots) { | ||
| System.err.println( | ||
| s"Expected and actual files differ, updating expected files in resources under $expectedRoot" | ||
| ) | ||
|
|
||
| for (subPath <- missing) { | ||
| val path = expectedRoot / subPath | ||
| System.err.println(s"Removing $subPath") | ||
| os.remove(path) | ||
| } | ||
|
|
||
| for (subPath <- extra) { | ||
| val source = root / subPath | ||
| val dest = expectedRoot / subPath | ||
| System.err.println(s"Creating $subPath") | ||
| os.copy(source, dest, createFolders = true) | ||
| } | ||
|
|
||
| for (subPath <- differentContent) { | ||
| val source = root / subPath | ||
| val dest = expectedRoot / subPath | ||
| System.err.println(s"Updating $subPath") | ||
| os.copy.over(source, dest, createFolders = true) | ||
| } |
There was a problem hiding this comment.
Could valid be computed using git diff as well? If exitCode != 0, then there is a difference between the expected and generated files, and the test should fail.
updateSnapshots seems like it could be implemented by a single coarse-grained os.copy of the entire generated folder to replace the expected folder.
There was a problem hiding this comment.
Just tried that, it seems to work.
I was using manual comparison and piecewise updates for two reasons: run sub-processes can be flaky sometimes, especially on Windows, and removing and copying all files updates last-modified times, which can sometimes lead to unintended side effects (with file watchers for example). But that might overcautious…
There was a problem hiding this comment.
Seems it's a problem on the CI, where we get things like
[11002-1] diff --git a/home/runner/work/mill/mill/main/maven/test/resources/expected/maven-samples/build.mill b/home/runner/work/mill/mill/out/main/maven/test/testCached.dest/mill.main.maven.BuildGenTests/sandbox/maven-samples/build.mill
[11002-1] old mode 100755
[11002-1] new mode 100644
[11002-1] diff --git a/home/runner/work/mill/mill/main/maven/test/resources/expected/maven-samples/multi-module/package.mill b/home/runner/work/mill/mill/out/main/maven/test/testCached.dest/mill.main.maven.BuildGenTests/sandbox/maven-samples/multi-module/package.mill
[11002-1] old mode 100755
[11002-1] new mode 100644
[11002-1] diff --git a/home/runner/work/mill/mill/main/maven/test/resources/expected/maven-samples/multi-module/server/package.mill b/home/runner/work/mill/mill/out/main/maven/test/testCached.dest/mill.main.maven.BuildGenTests/sandbox/maven-samples/multi-module/server/package.mill
[11002-1] old mode 100755
[11002-1] new mode 100644
[11002-1] diff --git a/home/runner/work/mill/mill/main/maven/test/resources/expected/maven-samples/multi-module/webapp/package.mill b/home/runner/work/mill/mill/out/main/maven/test/testCached.dest/mill.main.maven.BuildGenTests/sandbox/maven-samples/multi-module/webapp/package.mill
[11002-1] old mode 100755
[11002-1] new mode 100644
[11002-1] diff --git a/home/runner/work/mill/mill/main/maven/test/resources/expected/maven-samples/single-module/package.mill b/home/runner/work/mill/mill/out/main/maven/test/testCached.dest/mill.main.maven.BuildGenTests/sandbox/maven-samples/single-module/package.mill
[11002-1] old mode 100755
[11002-1] new mode 100644
There was a problem hiding this comment.
How about using os.walk/os.perms.set to normalize the permissions before running git diff? should be a one liner more or less
|
Thanks Alex! |
I had to update the test data of
mill.main.maven.BuildGenTestsfor #4067. The output of those tests didn't really help.This PR changes the helper method checking if generated files match test data in resources, so that it prints more helpful output (removed / added files, diff of modified files), and allows to update the test data on disk by setting
BuildGenTests.updateSnapshotsto true.