Skip to content
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

Fix up tests for Windows #5074

Merged
merged 9 commits into from
Aug 21, 2024
Merged

Conversation

jmschonfeld
Copy link
Contributor

This addresses some test failures where the tests need to be updated for the re-core on Windows. In some cases, the underlying code was changed and the test should be updated to reflect the new and/or correct behavior - in other cases the tests may have been lucky before but are incorrect and need to be changed or removed.

@jmschonfeld
Copy link
Contributor Author

@jmschonfeld
Copy link
Contributor Author

@jmschonfeld
Copy link
Contributor Author

swiftlang/swift#75932
swiftlang/swift-foundation#869

@swift-ci please test Windows platform

1 similar comment
@jmschonfeld
Copy link
Contributor Author

swiftlang/swift#75932
swiftlang/swift-foundation#869

@swift-ci please test Windows platform

@jmschonfeld
Copy link
Contributor Author

@jmschonfeld
Copy link
Contributor Author

swiftlang/swift#75932
swiftlang/swift-foundation#869

@swift-ci please test Windows platform

1 similar comment
@jmschonfeld
Copy link
Contributor Author

swiftlang/swift#75932
swiftlang/swift-foundation#869

@swift-ci please test Windows platform

@jmschonfeld
Copy link
Contributor Author

swiftlang/swift#75932
swiftlang/swift-foundation#869

@swift-ci please test Windows platform

2 similar comments
@jmschonfeld
Copy link
Contributor Author

swiftlang/swift#75932
swiftlang/swift-foundation#869

@swift-ci please test Windows platform

@jmschonfeld
Copy link
Contributor Author

swiftlang/swift#75932
swiftlang/swift-foundation#869

@swift-ci please test Windows platform

@jmschonfeld
Copy link
Contributor Author

swiftlang/swift#75932
swiftlang/swift-foundation#869

@swift-ci please test Linux platform

@@ -21,7 +21,9 @@ let validPathSeps: [Character] = ["/"]
#endif

public func NSTemporaryDirectory() -> String {
FileManager.default.temporaryDirectory.path()
FileManager.default.temporaryDirectory.withUnsafeFileSystemRepresentation {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was necessary to ensure that paths with drive letters do not have a leading slash (the file system representation standardizes that)

@@ -78,6 +78,14 @@ class BundlePlayground {
#endif
}
}

var fileNameSuffix: String? {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe Foundation's tests have been run in debug mode before, so this test was failing when Foundation was built in debug mode and this fixes it by adding the appropriate file suffix

Copy link
Member

Choose a reason for hiding this comment

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

Correct! We always build in release mode currently due to other limitations that I would like to see lifted.

@@ -104,6 +104,8 @@ class TestThread : XCTestCase {
func test_callStackSymbols() throws {
#if os(Android) || os(OpenBSD)
throw XCTSkip("Android/OpenBSD doesn't support backtraces at the moment.")
#elseif os(Windows)
throw XCTSkip("This test is unexpectedly crashing in CI at the moment.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not certain why, but these tests were crashing in CI. Disabling for now until we can followup with a fix

@@ -652,6 +652,9 @@ final class TestURLSession: LoopbackServerTest, @unchecked Sendable {
}

func test_repeatedRequestsStress() async throws {
#if os(Windows)
throw XCTSkip("This test is currently disabled on Windows")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a new test added after the re-core, so the test hadn't been run on Windows CI before. It's failing and @jrflat is looking into it


XCTAssertThrowsError(try fm.contentsOfDirectory(atPath: "")) {
let code = ($0 as? CocoaError)?.code
XCTAssertEqual(code, .fileReadNoSuchFile)
XCTAssertEqual(code, emptyFileNameError ?? .fileReadNoSuchFile)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to suggestions on this. Previously in SCL-F, for empty paths we always threw .fileReadInvalidFileName. As part of the re-core, the Linux implementation changed to match the Darwin implementation which threw errors like .fileReadNoSuchFile. However, the Windows implementation still throws .fileReadInvalidFileName. For now, this just updates the test to account for the behavior today, but if we want (potentially in a separate change if that's cleaner) we can update the Windows implementation to throw the same error codes

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please! We should match the Darwin error codes if possible. The Windows path did reference the Linux path heavily for error conditions and semantics originally and the two diverged :(.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed swiftlang/swift-foundation#882 to track standardizing this as a followup (I think it'd be great to land these tests now to prevent any other behavioral regressions on Windows and I'll followup shortly with a change to swift-foundation to throw the right error code and copy these tests to that repo as well

Copy link
Contributor

@itingliu itingliu left a comment

Choose a reason for hiding this comment

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

The change looks reasonable to me

@@ -78,6 +78,14 @@ class BundlePlayground {
#endif
}
}

var fileNameSuffix: String? {
Copy link
Member

Choose a reason for hiding this comment

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

Correct! We always build in release mode currently due to other limitations that I would like to see lifted.

@jmschonfeld jmschonfeld merged commit 1b8c1ea into swiftlang:main Aug 21, 2024
2 checks passed
@jmschonfeld jmschonfeld deleted the windows/test-fixes branch August 21, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants