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

DougGregor · 2024-04-23T05:00:28Z

Introduce a command-line argument --testing-library to the add-target
command to specify which test library to generate the test for. This
can be 'xctest' (the prior XCTest behavior), 'swift-testing' (to use the
new swift-testing library), or 'none' (for no test library at all).

For the new swift-testing generation, also add the appropriate package
and test target dependency, along with a stub testsuite to start from.

Fixes #7478

DougGregor · 2024-04-23T05:00:34Z

@swift-ci please test

DougGregor · 2024-04-23T05:32:20Z

@swift-ci please test

DougGregor · 2024-04-23T05:32:42Z

@swift-ci please test Windows

grynspan

Thanks for opening this! I understand your motivation in wanting to use a single option here, however splitting it into two flags is important for a few reasons I listed in comments.

Thanks again!

grynspan · 2024-04-23T11:48:03Z

Sources/Commands/PackageCommands/AddTarget.swift

@@ -60,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


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

It would be consistent with the flags we've added to other commands;

swift-testing is experimental only;

The default behaviour if neither is specified should be to add support for XCTest only;

In the future, we can always change the default if needed; and

It is valid to specify both libraries as dependencies for a test target.

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.

DougGregor · 2024-04-23T16:43:58Z

@swift-ci please test

DougGregor · 2024-04-23T16:44:20Z

@swift-ci please test Windows

bnbarham · 2024-04-24T16:46:04Z

Sources/PackageModel/InstalledSwiftPMConfiguration.swift

 )
 }
+
+ private static var defaultSwiftTestingVersionForTestTemplate: Version {


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.

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.

bnbarham · 2024-04-24T16:51:21Z

Tests/PackageModelSyntaxTests/ManifestEditTests.swift

+ )
+ """,
+ expectedManifest: """
+ // swift-tools-version: 5.5


Where does 5.5 actually come from 🤔?

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.

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

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

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.

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.

Okay, sounds good. :)

stmontgomery · 2024-05-08T02:03:11Z

Sources/PackageModelSyntax/AddTarget.swift

