Skip to content

Various fixes for tests on Windows #9268

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
daveinglis wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Various fixes for tests on Windows #9268

daveinglis wants to merge 1 commit into from

Conversation

@daveinglis
Copy link
Contributor

@daveinglis daveinglis commented Oct 21, 2025

Fix various tests so they pass on Windows

@daveinglis
Copy link
Contributor Author

daveinglis commented Oct 21, 2025

@swift-ci test

jakepetroules
jakepetroules reviewed Oct 22, 2025
View reviewed changes
Sources/SourceControl/Repository.swift
basename = String(basename.dropLast(4))
}
if basename == "/" {
if basename == "/" || basename == "\\" {
Copy link
Contributor

@jakepetroules jakepetroules Oct 22, 2025

Choose a reason for hiding this comment

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

We should have a single central variable that is an array of the possible path separators for the host.

See https://github.com/swiftlang/swift-build/blob/bc2e7d92d36caed0399f64594b9e5518b2f83731/Sources/SWBUtil/Path.swift#L69 for example.

There might already be one in the SwiftPM codebase somewhere, or in TSC.

Also, this part of the logic should probably be constrained to file URLs.

Copy link
Contributor Author

@daveinglis daveinglis Oct 22, 2025

Choose a reason for hiding this comment

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

Ya, I couldn't find a central definition for path separators like whats in swift-build, and it looks like from this https://github.com/swiftlang/swift-package-manager/pull/6706/files we moved away from URL

Copy link
Contributor

@bkhouri bkhouri Nov 24, 2025

Choose a reason for hiding this comment

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

I like python's take on the separator by storing in a variable. e.g.: os.sep. Should we consider something similar for Swift?

Copy link
Contributor

@jakepetroules jakepetroules Dec 1, 2025

Choose a reason for hiding this comment

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

If by "for Swift" you mean the Swift standard library, I don't think it's appropriate for the stdlib to be aware of the concept of filesystems and file paths. Maybe a SwiftSystem API.

@daveinglis
Copy link
Contributor Author

daveinglis commented Oct 22, 2025

@swift-ci test

@daveinglis
Copy link
Contributor Author

daveinglis commented Oct 22, 2025

@swift-ci test windows

@daveinglis daveinglis force-pushed the test_fixes branch 2 times, most recently from fb81f29 to 12e88df Compare November 24, 2025 15:37
bkhouri
bkhouri reviewed Nov 24, 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.

It's great to see tests being enabled for Windows!

Sources/SourceControl/Repository.swift
basename = String(basename.dropLast(4))
}
if basename == "/" {
if basename == "/" || basename == "\\" {
Copy link
Contributor

@bkhouri bkhouri Nov 24, 2025

Choose a reason for hiding this comment

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

I like python's take on the separator by storing in a variable. e.g.: os.sep. Should we consider something similar for Swift?

Tests/CommandsTests/PackageCommandTests.swift
}
} when: {
ProcessInfo.isHostAmazonLinux2()
ProcessInfo.isHostAmazonLinux2() //rdar://134238535
Copy link
Contributor

@bkhouri bkhouri Nov 24, 2025

Choose a reason for hiding this comment

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

suggestion: can we references a GitHub issue instead?

Tests/FunctionalTests/ResourcesTests.swift
await XCTAssertBuilds(
fixturePath,
Xswiftc: ["-warnings-as-errors"],
buildSystem: .native,
Copy link
Contributor

@bkhouri bkhouri Nov 24, 2025

Choose a reason for hiding this comment

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

issue: this suite is verifying the native build system. can we migrate this suite to SwiftTesting and ensure we verify the SwiftBuild build system too?

This comment applies to all applicable XCTestCase

@daveinglis daveinglis force-pushed the test_fixes branch 3 times, most recently from dfe2626 to e34946e Compare December 1, 2025 15:41
@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 1, 2025

@swift-ci test

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 3, 2025

@swift-ci test

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 3, 2025

@swift-ci test macos

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 3, 2025

@swift-ci test

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 4, 2025

@swift-ci test windows

@daveinglis daveinglis marked this pull request as ready for review December 4, 2025 19:34
@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test windows self hosted

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test windows

1 similar comment
@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test windows

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test windows

@daveinglis daveinglis enabled auto-merge (squash) December 5, 2025 13:45
@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test windows

1 similar comment
@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test windows

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test windows

1 similar comment
@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test windows

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test windows

1 similar comment
@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 5, 2025

@swift-ci test windows

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 7, 2025

@swift-ci test

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 7, 2025

@swift-ci test windows

@daveinglis
Various fixes for tests on Windows
fa56099 
- also mark withKnownIssues tests as isIntermittent: true so that as we
  fix things in dependent packages (like swift-build) we don't break
  swiftPM CI
@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 7, 2025

@swift-ci test

@daveinglis
Copy link
Contributor Author

daveinglis commented Dec 7, 2025

@swift-ci test windows

@rconnell9
Copy link
Contributor

rconnell9 commented Dec 8, 2025

@swift-ci test linux

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 left review comments

@owenv owenv owenv approved these changes

@dschaefer2 dschaefer2 Awaiting requested review from dschaefer2 dschaefer2 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

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@daveinglis @rconnell9 @jakepetroules @owenv @bkhouri