Fixing up Windows TestFileManager #2339
Conversation
TestFoundation/TestFileManager.swift
Outdated
| let code = CocoaError.Code(rawValue: ($0 as? NSError)!.code) | ||
| let code = CocoaError.Code(rawValue: ($0 as NSError).code) | ||
| #if os(Windows) | ||
| XCTAssertEqual(code, .fileReadNoSuchFile) |
There was a problem hiding this comment.
I think this is an issue in the error reporting — if a filename is invalid, the API should return .fileReadInvalidFileName no matter what the host OS thinks the right error is.
There was a problem hiding this comment.
We could do that, though it would mean every time we throw because for a Win32 api call failed, we'd have to check if the error is ERROR_FILE_NOT_FOUND then do some checks to see if it should actually be invalid path or perhaps some other more specific error. That doesn't seem cheap to me, but it would probably only be in the exceptional case.
TestFoundation/TestFileManager.swift
Outdated
| @@ -1378,15 +1451,15 @@ VIDEOS=StopgapVideos | |||
| XCTAssertTrue(fm.isDeletableFile(atPath: "")) | |||
|
|
|||
| XCTAssertThrowsError(try fm.attributesOfItem(atPath: "")) { | |||
| let code = CocoaError.Code(rawValue: ($0 as? NSError)!.code) | |||
| let code = CocoaError.Code(rawValue: ($0 as NSError).code) | |||
There was a problem hiding this comment.
This may not build on macOS.
gmittert
force-pushed
the
VariousFMFixes
branch
2 times, most recently
from
03fb43b to
8d5edd9
Compare
|
Removed the NSError changes, and have Windows follow posix such that "" is an invalid file instead of being an unfound file. |
|
@swift-ci please test |
|
Rebase @swift-ci please test linux |
|
@swift-ci please test linux |
|
I think that doing the remapping in the |
- _copyOrLinkDirectoryHelper should throw on errors - Accounting for that Windows permissions aren't compatible with posix ones - When given "" as a file name, most Windows apis return file not found, not invalid filename. Catch this and istead throw an invalid filename as posix does.
Fair, I've added a |
|
@swift-ci please test linux |
|
Thanks, this LGTM. |
| @@ -358,8 +360,7 @@ extension FileManager { | |||
| } | |||
|
|
|||
| internal func _copyOrLinkDirectoryHelper(atPath srcPath: String, toPath dstPath: String, variant: String = "Copy", _ body: (String, String, FileAttributeType) throws -> ()) throws { | |||
| var faAttributes: WIN32_FILE_ATTRIBUTE_DATA = WIN32_FILE_ATTRIBUTE_DATA() | |||
| do { faAttributes = try windowsFileAttributes(atPath: srcPath) } catch { return } | |||
| let faAttributes = try windowsFileAttributes(atPath: srcPath) | |||
There was a problem hiding this comment.
How come we don't need the exception to propagate here?
There was a problem hiding this comment.
Previously, if srcPath didn't exist, it would catch the error from windowsFileAttributes and _copyItem would silently not do anything. This way it bubbles up the exception and throws a file not found as it should.
| @@ -372,7 +373,7 @@ extension FileManager { | |||
| let src = joinPath(prefix: srcPath, suffix: item) | |||
| let dst = joinPath(prefix: dstPath, suffix: item) | |||
|
|
|||
| do { faAttributes = try windowsFileAttributes(atPath: src) } catch { return } | |||
| let faAttributes = try windowsFileAttributes(atPath: src) | |||
""as a file name, most Windows apis return file not found, not invalid filename