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.

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

disable broken test case on Windows #101

Open
wants to merge 2 commits into
base: release/1.3.0
Choose a base branch
from
Open

disable broken test case on Windows #101

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2afbd56
DNM: Tests: disable test on Windows
compnerd Jun 12, 2022
aacd458
[test] add explanation for skipping a test on Windows
glessard Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions Tests/SystemTests/FilePathTests/FilePathComponentsTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ final class FilePathComponentsTest: XCTestCase {
}

func testCases() {
let testPaths: Array<TestPathComponents> = [
var testPaths: Array<TestPathComponents> = [
TestPathComponents("", root: nil, []),
TestPathComponents("/", root: "/", []),
TestPathComponents("foo", root: nil, ["foo"]),
Expand All @@ -244,11 +244,16 @@ final class FilePathComponentsTest: XCTestCase {
TestPathComponents("/foo///bar", root: "/", ["foo", "bar"]),
TestPathComponents("foo/bar/", root: nil, ["foo", "bar"]),
TestPathComponents("foo///bar/baz/", root: nil, ["foo", "bar", "baz"]),
TestPathComponents("//foo///bar/baz/", root: "/", ["foo", "bar", "baz"]),
TestPathComponents("./", root: nil, ["."]),
TestPathComponents("./..", root: nil, [".", ".."]),
TestPathComponents("/./..//", root: "/", [".", ".."]),
]
#if !os(Windows)
// See https://github.com/apple/swift-system/issues/137
testPaths += [
glessard marked this conversation as resolved.
Show resolved Hide resolved
TestPathComponents("//foo///bar/baz/", root: "/", ["foo", "bar", "baz"]),
]
Copy link
Contributor

Choose a reason for hiding this comment

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

On Windows, would it become something like root: #"\\foo\\"#, ["bar", "baz"]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I need to check, but that is what I would expect. However, I don't remember if this was a case where we were asserting incorrectly or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we fix up incomplete roots and make them complete. C# seems to be pretty inconsistent / emergent-behaviorish here, but that's the only sensible model I could think of at the time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that the model is correct in that the behaviour is what we would like. The question is more about whether the behaviour matches the implementation or not (aka, there is a bug - I think that we were asserting on an empty path component somewhere?). Sorry if it came across as I didn't think that the behaviour was correct.

Copy link
Contributor

@milseman milseman Jun 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Path components cannot be empty, but I wonder if we should allow empty server or share portions of a UNC path root. Sorry, I've paged a lot of this out and am just getting back up to speed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

> [IO.Path]::GetFullPath("//foo///bar/baz/")
\\foo\bar\baz\
> [IO.Path]::GetPathRoot("//foo///bar/baz/")
\\foo\bar
> [IO.Path]::GetPathRoot([IO.Path]::GetFullPath("//foo///bar/baz/"))
\\foo\bar

This also matches the Microsoft documentation on path normalization:

Normally, any path passed to a Windows API is (effectively) passed to the GetFullPathName function and normalized. There is one important exception: a device path that begins with a question mark instead of a period. Unless the path starts exactly with \?\ (note the use of the canonical backslash), it is normalized.

#endif
testPaths.forEach { $0.runAllTests() }
}

Expand Down