disable broken test case on Windows #101
Conversation
This test triggers an assertion failure. It is unclear why this is currently being parsed improperly.
| #if !os(Windows) | ||
| testPaths += [ | ||
| TestPathComponents("//foo///bar/baz/", root: "/", ["foo", "bar", "baz"]), | ||
| ] |
There was a problem hiding this comment.
On Windows, would it become something like root: #"\\foo\\"#, ["bar", "baz"]?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
> [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\barThis 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.
|
@tomerd wanted to make progress on adopting System for SPM, and we need to resolve this (along with the other test failures). I've since paged a lot of the path processing out, but I think that this particular case should be parsed as |
|
@swift-ci please test |
|
I would like to unblock the rest of Windows testing, so I am inclined to merge this with the addition of a link to issue 137. |
|
@swift-ci please test |
glessard
changed the title
This test triggers an assertion failure. It is unclear why this is
currently being parsed improperly.