Allow skipping tests with a flag, instead of the environment variable

Author: finagolfin

Documentation/Development.md

@@ -47,6 +47,9 @@ $ swift test --parallel
 
 # Run a single test.
 $ swift test --filter PackageGraphTests.DependencyResolverTests/testBasics
+
+# Run tests for the test targets BuildTests and WorkspaceTests, but skip some test cases.
+$ swift test --filter BuildTests --skip BuildPlanTests --filter WorkspaceTests --skip InitTests
 ```
 
 Note: PackageDescription v4 is not available when developing using this method.
@@ -184,14 +187,6 @@ absolute search paths. SwiftPM will choose the first
 path which exists on disk. If none of the paths are present on disk, it will fall
 back to built-in computation.
 
-## Skipping SwiftPM tests
-
-SwiftPM has a hidden env variable `_SWIFTPM_SKIP_TESTS_LIST` that can be used
-to skip a list of tests. This value of the variable is either a file path that contains a
-newline separated list of tests to skip, or a colon-separated list of tests.
-
-This is only a development feature and should be considered _unsupported_.
-
 ## Making changes in TSC targets
 
 All targets with the prefix TSC define the interface for the tools support core. Those APIs might be used in other projects as well and need to be updated in this repository by copying their sources directories to the TSC repository. The repository can be found [here](https://github.com/apple/swift-tools-support-core).

IntegrationTests/Tests/IntegrationTests/BasicTests.swift

@@ -222,13 +222,13 @@ final class BasicTests: XCTestCase {
                         func testBaz() { }
                     }
                     """))
-            let testOutput = try sh(swiftTest, "--package-path", toolDir, "--filter", "MyTests.*").stderr
+            let testOutput = try sh(swiftTest, "--package-path", toolDir, "--filter", "MyTests.*", "--skip", "testBaz").stderr
 
             // Check the test log.
             XCTAssertContents(testOutput) { checker in
                 checker.check(.contains("Test Suite 'MyTests' started"))
                 checker.check(.contains("Test Suite 'MyTests' passed"))
-                checker.check(.contains("Executed 3 tests, with 0 failures"))
+                checker.check(.contains("Executed 2 tests, with 0 failures"))
             }
         }
     }

Sources/Commands/SwiftTestTool.swift

@@ -83,50 +83,25 @@ public class TestToolOptions: ToolOptions {
     /// If the path of the exported code coverage JSON should be printed.
     var shouldPrintCodeCovPath = false
 
-    var testCaseSpecifier: TestCaseSpecifier {
-        testCaseSpecifierOverride() ?? _testCaseSpecifier
-    }
+    var testCaseSpecifier: TestCaseSpecifier = .none
 
-    var _testCaseSpecifier: TestCaseSpecifier = .none
+    var testCaseSkip: TestCaseSpecifier = .none
 
     /// Path where the xUnit xml file should be generated.
     var xUnitOutput: AbsolutePath?
 
     /// The test product to use. This is useful when there are multiple test products
     /// to choose from (usually in multiroot packages).
     public var testProduct: String?
-
-    /// Returns the test case specifier if overridden in the env.
-    private func testCaseSpecifierOverride() -> TestCaseSpecifier? {
-        guard let override = ProcessEnv.vars["_SWIFTPM_SKIP_TESTS_LIST"] else {
-            return nil
-        }
-
-        do {
-            let skipTests: [String.SubSequence]
-            // Read from the file if it exists.
-            if let path = try? AbsolutePath(validating: override), localFileSystem.exists(path) {
-                let contents = try localFileSystem.readFileContents(path).cString
-                skipTests = contents.split(separator: "\n", omittingEmptySubsequences: true)
-            } else {
-                // Otherwise, read the env variable.
-                skipTests = override.split(separator: ":", omittingEmptySubsequences: true)
-            }
-
-            return .skip(skipTests.map(String.init))
-        } catch {
-            // FIXME: We should surface errors from here.
-        }
-        return nil
-    }
 }
 
 /// Tests filtering specifier
 ///
 /// This is used to filter tests to run
 ///   .none     => No filtering
 ///   .specific => Specify test with fully quantified name
-///   .regex    => RegEx patterns
+///   .regex    => RegEx patterns for tests to run
+///   .skip     => RegEx patterns for tests to skip
 public enum TestCaseSpecifier {
     case none
     case specific(String)
@@ -168,7 +143,9 @@ public class SwiftTestTool: SwiftTool<TestToolOptions> {
         case .listTests:
             let testProducts = try buildTestsIfNeeded()
             let testSuites = try getTestSuites(in: testProducts)
-            let tests = testSuites.filteredTests(specifier: options.testCaseSpecifier)
+            let tests = testSuites
+                .filteredTests(specifier: options.testCaseSpecifier)
+                .skippedTests(specifier: options.testCaseSkip)
 
             // Print the tests.
             for test in tests {
@@ -218,7 +195,9 @@ public class SwiftTestTool: SwiftTool<TestToolOptions> {
 
                 // Find the tests we need to run.
                 let testSuites = try getTestSuites(in: testProducts)
-                let tests = testSuites.filteredTests(specifier: options.testCaseSpecifier)
+                let tests = testSuites
+                    .filteredTests(specifier: options.testCaseSpecifier)
+                    .skippedTests(specifier: options.testCaseSkip)
 
                 // If there were no matches, emit a warning.
                 if tests.isEmpty {
@@ -252,7 +231,9 @@ public class SwiftTestTool: SwiftTool<TestToolOptions> {
             let toolchain = try getToolchain()
             let testProducts = try buildTestsIfNeeded()
             let testSuites = try getTestSuites(in: testProducts)
-            let tests = testSuites.filteredTests(specifier: options.testCaseSpecifier)
+            let tests = testSuites
+                .filteredTests(specifier: options.testCaseSpecifier)
+                .skippedTests(specifier: options.testCaseSkip)
             let buildParameters = try self.buildParameters()
 
             // If there were no matches, emit a warning and exit.
@@ -408,7 +389,7 @@ public class SwiftTestTool: SwiftTool<TestToolOptions> {
 
         binder.bind(
             option: parser.add(option: "--specifier", shortName: "-s", kind: String.self),
-            to: { $0._testCaseSpecifier = .specific($1) })
+            to: { $0.testCaseSpecifier = .specific($1) })
 
         binder.bind(
             option: parser.add(option: "--xunit-output", kind: PathArgument.self),
@@ -418,7 +399,12 @@ public class SwiftTestTool: SwiftTool<TestToolOptions> {
             option: parser.add(option: "--filter", kind: [String].self,
                 usage: "Run test cases matching regular expression, Format: <test-target>.<test-case> or " +
                     "<test-target>.<test-case>/<test>"),
-            to: { $0._testCaseSpecifier = .regex($1) })
+            to: { $0.testCaseSpecifier = .regex($1) })
+
+        binder.bind(
+            option: parser.add(option: "--skip", kind: [String].self,
+                usage: "Skip test cases matching regular expression, Example: --skip PerformanceTests"),
+            to: { $0.testCaseSkip = .skip($1) })
 
         binder.bind(
             option: parser.add(option: "--enable-code-coverage", kind: Bool.self,
@@ -967,14 +953,28 @@ fileprivate extension Dictionary where Key == AbsolutePath, Value == [TestSuite]
             })
         case .specific(let name):
             return allTests.filter{ $0.specifier == name }
+        case .skip:
+            fatalError("Tests to skip should never have been passed here.")
+        }
+    }
+}
+
+fileprivate extension Array where Element == UnitTest {
+    /// Skip tests matching the provided specifier
+    func skippedTests(specifier: TestCaseSpecifier) -> [UnitTest] {
+        switch specifier {
+        case .none:
+            return self
         case .skip(let skippedTests):
-            var result = allTests
+            var result = self
             for skippedTest in skippedTests {
                 result = result.filter{
                     $0.specifier.range(of: skippedTest, options: .regularExpression) == nil
                 }
             }
             return result
+        case .regex, .specific:
+            fatalError("Tests to filter should never have been passed here.")
         }
     }
 }
@@ -1086,6 +1086,6 @@ final class XUnitGenerator {
 
 private extension Diagnostic.Message {
     static var noMatchingTests: Diagnostic.Message {
-        .warning("'--filter' predicate did not match any test case")
+        .warning("No matching test cases were run")
     }
 }

Tests/FunctionalTests/MiscellaneousTests.swift

@@ -274,6 +274,22 @@ class MiscellaneousTestCase: XCTestCase {
         #endif
     }
 
+    func testSwiftTestSkip() throws {
+            fixture(name: "Miscellaneous/ParallelTestsPkg") { prefix in
+                let (stdout, _) = try SwiftPMProduct.SwiftTest.execute(["--skip", "ParallelTestsTests", "-l", "--enable-test-discovery"], packagePath: prefix)
+                XCTAssertNoMatch(stdout, .contains("testExample1"))
+                XCTAssertNoMatch(stdout, .contains("testExample2"))
+                XCTAssertMatch(stdout, .contains("testSureFailure"))
+            }
+
+            fixture(name: "Miscellaneous/ParallelTestsPkg") { prefix in
+                let (stdout, _) = try SwiftPMProduct.SwiftTest.execute(["--filter", "ParallelTestsTests", "--skip", ".*2", "--filter", "TestsFailure", "--skip", "testSureFailure", "-l", "--enable-test-discovery"], packagePath: prefix)
+                XCTAssertMatch(stdout, .contains("testExample1"))
+                XCTAssertNoMatch(stdout, .contains("testExample2"))
+                XCTAssertNoMatch(stdout, .contains("testSureFailure"))
+            }
+    }
+
     func testOverridingSwiftcArguments() throws {
       #if os(macOS)
         fixture(name: "Miscellaneous/OverrideSwiftcArgs") { prefix in