Skip to content

gh-110147: run console io test in new process #110268

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Oct 5, 2023
Merged

gh-110147: run console io test in new process #110268

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c88252e
run console io test in new process
aisk Oct 3, 2023
32f0209
Update test_msvcrt.py
aisk Oct 3, 2023
db2b10b
Merge branch 'main' into 'test-msvcrt-process'
aisk Oct 3, 2023
7ee4a1f
remove flush_console_input_buffer
aisk Oct 3, 2023
1f6e6ca
Merge branch ''test-msvcrt-process'' of https://github.com/aisk/cpyth…
aisk Oct 3, 2023
4ecbd84
update comments
aisk Oct 3, 2023
93ca648
Merge branch 'main' into 'test-msvcrt-process'
aisk Oct 3, 2023
2e7cd6d
Update Lib/test/test_msvcrt.py
aisk Oct 3, 2023
a469eae
Update Lib/test/test_msvcrt.py
aisk Oct 3, 2023
a0d2972
Reduce codes
aisk Oct 3, 2023
44ef80a
add requires_resource
aisk Oct 3, 2023
f222132
Reduce codes and add comment
aisk Oct 3, 2023
a3d0008
Update Lib/test/test_msvcrt.py
vstinner Oct 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 28 additions & 17 deletions Lib/test/test_msvcrt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import os
import subprocess
import sys
import unittest

Expand All @@ -11,8 +12,6 @@
import _winapi
import msvcrt;

from _testconsole import write_input, flush_console_input_buffer


class TestFileOperations(unittest.TestCase):
def test_locking(self):
Expand Down Expand Up @@ -62,33 +61,45 @@ def test_get_osfhandle(self):

class TestConsoleIO(unittest.TestCase):
def test_kbhit(self):
h = msvcrt.get_osfhandle(sys.stdin.fileno())
flush_console_input_buffer(h)
self.assertEqual(msvcrt.kbhit(), 0)
# Run test in a seprated process to avoid stdin conflicts.
# See: gh-110147
cmd = [sys.executable, '-c',
'import msvcrt;'
'assert msvcrt.kbhit() == 0;'
]
subprocess.run(cmd, check=True, capture_output=True)

def test_getch(self):
msvcrt.ungetch(b'c')
self.assertEqual(msvcrt.getch(), b'c')

def test_getwch(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")?

with open('CONIN$', 'rb', buffering=0) as stdin:
h = msvcrt.get_osfhandle(stdin.fileno())
flush_console_input_buffer(h)

write_input(stdin, c_encoded)
self.assertEqual(msvcrt.getwch(), c)
# Run test in a seprated process to avoid stdin conflicts.
# See: gh-110147
cmd = [sys.executable, '-c',
'import msvcrt;'
'from _testconsole import write_input;'
'stdin = open("CONIN$", "rb", buffering=0);'
f'write_input(stdin, {c_encoded});'
f'assert msvcrt.getwch() == "{c}";'
]
subprocess.run(cmd, check=True, capture_output=True)

def test_getche(self):
msvcrt.ungetch(b'c')
self.assertEqual(msvcrt.getche(), b'c')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be safer to run this test in a subprocess with a new console, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it should work, I'll try it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def test_getwche(self):
with open('CONIN$', 'rb', buffering=0) as stdin:
h = msvcrt.get_osfhandle(stdin.fileno())
flush_console_input_buffer(h)

write_input(stdin, c_encoded)
self.assertEqual(msvcrt.getwche(), c)
# Run test in a seprated process to avoid stdin conflicts.
# See: gh-110147
cmd = [sys.executable, '-c',
'import msvcrt;'
'from _testconsole import write_input;'
'stdin = open("CONIN$", "rb", buffering=0);'
f'write_input(stdin, {c_encoded});'
f'assert msvcrt.getwche() == "{c}";'
]
subprocess.run(cmd, check=True, capture_output=True)

def test_putch(self):
msvcrt.putch(b'c')
Expand Down