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 |
| 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")?
|
|
||
| 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.
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! |
4 participants
Uh oh!
There was an error while loading. Please reload this page.