Add an option to put WinAppDriver output in a file #151
Conversation
This enriches the internal `Win32ProcessTree` API to take `HANDLE` objects for stdin, stdout and stderr, as well as adding an option to use the parent console rather than spawning a new one.
| DWORD(CREATE_SUSPENDED) | DWORD(CREATE_NEW_PROCESS_GROUP) | ||
| | (options.spawnNewConsole ? DWORD(CREATE_NEW_CONSOLE) : 0) | ||
| if let stdoutHandle = options.stdoutHandle { | ||
| startupInfo.hStdOutput = stdoutHandle |
There was a problem hiding this comment.
Suggestion for more compact
startupInfo.hStdOutput = options.stdoutHandle ?? INVALID_HANDLE_VALUE
startupInfo.hStdError = options.stderrHandle ?? INVALID_HANDLE_VALUE
startupInfo.hStdInput = options.stdinHandle ?? INVALID_HANDLE_VALUE
let redirectStdHandle = options.stdoutHandle != INVALID_HANDLE_VALUE || options.stderrHandle != INVALID_HANDLE_VALUE || options.stdinHandle != INVALID_HANDLE_VALUEThere was a problem hiding this comment.
We should not set these handles to INVALID_HANDLE_VALUE because INVALID_HANDLE_VALUE (= -1 = 0xfff...) is not actually invalid but is the current process handle. So here, we use the current process input/output handles as a fallback, which makes the redirectStdHandle logic more complex than it should be.
GetStdHandle() will return NULL if the standard input/output devices have been closed, which is also a valid value to set the stdoutHandle option to if you don't want the child process to write anything, for instance. IIUC NULL HANDLE (=0) is a different value from Swift nil, since the stdoutHandle in the ProcessLaunchOptions struct is an optional.
| startupInfo.hStdOutput = stdoutHandle | ||
| redirectStdHandle = true | ||
| } else { | ||
| startupInfo.hStdOutput = INVALID_HANDLE_VALUE |
There was a problem hiding this comment.
Does this preserve the old behavior if we only set some of the handles? Should we force callers to either override all handles, or none?
| @@ -66,6 +122,9 @@ public class WinAppDriver: WebDriver { | |||
| assertionFailure("WinAppDriver did not terminate within the expected time: \(error).") | |||
| } | |||
| } | |||
| if let childStdinHandle { | |||
| CloseHandle(childStdinHandle) | |||
There was a problem hiding this comment.
Do we need to keep the stdin write handle until here if just to close it? And since we have it, we could send \n for nicer termination since WinAppDriver awaits one:
C:>"C:\Program Files (x86)\Windows Application Driver\WinAppDriver.exe"
Windows Application Driver listening for requests at: http://127.0.0.1:4723/
Press ENTER to exit.
Exiting...
There was a problem hiding this comment.
If we close the handle, this closes stdin for the WinAppDriver process, which causes the process to shut down. That's why we need to keep it open until the process shuts down. While we could use that as a proxy to shut down WinAppDriver in a cleaner manner, we only have childStdinHandle set up if we redirect the output to a file. I'm not opposed to changing the logic but then we have no way of redirecting the output to the spawned terminal to preserve API compatibility.
| }() | ||
|
|
||
| // Read the output file. | ||
| let output = try String(contentsOfFile: outputFile, encoding: .utf16LittleEndian) |
There was a problem hiding this comment.
Does WinAppDriver write as utf-16 when redirected to a file? That's unusual
There was a problem hiding this comment.
It writes UTF-16 either way. It's usually not noticeable because Windows Terminal handles either.
There was a problem hiding this comment.
Terminals are supposed to decode stdout according to the GetConsoleOutputCP() of the console, so outputting UTF-16 would result in garbled contents. 🤔🤔🤔
There was a problem hiding this comment.
Probably witchcraft involved:
PS > $output = & 'C:\Program Files (x86)\Windows Application Driver\WinAppDriver.exe'
PS > [system.Text.Encoding]::ASCII.GetBytes($output)
87
0
105
0
[...]
PS > $output = echo potato
PS > [system.Text.Encoding]::ASCII.GetBytes($output)
112
111
116
97
116
111There was a problem hiding this comment.
I don't know if that's a valid test but it is puzzling!
This enriches the internal
Win32ProcessTreeAPI to takeHANDLEobjects for stdin, stdout and stderr, as well as adding an option to use the parent console rather than spawning a new one.