From 4b7292430c6c3f7c25972355529edd588cfc28f9 Mon Sep 17 00:00:00 2001 From: Yuta Saito Date: Sun, 19 Oct 2025 04:04:38 +0000 Subject: [PATCH] [PackageToJS] Fix flaky `timestampBasedRebuild` test by abstracting file system operations The mtime-based change detection is not very reliable in tests due to the coarse timestamp resolution of some file systems, especially on artificial file operations in tests that happen in quick succession. This change introduces an abstraction over file system operations in MiniMake, allowing tests to use an in-memory file system with precise control over modification times. https://apenwarr.ca/log/20181113 --- Plugins/PackageToJS/Sources/MiniMake.swift | 51 +++++++++--- Plugins/PackageToJS/Tests/MiniMakeTests.swift | 81 +++++++++++++++++-- 2 files changed, 112 insertions(+), 20 deletions(-) diff --git a/Plugins/PackageToJS/Sources/MiniMake.swift b/Plugins/PackageToJS/Sources/MiniMake.swift index 0004af6c..663635b7 100644 --- a/Plugins/PackageToJS/Sources/MiniMake.swift +++ b/Plugins/PackageToJS/Sources/MiniMake.swift @@ -93,14 +93,18 @@ struct MiniMake { private var tasks: [TaskKey: Task] /// Whether to explain why tasks are built private var shouldExplain: Bool + /// File system operations + private var fileSystem: MiniMakeFileSystem /// Prints progress of the build private var printProgress: ProgressPrinter.PrintProgress init( explain: Bool = false, + fileSystem: MiniMakeFileSystem = DefaultMiniMakeFileSystem(), printProgress: @escaping ProgressPrinter.PrintProgress ) { self.tasks = [:] + self.fileSystem = fileSystem self.shouldExplain = explain self.printProgress = printProgress } @@ -222,7 +226,7 @@ struct MiniMake { /// Cleans all outputs of all tasks func cleanEverything(scope: VariableScope) { for task in self.tasks.values { - try? FileManager.default.removeItem(at: scope.resolve(path: task.output)) + try? fileSystem.removeItem(at: scope.resolve(path: task.output)) } } @@ -234,26 +238,20 @@ struct MiniMake { return true } let outputURL = scope.resolve(path: task.output) - if !FileManager.default.fileExists(atPath: outputURL.path) { + if !fileSystem.fileExists(at: outputURL) { explain("Task \(task.output) should be built because it doesn't exist") return true } - let outputMtime = try? outputURL.resourceValues(forKeys: [.contentModificationDateKey]) - .contentModificationDate + let outputMtime = try? fileSystem.modificationDate(of: outputURL) return task.inputs.contains { input in let inputURL = scope.resolve(path: input) // Ignore directory modification times - var isDirectory: ObjCBool = false - let fileExists = FileManager.default.fileExists( - atPath: inputURL.path, - isDirectory: &isDirectory - ) - if fileExists && isDirectory.boolValue { + let (fileExists, isDirectory) = fileSystem.fileExists(at: inputURL) + if fileExists && isDirectory { return false } - let inputMtime = try? inputURL.resourceValues(forKeys: [.contentModificationDateKey] - ).contentModificationDate + let inputMtime = try? fileSystem.modificationDate(of: inputURL) let shouldBuild = outputMtime == nil || inputMtime == nil || outputMtime! < inputMtime! if shouldBuild { @@ -337,3 +335,32 @@ struct BuildPath: Encodable, Hashable, CustomStringConvertible { try container.encode(self.description) } } + +/// Abstraction over file system operations +protocol MiniMakeFileSystem { + func removeItem(at url: URL) throws + func fileExists(at url: URL) -> Bool + func fileExists(at url: URL) -> (exists: Bool, isDirectory: Bool) + func modificationDate(of url: URL) throws -> Date? +} + +/// Default implementation of MiniMakeFileSystem using FileManager +struct DefaultMiniMakeFileSystem: MiniMakeFileSystem { + func removeItem(at url: URL) throws { + try FileManager.default.removeItem(at: url) + } + + func fileExists(at url: URL) -> Bool { + FileManager.default.fileExists(atPath: url.path) + } + + func fileExists(at url: URL) -> (exists: Bool, isDirectory: Bool) { + var isDirectory: ObjCBool = false + let exists = FileManager.default.fileExists(atPath: url.path, isDirectory: &isDirectory) + return (exists, isDirectory.boolValue) + } + + func modificationDate(of url: URL) throws -> Date? { + try url.resourceValues(forKeys: [.contentModificationDateKey]).contentModificationDate + } +} diff --git a/Plugins/PackageToJS/Tests/MiniMakeTests.swift b/Plugins/PackageToJS/Tests/MiniMakeTests.swift index c0bba29c..140f2880 100644 --- a/Plugins/PackageToJS/Tests/MiniMakeTests.swift +++ b/Plugins/PackageToJS/Tests/MiniMakeTests.swift @@ -4,6 +4,67 @@ import Testing @testable import PackageToJS @Suite struct MiniMakeTests { + final class InMemoryFileSystem: MiniMakeFileSystem { + struct FileEntry { + var content: Data + var modificationDate: Date + var isDirectory: Bool + } + private var storage: [URL: FileEntry] = [:] + + struct MonotonicDateGenerator { + private var currentDate: Date + + init(startingFrom date: Date = Date()) { + self.currentDate = date + } + + mutating func next() -> Date { + currentDate = currentDate.addingTimeInterval(1) + return currentDate + } + } + var dateGenerator = MonotonicDateGenerator() + + // MARK: - MiniMakeFileSystem conformance + + func removeItem(at url: URL) throws { + storage.removeValue(forKey: url) + } + + func fileExists(at url: URL) -> Bool { + return storage[url] != nil + } + + func fileExists(at url: URL) -> (exists: Bool, isDirectory: Bool) { + if let entry = storage[url] { + return (true, entry.isDirectory) + } else { + return (false, false) + } + } + + func modificationDate(of url: URL) throws -> Date? { + return storage[url]?.modificationDate + } + + func writeFile(at url: URL, content: Data) { + storage[url] = FileEntry(content: content, modificationDate: dateGenerator.next(), isDirectory: false) + } + + // MARK: - Helpers for tests + + func touch(_ url: URL) { + let date = dateGenerator.next() + if var entry = storage[url] { + entry.modificationDate = date + storage[url] = entry + } else { + storage[url] = FileEntry(content: Data(), modificationDate: date, isDirectory: false) + } + } + } + // Test basic task management functionality @Test func basicTaskManagement() throws { try withTemporaryDirectory { tempDir, _ in @@ -114,7 +175,11 @@ import Testing // Test that rebuilds are controlled by timestamps @Test func timestampBasedRebuild() throws { try withTemporaryDirectory { tempDir, _ in - var make = MiniMake(printProgress: { _, _ in }) + let fs = InMemoryFileSystem() + var make = MiniMake( + fileSystem: fs, + printProgress: { _, _ in } + ) let prefix = BuildPath(prefix: "PREFIX") let scope = MiniMake.VariableScope(variables: [ "PREFIX": tempDir.path @@ -123,25 +188,25 @@ import Testing let output = prefix.appending(path: "output.txt") var buildCount = 0 - try "Initial".write(toFile: scope.resolve(path: input).path, atomically: true, encoding: .utf8) + // Create initial input file + fs.touch(scope.resolve(path: input)) let task = make.addTask(inputFiles: [input], output: output) { task, scope in buildCount += 1 - let content = try String(contentsOfFile: scope.resolve(path: task.inputs[0]).path, encoding: .utf8) - try content.write(toFile: scope.resolve(path: task.output).path, atomically: true, encoding: .utf8) + fs.touch(scope.resolve(path: task.output)) } // First build - try make.build(output: task, scope: scope) + #expect(throws: Never.self) { try make.build(output: task, scope: scope) } #expect(buildCount == 1, "First build should occur") // Second build without changes - try make.build(output: task, scope: scope) + #expect(throws: Never.self) { try make.build(output: task, scope: scope) } #expect(buildCount == 1, "No rebuild should occur if input is not modified") // Modify input and rebuild - try "Modified".write(toFile: scope.resolve(path: input).path, atomically: true, encoding: .utf8) - try make.build(output: task, scope: scope) + fs.touch(scope.resolve(path: input)) + #expect(throws: Never.self) { try make.build(output: task, scope: scope) } #expect(buildCount == 2, "Should rebuild when input is modified") } }