-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-108996: fix and enable test_msvcrt #109226
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.
I would prefer to start easy: reenable the whole file, but still skip tests known to fail, and then in a following PR, fix these tests. So we can spend more time to test in length the second PR.
Lib/test/test_msvcrt.py
Outdated
@@ -1,9 +1,8 @@ | |||
import ctypes |
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.
ctypes is not always available. Please use try: import ctypes except ImportError: ctypes = None
, and then skip tests using ctypes if ctypes is None.
Lib/test/test_msvcrt.py
Outdated
sys.stdin = old_stdin | ||
with open('CONIN$', 'rb', buffering=0) as stdin: | ||
h = msvcrt.get_osfhandle(stdin.fileno()) | ||
ctypes.windll.kernel32.FlushConsoleInputBuffer(h) |
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.
Maybe it should be exposed in msvcrt, maybe as a private function just for testing.
@vstinner Current implementation doesn't failes in latest two runs, maybe it's been fixed. But the freebsd tests failed:
I think it's weird, the tests should not run on freebsd. |
The test failed because ctypes is not available on this FreeBSD CI:
I told you, in tests, you should tolerate that ctypes is not available. Or write a wrapper to FlushConsoleInputBuffer() in |
@vstinner Ah sorry, just understood what you mean above... |
This change passed the tests in the last three times, I think it's ready to review. |
Latest run failed because of freebsd test instance out of limit, not related to this change. |
I don't have any further feedback. @vstinner are you happy with this? |
!buildbot Windows |
!buildbot AMD64 Windows |
🤖 New build scheduled with the buildbot fleet by @vstinner for commit dfd3992 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
Let's see if buildbots are happy :-) |
Tests passed on these 4 Windows buildbots: good.
Example on ARM64 Windows Non-Debug PR:
|
Merged, thanks @aisk for your contribution. Let's see how it goes this time :-) I'm not surprised that these tests failed. It's hard to write reliable tests on a terminal :-( Calling FlushConsoleInputBuffer() seems to make tests more reliable: good! I enhanced/completed the commit message. |
Thanks for the review! |
* Add _testconsole.flush_console_input_buffer() function. * test_kbhit(), test_getwch() and test_getwche() now call flush_console_input_buffer(). * Don't override sys.stdin anymore (not needed).
* Add _testconsole.flush_console_input_buffer() function. * test_kbhit(), test_getwch() and test_getwche() now call flush_console_input_buffer(). * Don't override sys.stdin anymore (not needed).
More details: #109004