Skip to content
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

gh-108996: fix and enable test_msvcrt #109226

Merged
merged 10 commits into from Sep 22, 2023
Merged

gh-108996: fix and enable test_msvcrt #109226

merged 10 commits into from Sep 22, 2023

Conversation

aisk
Copy link
Member

@aisk aisk commented Sep 10, 2023

Copy link
Member

@vstinner vstinner left a 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.

@@ -1,9 +1,8 @@
import ctypes
Copy link
Member

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.

sys.stdin = old_stdin
with open('CONIN$', 'rb', buffering=0) as stdin:
h = msvcrt.get_osfhandle(stdin.fileno())
ctypes.windll.kernel32.FlushConsoleInputBuffer(h)
Copy link
Member

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.

@aisk
Copy link
Member Author

aisk commented Sep 10, 2023

@vstinner Current implementation doesn't failes in latest two runs, maybe it's been fixed. But the freebsd tests failed:

1 test failed again:
    test_msvcrt

I think it's weird, the tests should not run on freebsd.

@vstinner
Copy link
Member

I think it's weird, the tests should not run on freebsd.

The test failed because ctypes is not available on this FreeBSD CI:

ModuleNotFoundError: No module named '_ctypes'

I told you, in tests, you should tolerate that ctypes is not available.

Or write a wrapper to FlushConsoleInputBuffer() in msvcrt or in _testconsole module (PC/_testconsole.c).

@aisk aisk requested a review from a team as a code owner September 11, 2023 09:28
@aisk
Copy link
Member Author

aisk commented Sep 11, 2023

@vstinner Ah sorry, just understood what you mean above...

@aisk
Copy link
Member Author

aisk commented Sep 11, 2023

This change passed the tests in the last three times, I think it's ready to review.

@aisk aisk requested a review from vstinner September 15, 2023 09:57
@aisk
Copy link
Member Author

aisk commented Sep 15, 2023

Latest run failed because of freebsd test instance out of limit, not related to this change.

Lib/test/test_msvcrt.py Outdated Show resolved Hide resolved
@zooba
Copy link
Member

zooba commented Sep 21, 2023

I don't have any further feedback. @vstinner are you happy with this?

@vstinner
Copy link
Member

!buildbot Windows

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit dfd3992 🤖

The command will test the builders whose names match following regular expression: Windows

The builders matched are:

@vstinner
Copy link
Member

!buildbot AMD64 Windows

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit dfd3992 🤖

The command will test the builders whose names match following regular expression: AMD64 Windows

The builders matched are:

  • AMD64 Windows11 Refleaks PR
  • AMD64 Windows11 Non-Debug PR
  • AMD64 Windows10 PR
  • AMD64 Windows11 Bigmem PR

@vstinner
Copy link
Member

@vstinner are you happy with this?

Let's see if buildbots are happy :-)

@vstinner
Copy link
Member

Tests passed on these 4 Windows buildbots: good.

buildbot/AMD64 Windows11 Bigmem PR — Build done.
buildbot/AMD64 Windows11 Non-Debug PR — Build done.
buildbot/ARM64 Windows Non-Debug PR — Build done.
buildbot/ARM64 Windows PR — Build done.

Example on ARM64 Windows Non-Debug PR:

0:06:33 load avg: 0.13 [382/463/1] test_msvcrt passed

@vstinner vstinner merged commit 4230d7c into python:main Sep 22, 2023
79 of 100 checks passed
@vstinner
Copy link
Member

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.

@aisk
Copy link
Member Author

aisk commented Sep 22, 2023

Thanks for the review!

@aisk aisk deleted the fix-test-msvcrt branch September 22, 2023 10:20
csm10495 pushed a commit to csm10495/cpython that referenced this pull request Sep 28, 2023
* 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants