From ab1c45d1039abc06506083be9a9adba849b14ec3 Mon Sep 17 00:00:00 2001 From: Alex Archambault Date: Fri, 6 Dec 2024 00:08:05 +0100 Subject: [PATCH 1/8] Tweak Maven build gen tests --- .../src/mill/main/maven/BuildGenTests.scala | 87 +++++++++++++++++-- 1 file changed, 78 insertions(+), 9 deletions(-) diff --git a/main/maven/test/src/mill/main/maven/BuildGenTests.scala b/main/maven/test/src/mill/main/maven/BuildGenTests.scala index 5f0ebac94e6..d615f242de0 100644 --- a/main/maven/test/src/mill/main/maven/BuildGenTests.scala +++ b/main/maven/test/src/mill/main/maven/BuildGenTests.scala @@ -1,5 +1,6 @@ package mill.main.maven +import com.github.difflib.{DiffUtils, UnifiedDiffUtils} import mill.T import mill.api.PathRef import mill.scalalib.scalafmt.ScalafmtModule @@ -7,8 +8,13 @@ import mill.testkit.{TestBaseModule, UnitTester} import utest.* import utest.framework.TestPath +import scala.jdk.CollectionConverters._ + 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") @@ -33,7 +39,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 @@ -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 = { + val expectedFiles = buildFiles(expectedRoot).map(_.path.relativeTo(expectedRoot).asSubPath) + + val actualFilesSet = actualFiles.toSet + val expectedFilesSet = expectedFiles.toSet - 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 missing = expectedFiles.filterNot(actualFilesSet) + val extra = actualFiles.filterNot(expectedFilesSet) + + 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) + } + } else { + for (subPath <- missing) + System.err.println(s"Not found: $subPath") + for (subPath <- extra) + System.err.println(s"Found extra file: $subPath") + + for (subPath <- differentContent) { + val actual = os.read.lines(root / subPath) + val expected = os.read.lines(expectedRoot / subPath) + val patch = DiffUtils.diff(expected.asJava, actual.asJava) + val printablePatch = UnifiedDiffUtils.generateUnifiedDiff( + s"expected / $subPath", + s"obtained / $subPath", + expected.asJava, + patch, + 3 + ) + for (line <- printablePatch.asScala) { + val color = + if (line.startsWith("+")) Console.GREEN + else if (line.startsWith("-")) Console.RED + else if (line.startsWith("@")) Console.CYAN + else "" + System.err.println(color + line + Console.RESET) + } + } + } + + updateSnapshots || valid } } From 5b8ecccfed406b066cb5a596299d6af7aa8954a5 Mon Sep 17 00:00:00 2001 From: Alex Archambault Date: Fri, 6 Dec 2024 12:16:40 +0100 Subject: [PATCH 2/8] Use 'git diff --no-index' to print diff between expected / actual --- .../src/mill/main/maven/BuildGenTests.scala | 37 +++++-------------- 1 file changed, 9 insertions(+), 28 deletions(-) diff --git a/main/maven/test/src/mill/main/maven/BuildGenTests.scala b/main/maven/test/src/mill/main/maven/BuildGenTests.scala index d615f242de0..856652ea755 100644 --- a/main/maven/test/src/mill/main/maven/BuildGenTests.scala +++ b/main/maven/test/src/mill/main/maven/BuildGenTests.scala @@ -1,15 +1,13 @@ package mill.main.maven -import com.github.difflib.{DiffUtils, UnifiedDiffUtils} 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 -import scala.jdk.CollectionConverters._ - object BuildGenTests extends TestSuite { // Change this to true to update test data on disk @@ -176,31 +174,14 @@ object BuildGenTests extends TestSuite { os.copy.over(source, dest, createFolders = true) } } else { - for (subPath <- missing) - System.err.println(s"Not found: $subPath") - for (subPath <- extra) - System.err.println(s"Found extra file: $subPath") - - for (subPath <- differentContent) { - val actual = os.read.lines(root / subPath) - val expected = os.read.lines(expectedRoot / subPath) - val patch = DiffUtils.diff(expected.asJava, actual.asJava) - val printablePatch = UnifiedDiffUtils.generateUnifiedDiff( - s"expected / $subPath", - s"obtained / $subPath", - expected.asJava, - patch, - 3 - ) - for (line <- printablePatch.asScala) { - val color = - if (line.startsWith("+")) Console.GREEN - else if (line.startsWith("-")) Console.RED - else if (line.startsWith("@")) Console.CYAN - else "" - System.err.println(color + line + Console.RESET) - } - } + // 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 From 5a0b32f72d004a8e4a67779e786a2fd31f0d7cc8 Mon Sep 17 00:00:00 2001 From: Alex Archambault Date: Tue, 10 Dec 2024 12:21:48 +0100 Subject: [PATCH 3/8] Use git --diff / os.copy to check and update test data --- .../src/mill/main/maven/BuildGenTests.scala | 62 ++++++------------- 1 file changed, 19 insertions(+), 43 deletions(-) diff --git a/main/maven/test/src/mill/main/maven/BuildGenTests.scala b/main/maven/test/src/mill/main/maven/BuildGenTests.scala index 856652ea755..560a63e151b 100644 --- a/main/maven/test/src/mill/main/maven/BuildGenTests.scala +++ b/main/maven/test/src/mill/main/maven/BuildGenTests.scala @@ -140,50 +140,26 @@ object BuildGenTests extends TestSuite { 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" - ) + // 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) + + val diffExitCode = os.proc("git", "diff", "--no-index", expectedRoot, root) + .call(stdin = os.Inherit, stdout = os.Inherit, check = !updateSnapshots) + .exitCode + + if (updateSnapshots && diffExitCode != 0) { + 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) - } - } 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) - } + os.remove.all(expectedRoot) + os.copy(root, expectedRoot) + } - updateSnapshots || valid + diffExitCode == 0 || updateSnapshots } } From e21e71e1cd8980a2f61ee640f96a3b5b0acad813 Mon Sep 17 00:00:00 2001 From: Alex Archambault Date: Tue, 10 Dec 2024 14:28:19 +0100 Subject: [PATCH 4/8] Clean-up --- .../maven/test/src/mill/main/maven/BuildGenTests.scala | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/main/maven/test/src/mill/main/maven/BuildGenTests.scala b/main/maven/test/src/mill/main/maven/BuildGenTests.scala index 560a63e151b..601e0742f01 100644 --- a/main/maven/test/src/mill/main/maven/BuildGenTests.scala +++ b/main/maven/test/src/mill/main/maven/BuildGenTests.scala @@ -130,16 +130,6 @@ object BuildGenTests extends TestSuite { .toSeq def checkFiles(actualFiles: Seq[os.SubPath], root: os.Path, expectedRoot: os.Path): Boolean = { - 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) - - val shared = actualFiles.filter(expectedFilesSet) - // 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)) From a339e3c774e3e84eec1b970a17fcfd021848c329 Mon Sep 17 00:00:00 2001 From: Alex Archambault Date: Tue, 10 Dec 2024 14:28:29 +0100 Subject: [PATCH 5/8] Try not to change committed test data perms upon checkout --- .github/workflows/publish-artifacts.yml | 6 +++--- .github/workflows/run-mill-action.yml | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/publish-artifacts.yml b/.github/workflows/publish-artifacts.yml index 9f5fd986380..5d6348c192a 100644 --- a/.github/workflows/publish-artifacts.yml +++ b/.github/workflows/publish-artifacts.yml @@ -41,7 +41,7 @@ jobs: - uses: actions/upload-artifact@v4.4.3 with: - path: . + path: out include-hidden-files: true name: publish-artifacts @@ -67,7 +67,7 @@ jobs: steps: - uses: actions/download-artifact@v4 with: - path: . + path: out name: publish-artifacts - run: ls -la . @@ -98,7 +98,7 @@ jobs: steps: - uses: actions/download-artifact@v4 with: - path: . + path: out name: publish-artifacts - run: ls -la . diff --git a/.github/workflows/run-mill-action.yml b/.github/workflows/run-mill-action.yml index f77fd86ad93..9e0b8992d3c 100644 --- a/.github/workflows/run-mill-action.yml +++ b/.github/workflows/run-mill-action.yml @@ -46,7 +46,7 @@ jobs: - uses: actions/download-artifact@v4 if: ${{ !inputs.populate_cache }} with: - path: . + path: out name: ${{ inputs.os }}-artifact # Need to fix cached artifact file permissions because github actions screws it up @@ -96,7 +96,7 @@ jobs: - uses: actions/upload-artifact@v4.4.3 with: - path: . + path: out name: ${{ inputs.os }}-artifact include-hidden-files: true if: ${{ inputs.populate_cache }} From 79672c1aaec5be73a8dfbbd78c4b051d7306e1df Mon Sep 17 00:00:00 2001 From: Alex Archambault Date: Tue, 10 Dec 2024 14:34:44 +0100 Subject: [PATCH 6/8] Revert "Try not to change committed test data perms upon checkout" This reverts commit a339e3c774e3e84eec1b970a17fcfd021848c329. --- .github/workflows/publish-artifacts.yml | 6 +++--- .github/workflows/run-mill-action.yml | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/publish-artifacts.yml b/.github/workflows/publish-artifacts.yml index 5d6348c192a..9f5fd986380 100644 --- a/.github/workflows/publish-artifacts.yml +++ b/.github/workflows/publish-artifacts.yml @@ -41,7 +41,7 @@ jobs: - uses: actions/upload-artifact@v4.4.3 with: - path: out + path: . include-hidden-files: true name: publish-artifacts @@ -67,7 +67,7 @@ jobs: steps: - uses: actions/download-artifact@v4 with: - path: out + path: . name: publish-artifacts - run: ls -la . @@ -98,7 +98,7 @@ jobs: steps: - uses: actions/download-artifact@v4 with: - path: out + path: . name: publish-artifacts - run: ls -la . diff --git a/.github/workflows/run-mill-action.yml b/.github/workflows/run-mill-action.yml index 9e0b8992d3c..f77fd86ad93 100644 --- a/.github/workflows/run-mill-action.yml +++ b/.github/workflows/run-mill-action.yml @@ -46,7 +46,7 @@ jobs: - uses: actions/download-artifact@v4 if: ${{ !inputs.populate_cache }} with: - path: out + path: . name: ${{ inputs.os }}-artifact # Need to fix cached artifact file permissions because github actions screws it up @@ -96,7 +96,7 @@ jobs: - uses: actions/upload-artifact@v4.4.3 with: - path: out + path: . name: ${{ inputs.os }}-artifact include-hidden-files: true if: ${{ inputs.populate_cache }} From e460670300bc4b44150de1c848a294440d64b252 Mon Sep 17 00:00:00 2001 From: Alex Archambault Date: Tue, 10 Dec 2024 14:34:25 +0100 Subject: [PATCH 7/8] Other attempt --- .../maven/test/src/mill/main/maven/BuildGenTests.scala | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/main/maven/test/src/mill/main/maven/BuildGenTests.scala b/main/maven/test/src/mill/main/maven/BuildGenTests.scala index 601e0742f01..9d3eff8c50a 100644 --- a/main/maven/test/src/mill/main/maven/BuildGenTests.scala +++ b/main/maven/test/src/mill/main/maven/BuildGenTests.scala @@ -8,6 +8,8 @@ import mill.testkit.{TestBaseModule, UnitTester} import utest.* import utest.framework.TestPath +import java.nio.file.FileSystems + object BuildGenTests extends TestSuite { // Change this to true to update test data on disk @@ -137,6 +139,14 @@ object BuildGenTests extends TestSuite { .filter(!_.lastOpt.exists(_.endsWith(".mill"))) toCleanUp.foreach(os.remove) + // Try to normalize permissions while not touching those of committed test data + val supportsPerms = FileSystems.getDefault.supportedFileAttributeViews().contains("posix") + if (supportsPerms) + for (testFile <- os.walk(expectedRoot) if os.isFile(testFile)) { + val subPath = testFile.relativeTo(expectedRoot).asSubPath + os.perms.set(root / subPath, os.perms(testFile)) + } + val diffExitCode = os.proc("git", "diff", "--no-index", expectedRoot, root) .call(stdin = os.Inherit, stdout = os.Inherit, check = !updateSnapshots) .exitCode From cbd729d583d577b28d25d3cf5fc2a7ef2fd11726 Mon Sep 17 00:00:00 2001 From: Alex Archambault Date: Tue, 10 Dec 2024 14:37:29 +0100 Subject: [PATCH 8/8] fixup --- main/maven/test/src/mill/main/maven/BuildGenTests.scala | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/main/maven/test/src/mill/main/maven/BuildGenTests.scala b/main/maven/test/src/mill/main/maven/BuildGenTests.scala index 9d3eff8c50a..f95e59089f3 100644 --- a/main/maven/test/src/mill/main/maven/BuildGenTests.scala +++ b/main/maven/test/src/mill/main/maven/BuildGenTests.scala @@ -142,10 +142,13 @@ object BuildGenTests extends TestSuite { // Try to normalize permissions while not touching those of committed test data val supportsPerms = FileSystems.getDefault.supportedFileAttributeViews().contains("posix") if (supportsPerms) - for (testFile <- os.walk(expectedRoot) if os.isFile(testFile)) { - val subPath = testFile.relativeTo(expectedRoot).asSubPath - os.perms.set(root / subPath, os.perms(testFile)) + for { + testFile <- os.walk(expectedRoot) + if os.isFile(testFile) + targetFile = root / testFile.relativeTo(expectedRoot).asSubPath + if os.isFile(targetFile) } + os.perms.set(targetFile, os.perms(testFile)) val diffExitCode = os.proc("git", "diff", "--no-index", expectedRoot, root) .call(stdin = os.Inherit, stdout = os.Inherit, check = !updateSnapshots)