-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
gh-133374: Fix test_python_legacy_windows_stdio in test_cmd_line #133375
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect there's a good chance it's not getting real console handles in CI systems either... maybe we should also print type(sys.stdout)
(or possibly we have to dig into .buffer.raw
)?
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
Tested it on my own repo with: import sys
print(sys.stdin.encoding, sys.stdout.encoding)
print(sys.stdin.buffer.raw, sys.stdout.buffer.raw) And the result is:
Instead of what I've got on my local machine: utf-8 utf-8
<_io._WindowsConsoleIO mode='rb' closefd=False> <_io._WindowsConsoleIO mode='wb' closefd=False> So I think there is no real consoles on GHA, and we can't run this case on it. I guess we can just add |
After reading #114565, I'm not sure whether However, I found that it is not enabled or run in CI because the If we intend to enable the |
Or we can just skip the test partway through ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not test the stderr encoding.
We should use other way to pass data from subprocess. For example, temporary file or shared file descriptor (if this is supported on Windows).
out = p.stderr.read() | ||
p.stderr.close() | ||
p.wait() | ||
self.assertNotIn(b'utf-8', out) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not be true. It's entirely possible to set the default encoding to UTF-8 in Windows itself, at which point the legacy stdio will also be UTF-8.
Checking the type of the stdin/stdout objects in the subprocess is probably the best way. The real purpose of this environment variable is to go back to using FileIO
rather than _WindowsConsoleIO
, and the encoding is only a side-effect of that.
And if we just make the child process assert
, then we can check the exit code rather than reading the output.
test_python_legacy_windows_stdio
intest_cmd_line
isn't actually working #133374