gh-110147: run console io test in new process #110268
Conversation
|
Latest 4 tests passed https://github.com/python/cpython/actions/workflows/build.yml?query=branch%3A%27test-msvcrt-process%27 Except https://github.com/python/cpython/actions/runs/6392768647 failed with |
Lib/test/test_msvcrt.py
Outdated
| import msvcrt | ||
| assert msvcrt.kbhit() == 0 | ||
| ''') | ||
| self.run_in_separated_process(code) | ||
|
|
||
| def test_getch(self): | ||
| msvcrt.ungetch(b'c') | ||
| self.assertEqual(msvcrt.getch(), b'c') | ||
|
|
||
| def test_getwch(self): |
There was a problem hiding this comment.
test_getwche() looks like a copy/paste, you just replaced getwch() with getwche(). Can you rename the function to check_getwch(func), so test_getwch() would call check_getwch("getwch") and test_getwch() would call check_getwch("getwche")?
| write_input(stdin, {repr(c_encoded)}) | ||
| assert msvcrt.getwch() == "{c}" | ||
| ''') | ||
| self.run_in_separated_process(code) | ||
|
|
||
| def test_getche(self): | ||
| msvcrt.ungetch(b'c') | ||
| self.assertEqual(msvcrt.getche(), b'c') |
There was a problem hiding this comment.
It may be safer to run this test in a subprocess with a new console, no?
There was a problem hiding this comment.
Yes it would, but it's annoying. We might want to put it behind a resource marker, as the new console will pop up new GUI when it runs.
There was a problem hiding this comment.
Would CREATE_NO_WINDOW flag prevent popups?
The process is a console application that is being run without a console window. Therefore, the console handle for the application is not set. This flag is ignored if the application is not a console application, or if it is used with either CREATE_NEW_CONSOLE or DETACHED_PROCESS.
says: https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags
There was a problem hiding this comment.
Seems it should work, I'll try it.
There was a problem hiding this comment.
the console handle for the application is not set
Means no console and the test is irrelevant.
ignored if ... used with either CREATE_NEW_CONSOLE
This is the flag that you'd use to make the test more reliable, and it's incompatible with CREATE_NO_WINDOW.
There was a problem hiding this comment.
Ok, and added the @requires_resource('gui') marker to avoid the console pop-up.
For the tests test_getche and test_getch, they uses msvcrt.ungetch instead of the magic write_input, I think this stuff is stable enough, this test file is added and disabled days ago, so I fixed it serveral times, and never saw them failed once. So I think we should keep it (test_getche / test_getch) as current.
There was a problem hiding this comment.
For the tests test_getche and test_getch, they uses msvcrt.ungetch instead of the magic write_input, I think this stuff is stable enough
Hum, ok. Let's see if CI will agree with you :-) I'm fine with keeping them in the same process for now.
Co-authored-by: Victor Stinner <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
|
@aisk: Please, don't touch anything, I will schedule buildbot jobs to test your PR :-) |
|
!buildbot Windows |
|
🤖 New build scheduled with the buildbot fleet by @vstinner for commit a3d0008 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
|
test_msvcrt passed on the two GHA Windows jobs and on the 6 buildbots. So it looks good to me.
These errors are unrelated and can be ignored. |
|
Merged, thanks @aisk for the fix! |
Uh oh!
There was an error while loading. Please reload this page.