+ @Suite
+ struct \(raw: target.name)Tests {
+ @Test("\(raw: target.name) tests")
+ func tests() {


Stylistic suggestion: I recommend naming the func something not prefixed by the word "test", like example(). The @Test attribute already marks it as a test so having that prefix can feel redundant. (I realize this was just adapted from the prior XCTest template, of course!)

(If you take this suggestion, I think a unit test below will need to be adjusted)

Sure, I'll drop the test prefix for swift-testing.

stmontgomery · 2024-05-08T02:04:33Z

Utilities/config.json

-{"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":7,"patch":0}}


We recently released tag 0.8.0 so you can bump this, as well as another place I saw in this PR

I'll bump in both places, thanks!

stmontgomery

Left a couple comments but this largely looks good to me from the perspective of a swift-testing maintainer! Thank you

Introduce a command-line argument `--testing-library` to the add-target command to specify which test library to generate the test for. This can be 'xctest' (the prior XCTest behavior), 'swift-testing' (to use the new swift-testing library), or 'none' (for no test library at all). For the new swift-testing generation, also add the appropriate package and test target dependency, along with a stub testsuite to start from. Fixes apple#7478

Make the conformance to Codable explicit so we can swap in the default swift-testing package version when it wasn't present in the config file.

DougGregor · 2024-05-08T05:23:03Z

@swift-ci please test

DougGregor · 2024-05-08T05:23:23Z

@swift-ci please test Windows

DougGregor · 2024-05-08T05:33:00Z

@swift-ci please test

DougGregor · 2024-05-08T05:33:19Z

@swift-ci please test Windows

Discussed offline

DougGregor · 2024-05-08T06:27:42Z

@swift-ci please test Linux

DougGregor · 2024-05-08T06:44:10Z

apple/swift#73507

@swift-ci please test Linux

…#7481) Introduce a command-line argument `--testing-library` to the add-target command to specify which test library to generate the test for. This can be 'xctest' (the prior XCTest behavior), 'swift-testing' (to use the new swift-testing library), or 'none' (for no test library at all). For the new swift-testing generation, also add the appropriate package and test target dependency, along with a stub testsuite to start from. Fixes apple#7478 (cherry picked from commit a37631a)

…7481) (#7537) **Explanation**: Introduce a command-line argument `--testing-library` to the new add-target command to specify which test library to generate the test for. This can be 'xctest' (the prior XCTest behavior), 'swift-testing' (to use the new swift-testing library), or 'none' (for no test library at all). **Original PR**: #7481 **Risk**: Low. Extends new functionality in a new package command. **Testing**: New tests.

DougGregor requested review from bnbarham, MaxDesiatov and tomerd as code owners April 23, 2024 05:00

DougGregor mentioned this pull request Apr 23, 2024

Add separate "add test" manifest actions for XCTest and Swift Testing apple/sourcekit-lsp#1197

Merged

MaxDesiatov added enhancement command-line interface labels Apr 23, 2024

grynspan requested review from stmontgomery, briancroom and grynspan April 23, 2024 11:46

grynspan previously requested changes Apr 23, 2024

View reviewed changes

DougGregor force-pushed the add-target-swift-testing branch from 0ec1f0f to 05b3ece Compare April 23, 2024 16:43

MaxDesiatov approved these changes Apr 24, 2024

View reviewed changes

bnbarham reviewed Apr 24, 2024

View reviewed changes

stmontgomery reviewed May 8, 2024

View reviewed changes

stmontgomery approved these changes May 8, 2024

View reviewed changes

DougGregor added 2 commits May 7, 2024 22:08

Make InstalledSwiftPMConfiguration Codable conformance explicit

165e3fc

Make the conformance to Codable explicit so we can swap in the default swift-testing package version when it wasn't present in the config file.

DougGregor force-pushed the add-target-swift-testing branch from 05b3ece to bcbd9ea Compare May 8, 2024 05:22

Address review feedback

b08070d

DougGregor force-pushed the add-target-swift-testing branch from bcbd9ea to b08070d Compare May 8, 2024 05:32

DougGregor enabled auto-merge (squash) May 8, 2024 05:33

DougGregor mentioned this pull request May 8, 2024

Add code actions for adding library/executable/macro targets to a package manifest apple/sourcekit-lsp#1240

Open

DougGregor merged commit a37631a into apple:main May 8, 2024
5 checks passed

DougGregor deleted the add-target-swift-testing branch May 8, 2024 13:36

DougGregor mentioned this pull request May 8, 2024

[6.0] Teach swift package add-target --type test about swift-testing (#7481) #7537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DougGregor commented Apr 23, 2024

DougGregor commented Apr 23, 2024

DougGregor commented Apr 23, 2024

DougGregor commented Apr 23, 2024

grynspan left a comment

grynspan Apr 23, 2024

DougGregor Apr 23, 2024

DougGregor commented Apr 23, 2024

DougGregor commented Apr 23, 2024

bnbarham Apr 24, 2024

DougGregor Apr 24, 2024

bnbarham Apr 24, 2024

DougGregor Apr 24, 2024

grynspan Apr 24, 2024

grynspan Apr 24, 2024

DougGregor Apr 24, 2024

DougGregor Apr 24, 2024

grynspan Apr 24, 2024

stmontgomery May 8, 2024 •

edited

DougGregor May 8, 2024

stmontgomery May 8, 2024

DougGregor May 8, 2024

stmontgomery left a comment

DougGregor commented May 8, 2024

DougGregor commented May 8, 2024

DougGregor commented May 8, 2024

DougGregor commented May 8, 2024

DougGregor commented May 8, 2024

DougGregor commented May 8, 2024

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

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

Conversation

DougGregor commented Apr 23, 2024

DougGregor commented Apr 23, 2024

DougGregor commented Apr 23, 2024

DougGregor commented Apr 23, 2024

grynspan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DougGregor commented Apr 23, 2024

DougGregor commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stmontgomery May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stmontgomery left a comment

Choose a reason for hiding this comment

DougGregor commented May 8, 2024

DougGregor commented May 8, 2024

DougGregor commented May 8, 2024

DougGregor commented May 8, 2024

DougGregor commented May 8, 2024

DougGregor commented May 8, 2024

stmontgomery May 8, 2024 •

edited