Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Teach swift package add-target --type test about swift-testing #7481

Merged
merged 3 commits into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions Sources/Commands/PackageCommands/AddTarget.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import TSCBasic
import TSCUtility
import Workspace

extension AddTarget.TestHarness: ExpressibleByArgument { }

extension SwiftPackageCommand {
struct AddTarget: SwiftCommand {
/// The type of target that can be specified on the command line.
Expand Down Expand Up @@ -58,6 +60,9 @@ extension SwiftPackageCommand {
@Option(help: "The checksum for a remote binary target")
var checksum: String?

@Option(help: "The testing library to use when generating test targets, which can be one of 'xctest', 'swift-testing', or 'none'")
var testingLibrary: PackageModelSyntax.AddTarget.TestHarness = .default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please break this up into two flags, --enable/disable-xctest and --enable/disable-experimental-swift-testing:

  1. It would be consistent with the flags we've added to other commands;
  2. swift-testing is experimental only;
  3. The default behaviour if neither is specified should be to add support for XCTest only;
  4. In the future, we can always change the default if needed; and
  5. It is valid to specify both libraries as dependencies for a test target.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you are proposing is a confusing design for functionality that is adding a brand-new test target. Roughly nobody is going to create a new test target that uses both of these testing libraries: they'll pick one and stick with it.

Other commands need to deal with one or both libraries being present, because existing test targets likely use XCTest and will want to pick up Swift Testing. This functionality is not for those use cases.


func run(_ swiftCommandState: SwiftCommandState) throws {
let workspace = try swiftCommandState.getActiveWorkspace()

Expand Down Expand Up @@ -110,6 +115,7 @@ extension SwiftPackageCommand {
let editResult = try PackageModelSyntax.AddTarget.addTarget(
target,
to: manifestSyntax,
configuration: .init(testHarness: testingLibrary),
installedSwiftPMConfiguration: swiftCommandState
.getHostToolchain()
.installedSwiftPMConfiguration
Expand Down
54 changes: 52 additions & 2 deletions Sources/PackageModel/InstalledSwiftPMConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
//
//===----------------------------------------------------------------------===//

public struct InstalledSwiftPMConfiguration: Codable {
public struct InstalledSwiftPMConfiguration {
public struct Version: Codable, CustomStringConvertible {
let major: Int
let minor: Int
Expand All @@ -31,6 +31,7 @@ public struct InstalledSwiftPMConfiguration: Codable {

let version: Int
public let swiftSyntaxVersionForMacroTemplate: Version
public let swiftTestingVersionForTestTemplate: Version

public static var `default`: InstalledSwiftPMConfiguration {
return .init(
Expand All @@ -40,7 +41,56 @@ public struct InstalledSwiftPMConfiguration: Codable {
minor: 0,
patch: 0,
prereleaseIdentifier: "latest"
)
),
swiftTestingVersionForTestTemplate: defaultSwiftTestingVersionForTestTemplate
)
}

private static var defaultSwiftTestingVersionForTestTemplate: Version {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not have this at all and solely use config.json, swiftSyntaxVersionForMacroTemplate should also be removed but we can at least not add another.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this was driven by a failure in Linux CI in prior iterations of this patch, where we were getting a config.json from an older toolchain. It seemed like a reasonable policy that we would support at least slightly-older toolchains, so the default bridges that gap. If we want to be strict about this, we'll need to track down the configuration that produced this failure.

.init(
major: 0,
minor: 8,
patch: 0,
prereleaseIdentifier: nil
)
}
}

extension InstalledSwiftPMConfiguration: Codable {
enum CodingKeys: CodingKey {
case version
case swiftSyntaxVersionForMacroTemplate
case swiftTestingVersionForTestTemplate
}

public init(from decoder: any Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)

self.version = try container.decode(
Int.self,
forKey: CodingKeys.version
)
self.swiftSyntaxVersionForMacroTemplate = try container.decode(
Version.self,
forKey: CodingKeys.swiftSyntaxVersionForMacroTemplate
)
self.swiftTestingVersionForTestTemplate = try container.decodeIfPresent(
Version.self,
forKey: CodingKeys.swiftTestingVersionForTestTemplate
) ?? InstalledSwiftPMConfiguration.defaultSwiftTestingVersionForTestTemplate
}

public func encode(to encoder: any Encoder) throws {
var container = encoder.container(keyedBy: CodingKeys.self)

try container.encode(self.version, forKey: CodingKeys.version)
try container.encode(
self.swiftSyntaxVersionForMacroTemplate,
forKey: CodingKeys.swiftSyntaxVersionForMacroTemplate
)
try container.encode(
self.swiftTestingVersionForTestTemplate,
forKey: CodingKeys.swiftTestingVersionForTestTemplate
)
}
}
134 changes: 124 additions & 10 deletions Sources/PackageModelSyntax/AddTarget.swift
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,40 @@ public struct AddTarget {
"cxxLanguageStandard"
]

/// The kind of test harness to use. This isn't part of the manifest
/// itself, but is used to guide the generation process.
public enum TestHarness: String, Codable {
/// Don't use any library
case none

/// Create a test using the XCTest library.
case xctest

/// Create a test using the swift-testing package.
case swiftTesting = "swift-testing"

/// The default testing library to use.
public static var `default`: TestHarness = .xctest
}

/// Additional configuration information to guide the package editing
/// process.
public struct Configuration {
/// The test harness to use.
public var testHarness: TestHarness

public init(testHarness: TestHarness = .default) {
self.testHarness = testHarness
}
}

/// Add the given target to the manifest, producing a set of edit results
/// that updates the manifest and adds some source files to stub out the
/// new target.
public static func addTarget(
_ target: TargetDescription,
to manifest: SourceFileSyntax,
configuration: Configuration = .init(),
installedSwiftPMConfiguration: InstalledSwiftPMConfiguration = .default
) throws -> PackageEditResult {
// Make sure we have a suitable tools version in the manifest.
Expand All @@ -49,10 +77,20 @@ public struct AddTarget {
// content when needed.
var target = target

// Macro targets need to depend on a couple of libraries from
// SwiftSyntax.
if target.type == .macro {
// Add dependencies needed for various targets.
switch target.type {
case .macro:
// Macro targets need to depend on a couple of libraries from
// SwiftSyntax.
target.dependencies.append(contentsOf: macroTargetDependencies)

case .test where configuration.testHarness == .swiftTesting:
// Testing targets using swift-testing need to depend on
// SwiftTesting from the swift-testing package.
target.dependencies.append(contentsOf: swiftTestingTestTargetDependencies)

default:
break;
}

var newPackageCall = try packageCall.appendingToArrayArgument(
Expand Down Expand Up @@ -84,6 +122,7 @@ public struct AddTarget {
addPrimarySourceFile(
outerPath: outerPath,
target: target,
configuration: configuration,
to: &auxiliaryFiles
)

Expand Down Expand Up @@ -124,6 +163,17 @@ public struct AddTarget {
}
}

case .test where configuration.testHarness == .swiftTesting:
if !manifest.description.contains("swift-testing") {
newPackageCall = try AddPackageDependency
.addPackageDependencyLocal(
.swiftTesting(
configuration: installedSwiftPMConfiguration
),
to: newPackageCall
)
}

default: break;
}

Expand All @@ -140,6 +190,7 @@ public struct AddTarget {
fileprivate static func addPrimarySourceFile(
outerPath: RelativePath,
target: TargetDescription,
configuration: Configuration,
to auxiliaryFiles: inout AuxiliaryFiles
) {
let sourceFilePath = outerPath.appending(
Expand All @@ -153,7 +204,17 @@ public struct AddTarget {

// Add appropriate test module dependencies.
if target.type == .test {
importModuleNames.append("XCTest")
switch configuration.testHarness {
case .none:
break

case .xctest:
importModuleNames.append("XCTest")

case .swiftTesting:
// Import is handled by the added dependency.
break
}
}

let importDecls = importModuleNames.lazy.sorted().map { name in
Expand Down Expand Up @@ -184,14 +245,35 @@ public struct AddTarget {
"""

case .test:
"""
\(imports)
class \(raw: target.name): XCTestCase {
func test\(raw: target.name)() {
XCTAssertEqual(42, 17 + 25)
switch configuration.testHarness {
case .none:
"""
\(imports)
// Test code here
"""

case .xctest:
"""
\(imports)
class \(raw: target.name): XCTestCase {
func test\(raw: target.name)() {
XCTAssertEqual(42, 17 + 25)
}
}
"""

case .swiftTesting:
"""
\(imports)
@Suite
struct \(raw: target.name)Tests {
@Test("\(raw: target.name) tests")
func example() {
#expect(42 == 17 + 25)
}
}
"""
}
"""

case .regular:
"""
Expand Down Expand Up @@ -298,3 +380,35 @@ fileprivate extension PackageDependency {
)
}
}

/// The set of dependencies we need to introduce to a newly-created macro
/// target.
fileprivate let swiftTestingTestTargetDependencies: [TargetDescription.Dependency] = [
.product(name: "Testing", package: "swift-testing"),
]


/// The package dependency for swift-testing, for use in test files.
fileprivate extension PackageDependency {
/// Source control URL for the swift-syntax package.
static var swiftTestingURL: SourceControlURL {
"https://github.com/apple/swift-testing.git"
}

/// Package dependency on the swift-testing package.
static func swiftTesting(
configuration: InstalledSwiftPMConfiguration
) -> PackageDependency {
let swiftTestingVersionDefault =
configuration.swiftTestingVersionForTestTemplate
let swiftTestingVersion = Version(swiftTestingVersionDefault.description)!

return .sourceControl(
identity: PackageIdentity(url: swiftTestingURL),
nameForTargetDependencyResolutionOnly: nil,
location: .remote(swiftTestingURL),
requirement: .range(.upToNextMajor(from: swiftTestingVersion)),
productFilter: .everything
)
}
}
47 changes: 47 additions & 0 deletions Tests/PackageModelSyntaxTests/ManifestEditTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,53 @@ class ManifestEditTests: XCTestCase {
}
}

func testAddSwiftTestingTestTarget() throws {
try assertManifestRefactor("""
// swift-tools-version: 5.5
let package = Package(
name: "packages"
)
""",
expectedManifest: """
// swift-tools-version: 5.5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does 5.5 actually come from 🤔?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5.5 introduced some tweaks to the package manifest format (e.g., some things moved to labels), such as introducing new package dependencies and deprecating some old ones. We can either write a bunch of code to deal with all of the differences, or we can limit the functionality to 5.5 or newer. The latter was easier.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swift-testing is going to need a manifest version of 6.0 or later at some point in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm not sure if that affects its clients—I assume not?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if we end up changing the manifest format in some way for swift-testing, we'll need make changes here as well. We can enforce a 6.0 manifest or help with updating the manifest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clients of a package are not affected by the package manifest version of the package, other than the fact that all consumers of the client will need new-enough tools.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sounds good. :)

let package = Package(
name: "packages",
dependencies: [
.package(url: "https://github.com/apple/swift-testing.git", from: "0.8.0"),
],
targets: [
.testTarget(
name: "MyTest",
dependencies: [ .product(name: "Testing", package: "swift-testing") ]
),
]
)
""",
expectedAuxiliarySources: [
RelativePath("Tests/MyTest/MyTest.swift") : """
import Testing

@Suite
struct MyTestTests {
@Test("MyTest tests")
func example() {
#expect(42 == 17 + 25)
}
}
"""
]) { manifest in
try AddTarget.addTarget(
TargetDescription(
name: "MyTest",
type: .test
),
to: manifest,
configuration: .init(
testHarness: .swiftTesting
)
)
}
}
}


Expand Down
4 changes: 3 additions & 1 deletion Utilities/config.json
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
{"version":1,"swiftSyntaxVersionForMacroTemplate":{"major":600,"minor":0,"patch":0, "prereleaseIdentifier":"latest"}}
{"version":1,
"swiftSyntaxVersionForMacroTemplate":{"major":600,"minor":0,"patch":0, "prereleaseIdentifier":"latest"},
"swiftTestingVersionForTestTemplate":{"major":0,"minor":8,"patch":0}}