Skip to content

Non-darwin test discovery with Swift Build #8722

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Non-darwin test discovery with Swift Build #8722

wants to merge 1 commit into from
+243 −100

Conversation

owenv
Copy link
Contributor

@owenv owenv commented May 25, 2025

Still needs a fair amount of work and cleanup, I'll look at breaking off some parts that can land independently. Depends on the larger patch in swiftlang/swift-build#499

@owenv owenv marked this pull request as draft May 25, 2025 02:02
owenv
owenv commented May 25, 2025
View reviewed changes
owenv
owenv commented May 25, 2025
View reviewed changes
owenv
owenv commented May 25, 2025
View reviewed changes
@owenv
Copy link
Contributor Author

owenv commented May 25, 2025

Locally, I can pass a decent chunk of TestDiscoveryTests on linux. macOS will need a bit of work to skip building the test runner executables, and I expect Windows will need a few tweaks

@swift-ci test

jakepetroules
jakepetroules reviewed May 25, 2025
View reviewed changes
@owenv owenv force-pushed the owenv/test-discovery branch from a18182a to 10bc51d Compare May 25, 2025 20:30
@owenv
Copy link
Contributor Author

owenv commented May 25, 2025

swiftlang/swift-build#499
@swift-ci test

@owenv owenv marked this pull request as ready for review May 25, 2025 20:38
@owenv owenv changed the title WIP: Non-darwin test discovery with Swift Build Non-darwin test discovery with Swift Build May 25, 2025
@owenv owenv force-pushed the owenv/test-discovery branch from 10bc51d to 401a93f Compare June 4, 2025 18:20
@owenv
Copy link
Contributor Author

owenv commented Jun 4, 2025

@swift-ci test

@owenv owenv force-pushed the owenv/test-discovery branch from 401a93f to 3757526 Compare June 9, 2025 16:10
@owenv
Copy link
Contributor Author

owenv commented Jun 9, 2025

@swift-ci test

bkhouri
bkhouri reviewed Jun 10, 2025
View reviewed changes
Copy link
Contributor

@bkhouri bkhouri left a comment

Choose a reason for hiding this comment

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

Thanks for converting the TestDiscoverTests from XCTest to Swift Testing. I have a couple questions/comments, but nothing blocking.

@owenv owenv force-pushed the owenv/test-discovery branch from 633a864 to 88ccf4a Compare June 10, 2025 18:13
@owenv
Copy link
Contributor Author

owenv commented Jun 10, 2025

@swift-ci test

@owenv owenv force-pushed the owenv/test-discovery branch from 88ccf4a to 23a8d43 Compare June 11, 2025 16:45
@owenv
Copy link
Contributor Author

owenv commented Jun 11, 2025

@swift-ci test

@owenv owenv force-pushed the owenv/test-discovery branch from 23a8d43 to 61c3ec0 Compare June 11, 2025 18:33
@owenv
Copy link
Contributor Author

owenv commented Jun 11, 2025

@swift-ci test

@owenv
Copy link
Contributor Author

owenv commented Jun 11, 2025

Working to address linker failures in swiftlang/swift-build#576

@owenv
Copy link
Contributor Author

owenv commented Jun 12, 2025

@swift-ci test

@owenv owenv mentioned this pull request Jun 16, 2025
Draft
@owenv owenv force-pushed the owenv/test-discovery branch from 61c3ec0 to 8a2f0a2 Compare June 19, 2025 03:09
@owenv
Copy link
Contributor Author

owenv commented Jun 19, 2025

@swift-ci test

@owenv owenv force-pushed the owenv/test-discovery branch from 8a2f0a2 to 9cfbd82 Compare June 20, 2025 16:04
@owenv
Copy link
Contributor Author

owenv commented Jun 20, 2025

@swift-ci test

@owenv owenv force-pushed the owenv/test-discovery branch from 9cfbd82 to e63dc87 Compare June 20, 2025 19:27
@owenv
Copy link
Contributor Author

owenv commented Jun 20, 2025

@swift-ci test

@owenv owenv force-pushed the owenv/test-discovery branch from e63dc87 to 70dcd09 Compare June 20, 2025 20:51
bkhouri
bkhouri reviewed Jun 21, 2025
View reviewed changes
Tests/CommandsTests/PackageCommandTests.swift
@@ -4106,11 +4106,12 @@ class PackageCommandSwiftBuildTests: PackageCommandTestCase {
throw XCTSkip("SWBINTTODO: Build plan is not currently supported")
}

#if !os(macOS)
Copy link
Contributor

Choose a reason for hiding this comment

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

question: why are we not executing this test on macOS?

bkhouri
bkhouri reviewed Jun 21, 2025
View reviewed changes
Tests/FunctionalTests/TestDiscoveryTests.swift
static var buildSystems: [BuildSystemProvider.Kind] = {
if ProcessInfo.hostOperatingSystem == .windows {
// Windows currently fails due to long path handling issues on Windows
return [BuildSystemProvider.Kind.native]
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (blocking): we should execute these tests on Windows and mark each test as withKnownIssue so we get signal, so we can ensure the test is enabled once the underlying windows path issue is fixed.

Have a looks at Tests/FunctionalTests/TraitTests.swift for how those tests handle it.

bkhouri
bkhouri requested changes Jun 21, 2025
View reviewed changes
Copy link
Contributor

@bkhouri bkhouri left a comment

Choose a reason for hiding this comment

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

I only reviewed the tests changes. Although the overcharge is good, we should ensure to run the TestDicoveryTests against SwiftBuild on Windows and use the withKnownIssue Swift Testing API to essentially mark the test as "expected fail". This will give us signal when the underlying Windows issue is fix, thus prompting the removal of the withKnownIssue as opposed to having to remember to augment the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakepetroules jakepetroules jakepetroules left review comments

@bkhouri bkhouri bkhouri requested changes

@dschaefer2 dschaefer2 dschaefer2 approved these changes

@shawnhyam shawnhyam Awaiting requested review from shawnhyam shawnhyam is a code owner

@bripeticca bripeticca Awaiting requested review from bripeticca bripeticca is a code owner

@plemarquand plemarquand Awaiting requested review from plemarquand plemarquand is a code owner

@cmcgee1024 cmcgee1024 Awaiting requested review from cmcgee1024 cmcgee1024 is a code owner

@daveyc123 daveyc123 Awaiting requested review from daveyc123 daveyc123 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@owenv @jakepetroules @bkhouri @dschaefer2