Skip to content

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

Closed
wants to merge 3 commits into from

Conversation

aisk
Copy link
Contributor

@aisk aisk commented May 4, 2025

Copy link
Member

@chris-eibl chris-eibl left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@zooba zooba left a 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>
@aisk
Copy link
Contributor Author

aisk commented May 6, 2025

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)?

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:

cp1252 cp1252
<_io.FileIO name='<stdin>' mode='rb' closefd=False> <_io.FileIO name='<stdout>' mode='wb' closefd=False>

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 @requires_resource('console') to skip it on GHA and just run it with other environments?

@aisk
Copy link
Contributor Author

aisk commented May 6, 2025

After reading #114565, I'm not sure whether @requires_resource('console')is the correct approach. It seems intended to be skipped during local development to avoid unnecessary output, and should be enabled in CI?

However, I found that it is not enabled or run in CI because the console resource is not defined in libregrtest (#133519).

If we intend to enable the console resource in CI, should we add another resource like windows-console to allow skipping this test in environments that don't have a Windows console?

@zooba
Copy link
Member

zooba commented May 6, 2025

Or we can just skip the test partway through (raise unittest.SkipTest or something like that) when we figure out that it's not a real console. If there's only a small handful of cases, then there's no real need to define a resource, and it's a legacy test, not a critical one.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a 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)
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants