refactor test fixtures throw underlying error (#4124)

motivation: test fixtures currently swallow the original error and hide it with XCTFail which does not allow using XCTSkip inside a fixture block

changes:
* update the fixture function to be throwing
* adjust call sites (lots of them!)
* cleanup use of XCTSkip for platform check

Author: tomerd

Sources/SPMTestSupport/misc.swift

@@ -38,7 +38,7 @@ public func fixture(
     file: StaticString = #file,
     line: UInt = #line,
     body: (AbsolutePath) throws -> Void
-) {
+) throws {
     do {
         // Make a suitable test directory name from the fixture subpath.
         let fixtureSubpath = RelativePath(name)
@@ -91,9 +91,7 @@ public func fixture(
         print("**** FAILURE EXECUTING SUBPROCESS ****")
         print("output:", output)
         print("stderr:", stderr)
-        XCTFail("\(error)", file: file, line: line)
-    } catch {
-        XCTFail("\(error)", file: file, line: line)
+        throw error
     }
 }
 

Tests/BasicsTests/URLSessionHTTPClientTests.swift

@@ -209,12 +209,15 @@ final class URLSessionHTTPClientTest: XCTestCase {
 
     // MARK: - download
 
-    // URLSession Download tests can only run on macOS
-    // as re-libs-foundation's URLSessionTask implementation which expects the temporaryFileURL property to be on the request.
-    // and there is no way to set it in a mock
-    // https://github.com/apple/swift-corelibs-foundation/pull/2593 tries to address the latter part
-    #if os(macOS)
+
     func testDownloadSuccess() throws {
+        #if !os(macOS)
+        // URLSession Download tests can only run on macOS
+        // as re-libs-foundation's URLSessionTask implementation which expects the temporaryFileURL property to be on the request.
+        // and there is no way to set it in a mock
+        // https://github.com/apple/swift-corelibs-foundation/pull/2593 tries to address the latter part
+        try XCTSkipIf(true, "test is only supported on macOS")
+        #endif
         let configuration = URLSessionConfiguration.default
         configuration.protocolClasses = [MockURLProtocol.self]
         let urlSession = URLSessionHTTPClient(configuration: configuration)
@@ -270,6 +273,13 @@ final class URLSessionHTTPClientTest: XCTestCase {
     }
 
     func testDownloadAuthenticatedSuccess() throws {
+        #if !os(macOS)
+        // URLSession Download tests can only run on macOS
+        // as re-libs-foundation's URLSessionTask implementation which expects the temporaryFileURL property to be on the request.
+        // and there is no way to set it in a mock
+        // https://github.com/apple/swift-corelibs-foundation/pull/2593 tries to address the latter part
+        try XCTSkipIf(true, "test is only supported on macOS")
+        #endif
         let netrcContent = "machine protected.downloader-tests.com login anonymous password qwerty"
         guard case .success(let netrc) = Netrc.from(netrcContent) else {
             return XCTFail("Cannot load netrc content")
@@ -334,6 +344,13 @@ final class URLSessionHTTPClientTest: XCTestCase {
     }
 
     func testDownloadDefaultAuthenticationSuccess() throws {
+        #if !os(macOS)
+        // URLSession Download tests can only run on macOS
+        // as re-libs-foundation's URLSessionTask implementation which expects the temporaryFileURL property to be on the request.
+        // and there is no way to set it in a mock
+        // https://github.com/apple/swift-corelibs-foundation/pull/2593 tries to address the latter part
+        try XCTSkipIf(true, "test is only supported on macOS")
+        #endif
         let netrcContent = "default login default password default"
         guard case .success(let netrc) = Netrc.from(netrcContent) else {
             return XCTFail("Cannot load netrc content")
@@ -398,6 +415,13 @@ final class URLSessionHTTPClientTest: XCTestCase {
     }
 
     func testDownloadClientError() throws {
+        #if !os(macOS)
+        // URLSession Download tests can only run on macOS
+        // as re-libs-foundation's URLSessionTask implementation which expects the temporaryFileURL property to be on the request.
+        // and there is no way to set it in a mock
+        // https://github.com/apple/swift-corelibs-foundation/pull/2593 tries to address the latter part
+        try XCTSkipIf(true, "test is only supported on macOS")
+        #endif
         let configuration = URLSessionConfiguration.default
         configuration.protocolClasses = [MockURLProtocol.self]
         let urlSession = URLSessionHTTPClient(configuration: configuration)
@@ -453,6 +477,13 @@ final class URLSessionHTTPClientTest: XCTestCase {
     }
 
     func testDownloadServerError() throws {
+        #if !os(macOS)
+        // URLSession Download tests can only run on macOS
+        // as re-libs-foundation's URLSessionTask implementation which expects the temporaryFileURL property to be on the request.
+        // and there is no way to set it in a mock
+        // https://github.com/apple/swift-corelibs-foundation/pull/2593 tries to address the latter part
+        try XCTSkipIf(true, "test is only supported on macOS")
+        #endif
         let configuration = URLSessionConfiguration.default
         configuration.protocolClasses = [MockURLProtocol.self]
         let urlSession = URLSessionHTTPClient(configuration: configuration)
@@ -493,6 +524,13 @@ final class URLSessionHTTPClientTest: XCTestCase {
     }
 
     func testDownloadFileSystemError() throws {
+        #if !os(macOS)
+        // URLSession Download tests can only run on macOS
+        // as re-libs-foundation's URLSessionTask implementation which expects the temporaryFileURL property to be on the request.
+        // and there is no way to set it in a mock
+        // https://github.com/apple/swift-corelibs-foundation/pull/2593 tries to address the latter part
+        try XCTSkipIf(true, "test is only supported on macOS")
+        #endif
         let configuration = URLSessionConfiguration.default
         configuration.protocolClasses = [MockURLProtocol.self]
         let urlSession = URLSessionHTTPClient(configuration: configuration)
@@ -525,7 +563,6 @@ final class URLSessionHTTPClientTest: XCTestCase {
             wait(for: [completionExpectation], timeout: 1.0)
         }
     }
-    #endif
 }
 
 private class MockURLProtocol: URLProtocol {

Tests/BuildTests/IncrementalBuildTests.swift

@@ -35,22 +35,22 @@ import TSCBasic
 ///
 final class IncrementalBuildTests: XCTestCase {
 
-    func testIncrementalSingleModuleCLibraryInSources() {
-        fixture(name: "CFamilyTargets/CLibrarySources") { prefix in
+    func testIncrementalSingleModuleCLibraryInSources() throws {
+        try fixture(name: "CFamilyTargets/CLibrarySources") { fixturePath in
             // Build it once and capture the log (this will be a full build).
-            let (fullLog, _) = try executeSwiftBuild(prefix)
+            let (fullLog, _) = try executeSwiftBuild(fixturePath)
 
             // Check various things that we expect to see in the full build log.
             // FIXME:  This is specific to the format of the log output, which
             // is quite unfortunate but not easily avoidable at the moment.
             XCTAssertMatch(fullLog, .contains("Compiling CLibrarySources Foo.c"))
 
-            let llbuildManifest = prefix.appending(components: ".build", "debug.yaml")
+            let llbuildManifest = fixturePath.appending(components: ".build", "debug.yaml")
 
             // Modify the source file in a way that changes its size so that the low-level
             // build system can detect the change (the timestamp change might be too small
             // for the granularity of the file system to represent as distinct values).
-            let sourceFile = prefix.appending(components: "Sources", "Foo.c")
+            let sourceFile = fixturePath.appending(components: "Sources", "Foo.c")
             let sourceStream = BufferedOutputByteStream()
             sourceStream <<< (try localFileSystem.readFileContents(sourceFile)) <<< "\n"
             try localFileSystem.writeFileContents(sourceFile, bytes: sourceStream.bytes)
@@ -59,15 +59,15 @@ final class IncrementalBuildTests: XCTestCase {
             let llbuildContents1: String = try localFileSystem.readFileContents(llbuildManifest)
 
             // Now build again.  This should be an incremental build.
-            let (log2, _) = try executeSwiftBuild(prefix)
+            let (log2, _) = try executeSwiftBuild(fixturePath)
             XCTAssertMatch(log2, .contains("Compiling CLibrarySources Foo.c"))
 
             // Read the second llbuild manifest.
             let llbuildContents2: String = try localFileSystem.readFileContents(llbuildManifest)
 
             // Now build again without changing anything.  This should be a null
             // build.
-            let (log3, _) = try executeSwiftBuild(prefix)
+            let (log3, _) = try executeSwiftBuild(fixturePath)
             XCTAssertNoMatch(log3, .contains("Compiling CLibrarySources Foo.c"))
 
             // Read the third llbuild manifest.
@@ -79,22 +79,22 @@ final class IncrementalBuildTests: XCTestCase {
             // Modify the header file in a way that changes its size so that the low-level
             // build system can detect the change (the timestamp change might be too small
             // for the granularity of the file system to represent as distinct values).
-            let headerFile = prefix.appending(components: "Sources", "include", "Foo.h")
+            let headerFile = fixturePath.appending(components: "Sources", "include", "Foo.h")
             let headerStream = BufferedOutputByteStream()
             headerStream <<< (try localFileSystem.readFileContents(headerFile)) <<< "\n"
             try localFileSystem.writeFileContents(headerFile, bytes: headerStream.bytes)
 
             // Now build again.  This should be an incremental build.
-            let (log4, _) = try executeSwiftBuild(prefix)
+            let (log4, _) = try executeSwiftBuild(fixturePath)
             XCTAssertMatch(log4, .contains("Compiling CLibrarySources Foo.c"))
         }
     }
 
-    func testBuildManifestCaching() {
-        fixture(name: "ValidLayouts/SingleModule/Library") { prefix in
+    func testBuildManifestCaching() throws {
+        try fixture(name: "ValidLayouts/SingleModule/Library") { fixturePath in
             @discardableResult
             func build() throws -> String {
-                return try executeSwiftBuild(prefix).stdout
+                return try executeSwiftBuild(fixturePath).stdout
             }
 
             // Perform a full build.
@@ -110,7 +110,7 @@ final class IncrementalBuildTests: XCTestCase {
             XCTAssertNoMatch(log3, .contains("Planning build"))
 
             // Check that we do run planning when a new source file is added.
-            let sourceFile = prefix.appending(components: "Sources", "Library", "new.swift")
+            let sourceFile = fixturePath.appending(components: "Sources", "Library", "new.swift")
             try localFileSystem.writeFileContents(sourceFile, bytes: "")
             let log4 = try build()
             XCTAssertMatch(log4, .contains("Compiling Library"))
@@ -123,11 +123,11 @@ final class IncrementalBuildTests: XCTestCase {
         }
     }
 
-    func testDisableBuildManifestCaching() {
-        fixture(name: "ValidLayouts/SingleModule/Library") { prefix in
+    func testDisableBuildManifestCaching() throws {
+        try fixture(name: "ValidLayouts/SingleModule/Library") { fixturePath in
             @discardableResult
             func build() throws -> String {
-                return try executeSwiftBuild(prefix, extraArgs: ["--disable-build-manifest-caching"]).stdout
+                return try executeSwiftBuild(fixturePath, extraArgs: ["--disable-build-manifest-caching"]).stdout
             }
 
             // Perform a full build.