Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
aisk wants to merge3 commits intopython:mainfromaisk:fix-cmd-line-test

Conversation

aisk
Copy link
Contributor

@aiskaisk commentedMay 4, 2025
edited by bedevere-appbot
Loading

Copy link
Member

@chris-eiblchris-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

@zoobazooba 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 printtype(sys.stdout) (or possibly we have to dig into.buffer.raw)?

Co-authored-by: Steve Dower <steve.dower@microsoft.com>
@aisk
Copy link
ContributorAuthor

aisk commentedMay 6, 2025
edited
Loading

I suspect there's a good chance it's not getting real console handles in CI systems either... maybe we should also printtype(sys.stdout) (or possibly we have to dig into.buffer.raw)?

Tested it onmy own repo with:

importsysprint(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-8utf-8<_io._WindowsConsoleIOmode='rb'closefd=False><_io._WindowsConsoleIOmode='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
ContributorAuthor

aisk commentedMay 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 theconsole resource is not defined inlibregrtest (#133519).

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

@zooba
Copy link
Member

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-storchakaserhiy-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. Thereal purpose of this environment variable is to go back to usingFileIO rather than_WindowsConsoleIO, and the encoding is only a side-effect of that.

And if we just make the child processassert, then we can check the exit code rather than reading the output.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@zoobazoobazooba left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka left review comments

@chris-eiblchris-eiblchris-eibl approved these changes

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@aisk@zooba@serhiy-storchaka@chris-eibl@methane

[8]ページ先頭

©2009-2025 Movatter.jp