-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
@swift-ci test |
| basename = String(basename.dropLast(4)) | ||
| } | ||
| if basename == "/" { | ||
| if basename == "/" || basename == "\\" { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
8c9336b to
18543ae
Compare
|
@swift-ci test |
|
@swift-ci test windows |
fb81f29 to
12e88df
Compare
bkhouri
left a comment
There was a problem hiding this 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!
| basename = String(basename.dropLast(4)) | ||
| } | ||
| if basename == "/" { | ||
| if basename == "/" || basename == "\\" { |
There was a problem hiding this comment.
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?
| } | ||
| } when: { | ||
| ProcessInfo.isHostAmazonLinux2() | ||
| ProcessInfo.isHostAmazonLinux2() //rdar://134238535 |
There was a problem hiding this comment.
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?
| await XCTAssertBuilds( | ||
| fixturePath, | ||
| Xswiftc: ["-warnings-as-errors"], | ||
| buildSystem: .native, |
There was a problem hiding this comment.
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
| .IssueWindowsLongPath, | ||
| .IssueWindowsPathLastComponent, | ||
| .IssueWindowsRelativePathAssert, | ||
| .tags( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (blocking): ca we keep the .tags(...) associated with this test?
12e88df to
9a11616
Compare
- 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
9a11616 to
dfe2626
Compare
Fix various tests so they pass on Windows