Add swift-testing support to `swift build --build-tests` #7377

grynspan · 2024-02-28T16:01:50Z

This PR generalizes the enable/disable XCTest/swift-testing options from swift test and swift package init and adds them to swift build --build-tests so that it is possible to build all test content without actually running the tests. If neither flag is specified, the default test content is built (always XCTest, and swift-testing if it's a dependency, same as swift test.)

I don't have unit tests for this work because SwiftPM cannot currently reference or link to swift-testing in CI. I'm open to ideas here.

Resolves #7375.

This PR generalizes the enable/disable XCTest/swift-testing options from `swift test` and `swift package init` and adds them to `swift build --build-tests` so that it is possible to build all test content without actually running the tests. If neither flag is specified, the default test content is built (always XCTest, and swift-testing if it's a dependency, same as `swift test`.) I don't have unit tests for this work because SwiftPM cannot currently reference or link to swift-testing in CI. I'm open to ideas here. Resolves #7375.

weissi

I can't claim to fully understand the code but it does look good to me. Thank you!!

MaxDesiatov · 2024-02-28T16:07:46Z

Sources/Commands/SwiftTestCommand.swift

@@ -132,6 +148,20 @@ struct SharedOptions: ParsableArguments {
        // Default to disabled since swift-testing is experimental (opt-in.)
        return false
    }
+
+    /// Get the set of enabled testing libraries.
+    func enabledTestingLibraries(swiftCommandState: SwiftCommandState) throws -> Set<BuildParameters.Testing.Library> {


Formatting nit: we avoid long lines for better readability in diff views and on devices that don't have huge screens.

Suggested change

func enabledTestingLibraries(swiftCommandState: SwiftCommandState) throws -> Set<BuildParameters.Testing.Library> {

func enabledTestingLibraries(

swiftCommandState: SwiftCommandState

) throws -> Set<BuildParameters.Testing.Library> {

What's the cutoff you generally prefer? I can set my gutter to that.

We have all of the formatting rules specified in the formatter config https://github.com/apple/swift-package-manager/blob/main/.swiftformat. Just run ./Utilities/soundness.sh before committing and that will reformat changed files for you with that config.

That script did not produce any diagnostics nor make any changes to my branch.

grynspan · 2024-02-28T16:09:07Z

@swift-ci please test

MaxDesiatov

Would you mind adding a test case to prevent future regressions?

grynspan · 2024-02-28T16:56:02Z

Would you mind adding a test case to prevent future regressions?

See the PR description. How would you implement it?

grynspan · 2024-02-28T17:01:42Z

@swift-ci please test

grynspan · 2024-02-28T17:22:41Z

@swift-ci please test Windows

Sources/Commands/PackageCommands/Init.swift

Sources/Commands/SwiftTestCommand.swift

MaxDesiatov · 2024-02-28T19:23:16Z

See the PR description. How would you implement it?

I think we need a new BuildSystemProvider that can create a mock build system. Then we can initialize SwiftCommandState with desired options in the test and verify that the mock build system is passed correct build parameters for correct build subsets.

grynspan · 2024-02-28T20:01:10Z

See the PR description. How would you implement it?

I think we need a new BuildSystemProvider that can create a mock build system. Then we can initialize SwiftCommandState with desired options in the test and verify that the mock build system is passed correct build parameters for correct build subsets.

It looks like --build-tests doesn't really have any test coverage right now. I filed #7378 for test coverage for that option.

grynspan · 2024-02-28T20:06:33Z

@swift-ci please test

grynspan · 2024-02-28T20:09:13Z

@swift-ci please test Windows

MaxDesiatov · 2024-02-28T20:50:59Z

It looks like --build-tests doesn't really have any test coverage right now.

I understand, this shouldn't be the reason to reduce our overall test coverage though.

grynspan · 2024-02-28T21:01:45Z

It looks like --build-tests doesn't really have any test coverage right now.

I understand, this shouldn't be the reason to reduce our overall test coverage though.

I don't believe I'm reducing it as this feature already does not have any test coverage, but maybe I'm misunderstanding? 🙂

weissi · 2024-02-28T21:05:02Z

It looks like --build-tests doesn't really have any test coverage right now.

I understand, this shouldn't be the reason to reduce our overall test coverage though.

I'd argue that right now the feature is untested (bad) but also not working (worse). Why don't we make the situation better by fixing it? Once this is merged (and therefore fixed) we can maybe team up to write at least some basic tests?

grynspan · 2024-02-28T21:06:35Z

It looks like --build-tests doesn't really have any test coverage right now.

I understand, this shouldn't be the reason to reduce our overall test coverage though.

I'd argue that right now the feature is untested (bad) but also not working (worse). Why don't we make the situation better by fixing it? Once this is merged (and therefore fixed) we can maybe team up to write at least some basic tests?

That sounds like an excellent plan!

finagolfin · 2024-03-03T20:59:47Z

It appears the lack of tests hurt us here: this pull broke swift build --build-tests registering the --enable-test-discovery flag also. My Android CI builds swift-numerics trunk daily with the latest Swift trunk snapshot builds and those flags: it now errors with the March 1 snapshot.

I can reproduce the crash on linux x86_64 with the following commands, which work with the last Feb. 29 trunk snapshot before this pull:

> ../swift-DEVELOPMENT-SNAPSHOT-2024-03-01-a-ubi9/usr/bin/swift build --build-tests --enable-test-discovery
> .build/x86_64-unknown-linux-gnu/debug/swift-numericsPackageTests.xctest

) This PR generalizes the enable/disable XCTest/swift-testing options from `swift test` and `swift package init` and adds them to `swift build --build-tests` so that it is possible to build all test content without actually running the tests. If neither flag is specified, the default test content is built (always XCTest, and swift-testing if it's a dependency, same as `swift test`.) Resolves swiftlang#7375.

grynspan requested review from bnbarham, MaxDesiatov and tomerd as code owners February 28, 2024 16:01

grynspan requested a review from weissi February 28, 2024 16:02

grynspan self-assigned this Feb 28, 2024

grynspan added bug swift build Changes impacting `swift build` swift test Changes impacting `swift test` tool labels Feb 28, 2024

weissi approved these changes Feb 28, 2024

View reviewed changes

MaxDesiatov changed the title ~~Add swift-testing support to swift build --build-tests.~~ Add swift-testing support to swift build --build-tests Feb 28, 2024

MaxDesiatov reviewed Feb 28, 2024

View reviewed changes

MaxDesiatov added the needs tests This change needs test coverage label Feb 28, 2024

MaxDesiatov reviewed Feb 28, 2024

View reviewed changes

grynspan requested a review from MaxDesiatov February 28, 2024 16:56

grynspan added 2 commits February 28, 2024 11:59

Incorporate feedback

c29a3fc

Fix a build failure

5a3cb46

MaxDesiatov reviewed Feb 28, 2024

View reviewed changes

Sources/Commands/PackageCommands/Init.swift Outdated Show resolved Hide resolved

MaxDesiatov reviewed Feb 28, 2024

View reviewed changes

Sources/Commands/SwiftTestCommand.swift Outdated Show resolved Hide resolved

Incorporate feedback

f00a3cf

grynspan requested a review from MaxDesiatov February 28, 2024 21:34

MaxDesiatov merged commit 64adc92 into main Feb 29, 2024

MaxDesiatov deleted the jgrynspan/7375-swift-build-swift-testing branch February 29, 2024 08:53

MaxDesiatov mentioned this pull request Mar 4, 2024

swift build --build-tests doesn't work with --enable-test-discovery flag #7389

Closed

Add swift-testing support to swift build --build-tests #7377

Add swift-testing support to swift build --build-tests #7377

Uh oh!

Conversation

grynspan commented Feb 28, 2024

Uh oh!

weissi left a comment

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov Feb 28, 2024 • 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!

grynspan Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

grynspan Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

grynspan commented Feb 28, 2024

Uh oh!

MaxDesiatov left a comment

Choose a reason for hiding this comment

Uh oh!

grynspan commented Feb 28, 2024

Uh oh!

grynspan commented Feb 28, 2024

Uh oh!

grynspan commented Feb 28, 2024

Uh oh!

Uh oh!

Uh oh!

MaxDesiatov commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grynspan commented Feb 28, 2024

Uh oh!

grynspan commented Feb 28, 2024

Uh oh!

grynspan commented Feb 28, 2024

Uh oh!

MaxDesiatov commented Feb 28, 2024

Uh oh!

grynspan commented Feb 28, 2024

Uh oh!

weissi commented Feb 28, 2024

Uh oh!

grynspan commented Feb 28, 2024

Uh oh!

finagolfin commented Mar 3, 2024

Uh oh!

Uh oh!

Add swift-testing support to `swift build --build-tests` #7377

Add swift-testing support to `swift build --build-tests` #7377

MaxDesiatov Feb 28, 2024 •

edited

Loading

MaxDesiatov commented Feb 28, 2024 •

edited

Loading