Migrate PluginTests to Swift Testing #8793

cmcgee1024 · 2025-06-09T19:58:34Z

Migrate the test suite to Swift Testing.

Explicitly mark tests that are enabled/disabled.

Add bug links to issues and radars for better tracking combined
withKnownIssues so that when bugs are fixed there is immediate
notification that the tests can be enabled.

cmcgee1024 · 2025-06-09T19:58:44Z

@swift-ci please test

bkhouri

There are still references to custom XCTAssertion calls, which mixes XCTest and Swift Testing. Not to mention I believe the use of #file and #line should be replaced with #_sourceLocation based on the some Swift Testing trait documentation: https://developer.apple.com/documentation/testing/trait/enabled(if:_:sourcelocation:).

Tests/FunctionalTests/PluginTests.swift

bkhouri · 2025-06-09T20:24:18Z

Tests/FunctionalTests/PluginTests.swift

+        try await withKnownIssue {
+            // Try again with the Swift Build build system
+            try await fixture(name: "Miscellaneous/Plugins") { fixturePath in
+                let (stdout, _) = try await executeSwiftBuild(fixturePath.appending("MySourceGenPlugin"), configuration: .Debug, extraArgs: ["--vv", "--product", "MyLocalTool", "--build-system", "swiftbuild"])


suggestion: instead of creating a separate code block in. the same "test case", consider using parameterized tests.

See #8714 as an example.

This comment applies to all applicable tests in suite.

As far as I'm aware the parameterized tests don't permit skipping certain parameter combinations, or have separate parts that have different known issue when conditions. I expect there to be too many parameters to make the parameterization worthwhile.

The withKnownIssue can accept a when: argument.

e.g.:

withKnownIssue { // test code } when: { buildSystem == .swiftbuild }

See the known issues documentation.

Regarding skipping: It is my opinion that a test should be skipped if, and only if, the test is not applicable. Otherwise, we ought to use withKnownIssue to report the currently failing behaviour.

bkhouri · 2025-06-09T20:29:46Z

Tests/FunctionalTests/PluginTests.swift

    func testInternalExecutableAvailableOnlyToPlugin() async throws {
-        // Only run the test if the environment in which we're running actually supports Swift concurrency (which the plugin APIs require).
-        try XCTSkipIf(!UserToolchain.default.supportsSwiftConcurrency(), "skipping because test environment doesn't support concurrency")
-
        try await fixture(name: "Miscellaneous/Plugins") { fixturePath in
            await XCTAssertThrowsCommandExecutionError(try await executeSwiftBuild(fixturePath.appending("InvalidUseOfInternalPluginExecutable")), "Illegally used internal executable") { error in


Issue: (blocking): This test has a mixture of XCTest and Swift Testing. We need convert this to Swift Testing.

Thanks, do you know if anyone has make equivalents for Swift Testing for these utilities in _InternalTestSupport? There's significant logic in these utility methods, and not entirely obvious how to convert them to the new testing API.

Have a look at BuildCommandTests.swift here for a possible conversion option:

https://github.com/swiftlang/swift-package-manager/pull/8714/files#diff-b1845cad8f64e37500b06818ff71629726c14cf5a8d34add6ef3a9d38236cfe5

In addition, has a look a the Record issues in the Migrate a test from XCTest.

Tests/FunctionalTests/PluginTests.swift

bkhouri · 2025-06-09T20:54:35Z

Tests/FunctionalTests/PluginTests.swift

                    }
                }
                catch {
-                    XCTFail("error \(String(describing: error))", file: file, line: line)
+                    try #require(true == false, "error \(String(describing: error)) at \(file) and \(line)")


issue: We should use `Issue.record("error (String(describing: error)) at (sourceLocation)")" here instead

Thanks, I couldn't find the API to raise an issue directly.

Tests/FunctionalTests/PluginTests.swift

bkhouri · 2025-06-09T21:09:13Z

Tests/FunctionalTests/PluginTests.swift

@@ -945,15 +976,16 @@ final class PluginTests: XCTestCase {
                delegateQueue.sync {
                    pid = delegate.parsedProcessIdentifier
                }
-                guard let pid else {
-                    throw XCTSkip("skipping test because no pid was received from the plugin; being investigated as rdar://88792829\n\(delegate.diagnostics.description)")
+                guard let pid = pid else {


issue (possibly-blocking): if we want to fail the test, we should invoke try #require(pid, "Test failed because no pid was received from the plugin; being investigated as rdar://88792829\n\(delegate.diagnostics.description)")

Can we add .bug("rdar://88792829") trait to the test, and preferably reference a GitHub issue.

The code that's written here before appears to want to skip the test in middle in this case, not fail it. I'm not sure how to do that with Swift Test API, other than to just return. How would one print this information out without just printing it?

@grynspan or @stmontgomery might have ideas on how to handle this.

Consider using withKnownIssue and use try #require in its body closure to unwrap the pid and use it in the subsequent code. You could use a matching: closure on withKnownIssue to only match the specific expectation which is attempting to unwrap pid.

Tests/FunctionalTests/PluginTests.swift

…ager into plugin_testing_rework

bkhouri · 2025-06-10T14:52:05Z

@swift-ci test windows

…he plugin output directory

…uivalents

cmcgee1024 · 2025-06-11T19:45:34Z

@swift-ci please test

…t only

cmcgee1024 · 2025-06-12T15:25:04Z

@swift-ci please test

cmcgee1024 · 2025-06-12T15:28:35Z

@swift-ci test Windows

bkhouri

Nothing blocking, however there are changes that should be addressed. Addressing them in a follow-up PR is acceptable, unless the branch will be updated.

bkhouri · 2025-06-12T18:29:57Z

Tests/FunctionalTests/PluginTests.swift

-            XCTAssert(stdout.contains("Linking MyOtherLocalTool"), "stdout:\n\(stdout)")
-            XCTAssert(stdout.contains("Build of product 'MyOtherLocalTool' complete!"), "stdout:\n\(stdout)")
-        }
+        try await withKnownIssue {


nit (non-blocking, if-updating-pr): Can we wrap the withKnownIssue around each build system? This should give more fine grained indication if this test will be supported on windows for native and SwiftBuild build system.

This comment applies to all applicable test cases.

bkhouri · 2025-06-12T18:32:16Z

Tests/FunctionalTests/PluginTests.swift

        }

        try await fixture(name: "Miscellaneous/Plugins") { fixturePath in
-            await XCTAssertThrowsCommandExecutionError(try await executeSwiftBuild(fixturePath.appending("InvalidUseOfInternalPluginExecutable")), "Illegally used internal executable"
-) { error in
+            let error =  try await #require(throws: SwiftPMError.self, "Illegally used internal executable") {


question: Are we able to add an expectation to verify we failed in the expected way?

This is just attempting to faithfully duplicate the logic from the XCTest function here. It's expecting that the SwiftPMError is thrown, and that the error contains certain details taken from the XCTAssertThrowsCommandExecutionError.

bkhouri · 2025-06-12T18:34:13Z

Tests/FunctionalTests/PluginTests.swift

-            await XCTAssertThrowsCommandExecutionError(try await executeSwiftBuild(fixturePath.appending("InvalidUseOfInternalPluginExecutable")), "Illegally used internal executable"
-) { error in
+            let error =  try await #require(throws: SwiftPMError.self, "Illegally used internal executable") {
+                try await executeSwiftBuild(fixturePath.appending("InvalidUseOfInternalPluginExecutable"))


issue (possibly-blocking): This running the same action as lie 162. Did we intend to build this using SwiftBuild build system? If so, can we update this test? It's ok to do this in a follow-up PR.

This test already had these two cases, one that checks stderr's contents. It didn't cover swiftbuild build system. This is just faithfully converting it to Swift Testing as-is.

bkhouri · 2025-06-12T18:36:28Z

Tests/FunctionalTests/PluginTests.swift

+            try await withKnownIssue {
+                // Try again with the Swift Build build system
+                try await fixture(name: "Miscellaneous/Plugins") { fixturePath in
+                    let (stdout, _) = try await executeSwiftBuild(fixturePath.appending("MyBuildToolPluginDependencies"), extraArgs: ["--vv", "--build-system", "swiftbuild"])


question: does this test now need the --vv argument?

No, it's just that without the extra verbose logging, when the test does fail it doesn't yield very useful information for things like linking failures.

I've removed the verbose logging so that it doesn't fill up the logs. With linker errors, one will need a reproducing environment to be able to exfiltrate the linker arguments.

bkhouri · 2025-06-12T18:50:39Z

Tests/FunctionalTests/PluginTests.swift

@@ -643,62 +698,63 @@ final class PluginTests: XCTestCase {
                      )
                    }
                    if expectFailure {
-                        XCTAssertFalse(success, "expected command to fail, but it succeeded", file: file, line: line)
+                        #expect(!success, "expected command to fail, but it succeeded")


question: should we pass through the sourceLocation argument to the expectation so the failure has a more meaningful file and line number?

bkhouri · 2025-06-12T18:50:49Z

Tests/FunctionalTests/PluginTests.swift

                    }
                    else {
-                        XCTAssertTrue(success, "expected command to succeed, but it failed", file: file, line: line)
+                        #expect(success, "expected command to succeed, but it failed")


question: should we pass through the sourceLocation argument to the expectation so the failure has a more meaningful file and line number?

bkhouri · 2025-06-12T18:53:29Z

Tests/FunctionalTests/PluginTests.swift

                    }
                }
                catch {
-                    XCTFail("error \(String(describing: error))", file: file, line: line)
+                    Issue.record("error \(String(describing: error)) at \(sourceLocation)")


suggestion (non-blocking): Instead of include the source location in failure message, set the sourceLocation argument.

Suggested change

Issue.record("error \(String(describing: error)) at \(sourceLocation)")

Issue.record("error \(String(describing: error))", sourceLocation: sourceLocation)`

See https://developer.apple.com/documentation/testing/issue/record(_:_:sourcelocation:)

bkhouri · 2025-06-12T19:00:02Z

Tests/FunctionalTests/PluginTests.swift

@@ -925,15 +979,15 @@ final class PluginTests: XCTestCase {
                        do {


suggestion (non-blocking): this do block could be replace with

#expect(throws: Never.self) { try scriptRunner.cancel(deadline: .now() + .seconds(5)) }

bkhouri · 2025-06-12T19:02:51Z

Tests/FunctionalTests/PluginTests.swift

+                    let (stdout, stderr) = try await executeSwiftBuild(
+                        fixturePath.appending(component: "MySourceGenPlugin"),
+                        configuration: .Debug,
+                        extraArgs: ["--vv", "--product", "MyLocalTool", "-Xbuild-tools-swiftc", "-DUSE_CREATE", "--build-system", "swiftbuild"]


question: is --vv required to to pass this test?

The "--vv" will give us enough information from swift build for problem diagnosis for things like linker failures. Otherwise, it's just a single error message indicating that linking failed, which isn't very helpful.

I've removed the verbose logging so that it doesn't fill up the logs. With linker errors, one will need a reproducing environment to be able to exfiltrate the linker arguments.

bkhouri · 2025-06-12T19:04:39Z

Sources/_InternalTestSupport/Observability.swift

+    if diagnostics.isEmpty { return }
+
+    let description = diagnostics.map { "- " + $0.description }.joined(separator: "\n")
+    Issue.record("Found unexpected diagnostics: \n\(description) \(sourceLocation.fileName):\(sourceLocation.line)")


issue (if-patch-updated): instead of putting the source location in the error message, can we call Issue.record with the sourceLocation argument set. That's, Swift Testing will accurately report the filename and line number.

Suggested change

Issue.record("Found unexpected diagnostics: \n\(description) \(sourceLocation.fileName):\(sourceLocation.line)")

Issue.record("Found unexpected diagnostics: \n\(description)", sourceLocation: sourceLocation)

See https://developer.apple.com/documentation/testing/issue/record(_:sourcelocation:)

cmcgee1024 · 2025-06-17T10:42:02Z

@swift-ci please test

cmcgee1024 · 2025-06-17T10:48:16Z

@swift-ci test Windows

Migrate PluginTests to Swift Testing

0ccba37

cmcgee1024 requested review from jakepetroules, dschaefer2, shawnhyam, bripeticca, plemarquand, owenv, bkhouri and daveyc123 as code owners June 9, 2025 19:58

bkhouri requested changes Jun 9, 2025

View reviewed changes

cmcgee1024 added 5 commits June 9, 2025 19:29

Skip more tests that have known issues with Windows

43e4cf1

Code review feedback

77a70f7

Code review feedback

3639b46

Fix compile error

e154575

Merge branch 'main' of https://github.com/swiftlang/swift-package-man…

6cb958e

…ager into plugin_testing_rework

cmcgee1024 added 3 commits June 10, 2025 14:54

Fix failing test on macOS by setting the writable directories to be t…

cf9fa98

…he plugin output directory

Remove remaining XCTest related assertions and adopt Swift Testing eq…

4a198fa

…uivalents

Skip test that is failing in CI smoke test for Linux

f062865

Serialize the test suite and mark known issue with Linux in smoke tes…

491cd55

…t only

cmcgee1024 enabled auto-merge (squash) June 12, 2025 19:02

bkhouri approved these changes Jun 12, 2025

View reviewed changes

cmcgee1024 disabled auto-merge June 12, 2025 19:09

cmcgee1024 added 2 commits June 12, 2025 15:46

Code review feedback

73ea6b2

Add verbose flag to test so that stdout can verify plugin execution

c469404

cmcgee1024 enabled auto-merge (squash) June 13, 2025 15:57

cmcgee1024 and others added 3 commits June 16, 2025 15:13

Merge branch 'main' into plugin_testing_rework

4c1c7cd

Fix debug constants

8524064

Merge branch 'main' into plugin_testing_rework

be3ed1e

	Issue.record("error \(String(describing: error)) at \(sourceLocation)")
	Issue.record("error \(String(describing: error))", sourceLocation: sourceLocation)`

		@@ -925,15 +979,15 @@ final class PluginTests: XCTestCase {
		do {

	Issue.record("Found unexpected diagnostics: \n\(description) \(sourceLocation.fileName):\(sourceLocation.line)")
	Issue.record("Found unexpected diagnostics: \n\(description)", sourceLocation: sourceLocation)

Migrate PluginTests to Swift Testing #8793

Are you sure you want to change the base?

Migrate PluginTests to Swift Testing #8793

Conversation

cmcgee1024 commented Jun 9, 2025

Uh oh!

cmcgee1024 commented Jun 9, 2025

Uh oh!

bkhouri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkhouri Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmcgee1024 Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bkhouri commented Jun 10, 2025

Uh oh!

cmcgee1024 commented Jun 11, 2025

Uh oh!

cmcgee1024 commented Jun 12, 2025

Uh oh!

cmcgee1024 commented Jun 12, 2025

Uh oh!

bkhouri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkhouri Jun 10, 2025 •

edited

Loading

cmcgee1024 Jun 9, 2025 •

edited

Loading