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.

Sign up for GitHub

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

Tweak Maven build gen tests #4079

Merged
merged 8 commits into from
Dec 11, 2024
Merged

Tweak Maven build gen tests #4079

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ab1c45d
Tweak Maven build gen tests
alexarchambault Dec 5, 2024
5b8eccc
Use 'git diff --no-index' to print diff between expected / actual
alexarchambault Dec 6, 2024
5a0b32f
Use git --diff / os.copy to check and update test data
alexarchambault Dec 10, 2024
e21e71e
Clean-up
alexarchambault Dec 10, 2024
a339e3c
Try not to change committed test data perms upon checkout
alexarchambault Dec 10, 2024
79672c1
Revert "Try not to change committed test data perms upon checkout"
alexarchambault Dec 10, 2024
e460670
Other attempt
alexarchambault Dec 10, 2024
cbd729d
fixup
alexarchambault Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 59 additions & 9 deletions main/maven/test/src/mill/main/maven/BuildGenTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@ package mill.main.maven

import mill.T
import mill.api.PathRef
import mill.main.client.OutFiles
import mill.scalalib.scalafmt.ScalafmtModule
import mill.testkit.{TestBaseModule, UnitTester}
import utest.*
import utest.framework.TestPath

object BuildGenTests extends TestSuite {

// Change this to true to update test data on disk
def updateSnapshots = false

def tests: Tests = Tests {
val resources = os.Path(sys.env("MILL_TEST_RESOURCE_DIR"))
val scalafmtConfigFile = PathRef(resources / ".scalafmt.conf")
Expand All @@ -33,7 +37,7 @@ object BuildGenTests extends TestSuite {
eval(module.reformat())

// test
checkFiles(files, resources / expectedRel)
checkFiles(files.map(_.path.relativeTo(dest).asSubPath), dest, resources / expectedRel)
}

// multi level nested modules
Expand Down Expand Up @@ -125,15 +129,61 @@ 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 = {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 expectedFiles = buildFiles(expectedRoot).map(_.path.relativeTo(expectedRoot).asSubPath)

val actualFilesSet = actualFiles.toSet
val expectedFilesSet = expectedFiles.toSet

val missing = expectedFiles.filterNot(actualFilesSet)
val extra = actualFiles.filterNot(expectedFilesSet)

actualFiles.nonEmpty &&
actualFiles.length == expectedFiles.length &&
actualFiles.iterator.zip(expectedFiles.iterator).forall {
case (actual, expected) =>
actual.path.endsWith(expected.path.relativeTo(expectedRoot)) &&
os.read.lines(actual.path) == os.read.lines(expected.path)
val shared = actualFiles.filter(expectedFilesSet)

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)
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

How about using os.walk/os.perms.set to normalize the permissions before running git diff? should be a one liner more or less

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right

} else {
// Non *.mill files, that are not in test data, that we don't want
// to see in the diff
val toCleanUp = os.walk(root, skip = _.startsWith(root / OutFiles.defaultOut))
.filter(os.isFile)
.filter(!_.lastOpt.exists(_.endsWith(".mill")))
toCleanUp.foreach(os.remove)
os.proc("git", "diff", "--no-index", expectedRoot, root)
.call(stdin = os.Inherit, stdout = os.Inherit)
}

updateSnapshots || valid
}
}
Loading