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

TestFoundation: construct FileHandle properly on Windows #2242

Merged
merged 1 commit into from
Jul 2, 2019
Merged

TestFoundation: construct FileHandle properly on Windows #2242

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 22 additions & 0 deletions TestFoundation/TestFileHandle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
#if !DARWIN_COMPATIBILITY_TESTS // Disable until Foundation has the new FileHandle API

import Dispatch
#if os(Windows)
import WinSDK
#endif

class TestFileHandle : XCTestCase {
var allHandles: [FileHandle] = []
Expand Down Expand Up @@ -101,9 +104,28 @@ class TestFileHandle : XCTestCase {
func createFileHandleForReadErrors() -> FileHandle {
// Create a file handle where calling read returns -1.
// Accomplish this by creating one for a directory.
#if os(Windows)
let hDirectory: HANDLE = ".".withCString(encodedAs: UTF16.self) {
// NOTE(compnerd) we need the FILE_FLAG_BACKUP_SEMANTICS so that we
// can create the handle to the directory.
CreateFileW($0, GENERIC_READ,
DWORD(FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE),
nil, DWORD(OPEN_EXISTING),
DWORD(FILE_ATTRIBUTE_NORMAL | FILE_FLAG_BACKUP_SEMANTICS), nil)
}
if hDirectory == INVALID_HANDLE_VALUE {
fatalError("unable to create handle to current directory")
}
let fd = _open_osfhandle(intptr_t(bitPattern: hDirectory), 0)
if fd == -1 {
fatalError("unable to associate file descriptor with handle")
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to make this function return a FileHandle? so this can return nil and be XCTFailed by the caller, rather than stopping all of the tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

I followed what the old path was doing (fatal error'ing). I do like that, and since you asked, I'll go ahead and do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, lets do that in a follow up, I have a huge patch stack locally for the DEPLOYMENT_TARGET_* to TARGET_OS_* and that is making things more complicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I took a look at this and this is really ugly without some of the internal helpers (i.e. _NSErrorWithErrno and _NSErrorWithWindowsError). Do you want to convert this to a testable import test? CC: @millenomi

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that expectTrue() below doesn't invoke fatalError().

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if we can do anything else here. Its an internal error because this implies that the handle that was provided was invalid. It would indicate an error in the test itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay with that.

Copy link
Contributor

@millenomi millenomi Jul 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Meaning: I'm okay error'ing out if the test is executing such that '.' is not an accessible directory for reading.

}
let fh = FileHandle(fileDescriptor: fd, closeOnDealloc: true)
#else
let fd = open(".", O_RDONLY)
expectTrue(fd > 0, "We must be able to open a fd to the current directory (.)")
let fh = FileHandle(fileDescriptor: fd, closeOnDealloc: true)
#endif
allHandles.append(fh)
return fh
}
Expand Down