bpo-41818: Updated tests for the standard pty library by 8vasu · Pull Request #22962 · python/cpython · GitHub

This converts the test for pty.master_open() to a test for pty.openpty() with additional tests such as checking slave termios, slave winsize; these additional tests are expected to fail.

This also adds a small test that is expected to fail on FreeBSD and should pass on Linux; reflecting pty.spawn()'s FreeBSD hang issue [ bpo-26228 ].

Solutions to these problems have been implemented here: https://github.com/8vasu/pypty2

If and when these tests are acknowledged, the solutions will be submitted in parts.

This follows the conversation here: #21861

Signed-off-by: Soumendra Ganguly soumendraganguly@gmail.com

https://bugs.python.org/issue41818

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

@ethanfurman @aeros

Sir, thank you very much for being patient. Please run the tests when you have time. I welcome suggestions/criticism. I have also sent a message to the teaching email thread.

…FILENO is a tty Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

…nBSD Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

🤖 New build scheduled with the buildbot fleet by @gpshead for commit 2a3cba4d8e906af233fc22b1be648bae05e7f08b 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

i kicked off a test on the buildbots for some additional platform coverage out out curiosity. Note: our buildbot fleet may generate a lot of noise, they aren't all reliably and some were already failing on the main branch to begin with right now. so don't fret if it claims there are failures. only look for obviously related things within them if you go looking at those. :)

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

@gpshead I have removed those comments :)

I've completed a preliminary review of the test changes. Most of the suggested changes are fairly minor, for the most part I think this is good to go (although for the final approval/merge, I'd prefer to leave it to @ethanfurman or @Yhg1s since I'm no expert with pty or tty).

Shouldn't this at least result in a warning via warnings.warn()? I suspect it would get buried otherwise.

You are absolutely correct. I will change this. I have one question before I change this. If I use warnings.warn() here, then it will always generate warnings when the test is not run with stdin being a tty; for example the automated tests done by GitHub. Is that going to be an issue?

It may cause a slight bit of extra noise on the tests, but it shouldn't cause the GitHub CI to fail when the warning is emitted (I don't believe we have any of our CI set to fail on warning, but it can be ran with that setting locally).

This existing whitespace line should be re-added, since generally there are two lines in between the last line of a class's body and functions after it.

Thank you for teaching me this. I will make this change.

The news entry should optimally refer to the tests that were changed (in this case test_opentty was updated and a new test_master_read() was added.

Sure!

I don't think this comment adds any additional context for the reader, and can be removed.

Gotcha. I did this because the test_fork() function also has a similar comment at the end thereof. Should I remove that one as well?

Maybe @ethanfurman or @Yhg1s could clarify, but I don't think we should add a new "RELEVANT ANYMORE?" comment.

Thank you for noticing this. I agree that they are annoying; I apologize for this. Yes, I need clarification. The plan was to remove the annoying "RELEVANT ANYMORE?" comments after clarification.

I myself actually did not put much thought into the blocks that I marked with "RELEVANT ANYMORE?"; I will think about them in greater detail on Saturday. They were just supposed to be there as reminders; anyway, they do not hurt the tests :)

Aah. Now I realize why I added the extra "RELEVANT ANYMORE" here. Notice that the following comment mentions pty.master_open(), which opens the slave and subsequently closes it. As a result, the original test_basic() function was having to call pty.slave_open() to re-open the slave. However, the new test_openpty() calls pty.openpty(), which, unlike pty.master_open(), does not close the slave end. Anyway, that part does not seem to affect the test on Linux and FreeBSD.

Could we potentially surround with a try...finally to avoid this? A single test failure should really not result in the other tests within test_pty being unreliable.

Good point. Thank you. I will work on this.

@aeros I am grateful for the reviews. I will keep working on this with the hope that we can finally put these old bugs to rest.

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

@aeros Thank you for being patient. I have made the following changes as per your suggestions:

I had added a comment saying that the tty could be left in an abnormal state [ only winsize, not termios/terminfo ]. I have solved the problem by adding an addCleanup(). I have also renamed some variables, added functions [ _get_term_winsz() and _set_term_winsz() ] to increase readability.
I have changed the debug() in the except clause of the second try-except block in test_openpty() to a warnings.warn() as per your suggestion.
I realized why I added the extra "RELEVANT ANYMORE" comment. The following comment mentions pty.master _open(), which opens the slave and subsequently closes it. As a result, the original test_basic() function was having t
o call pty.slave_open() to re-open the slave. However, the new test_openpty() calls pty.openpty(), which, unlike pty.ma ster_open(), does not close the slave end. Anyway, that part does not seem to affect the test on Linux and FreeBSD.
I have removed the "RELEVANT ANYMORE" comments.
Removed the "pty.openpty() passed" and "pty.fork() passed" comments.
Replaced % with f-strings. However, I have not trimmed down the debug messages yet because test_fork() has similar debug messages in it. I will remove them all at once if you wish.
Added the existing empty line between class and subsequent function definition.

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

I've been away from the code, so this might be a dumb question, but can't we determine if stdin is a tty?

Sorry for the late reply. That is actually a great question. tcgetattr() can fail due to 2 reasons: errno == EBADF ( not a valid file descriptor ), errno == ENOTTY ( not a tty ). See https://pubs.opengroup.org/onlinepubs/9699919799/. In either case, your stdin is not a tty. In fact, all glibc's isatty() does is call tcgetattr() to see if it fails; see https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/isatty.c.

I have verified that Linux, FreeBSD, and NetBSD follow this approach for their isatty() implementations, while OpenBSD calls fcntl()+F_ISATTY. Anyway, the tcgetattr() approach works on all the above platforms. I avoid isatty(), which is often followed by a separate tcgetattr() call, thereby avoiding an extra tcgetattr() call [ OCD :D ].

In our code, if tcgetattr() fails, then either pty.STDIN_FILENO, which is 0, is a valid fd but not a tty (ENOTTY), or it is not a valid fd at all (EBADF). EBADF will not happen unless something closes 0 [ uncommon scenario ]; for example, by using [ p close(0) ] in gdb, or if your script itself voluntarily closes 0 for some reason.

The "possible" in the comment # possible reason: current stdin is not a tty is not because of lack of confidence, but because I did not want to discount the (uncommon) chance of an EBADF, in which case 0 is not a valid fd at all [ OCD ]. Anyway, that comment can be safely removed. Do you want me to remove it?

I would say update it:

# possible reason ... to # not a tty or bad/closed fd

and, of course, make that change throughout.

Done.

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

LGTM, I think we can merge the PR and unblock @8vasu to work on future PTY improvements.

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Installed with X 3.x has failed when building commit c13d899.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/464/builds/445) and take a look at the build logs.
Check if the failure is related to this commit (c13d899) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/464/builds/445

Failed tests:

test_pty

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

411 tests OK.

1 test failed:
test_pty

13 tests skipped:
test_asdl_parser test_check_c_globals test_clinic test_devpoll
test_gdb test_ioctl test_kqueue test_msilib test_startfile
test_winconsoleio test_winreg test_winsound test_zipfile64

1 re-run test:
test_pty

Total duration: 28 min 6 sec

Click to see traceback logs

remote: Enumerating objects: 18, done.        
remote: Counting objects:   5% (1/18)        
remote: Counting objects:  11% (2/18)        
remote: Counting objects:  16% (3/18)        
remote: Counting objects:  22% (4/18)        
remote: Counting objects:  27% (5/18)        
remote: Counting objects:  33% (6/18)        
remote: Counting objects:  38% (7/18)        
remote: Counting objects:  44% (8/18)        
remote: Counting objects:  50% (9/18)        
remote: Counting objects:  55% (10/18)        
remote: Counting objects:  61% (11/18)        
remote: Counting objects:  66% (12/18)        
remote: Counting objects:  72% (13/18)        
remote: Counting objects:  77% (14/18)        
remote: Counting objects:  83% (15/18)        
remote: Counting objects:  88% (16/18)        
remote: Counting objects:  94% (17/18)        
remote: Counting objects: 100% (18/18)        
remote: Counting objects: 100% (18/18), done.        
remote: Compressing objects:  10% (1/10)        
remote: Compressing objects:  20% (2/10)        
remote: Compressing objects:  30% (3/10)        
remote: Compressing objects:  40% (4/10)        
remote: Compressing objects:  50% (5/10)        
remote: Compressing objects:  60% (6/10)        
remote: Compressing objects:  70% (7/10)        
remote: Compressing objects:  80% (8/10)        
remote: Compressing objects:  90% (9/10)        
remote: Compressing objects: 100% (10/10)        
remote: Compressing objects: 100% (10/10), done.        
remote: Total 10 (delta 8), reused 1 (delta 0), pack-reused 0        
From https://github.com/python/cpython
 * branch                  master     -> FETCH_HEAD
Reset branch 'master'

	not a dynamic executable

	not a dynamic executable
	not a dynamic executable
  WARNING: The script easy_install-3.10 is installed in '/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/bin' which is not on PATH.
  Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location.
  WARNING: The scripts pip3 and pip3.10 are installed in '/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/bin' which is not on PATH.
  Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location.

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Non-Debug with X 3.x has failed when building commit c13d899.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/58/builds/442) and take a look at the build logs.
Check if the failure is related to this commit (c13d899) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/58/builds/442

Failed tests:

test_pty

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

414 tests OK.

10 slowest tests:

test_peg_generator: 7 min 5 sec
test_tokenize: 3 min 45 sec
test_concurrent_futures: 3 min 45 sec
test_multiprocessing_spawn: 2 min 58 sec
test_asyncio: 2 min 39 sec
test_lib2to3: 2 min 31 sec
test_unparse: 2 min 25 sec
test_multiprocessing_forkserver: 1 min 54 sec
test_multiprocessing_fork: 1 min 21 sec
test_io: 1 min 15 sec

1 test failed:
test_pty

10 tests skipped:
test_devpoll test_gdb test_ioctl test_kqueue test_msilib
test_startfile test_winconsoleio test_winreg test_winsound
test_zipfile64

1 re-run test:
test_pty

Total duration: 35 min 23 sec

Click to see traceback logs

remote: Enumerating objects: 18, done.        
remote: Counting objects:   5% (1/18)        
remote: Counting objects:  11% (2/18)        
remote: Counting objects:  16% (3/18)        
remote: Counting objects:  22% (4/18)        
remote: Counting objects:  27% (5/18)        
remote: Counting objects:  33% (6/18)        
remote: Counting objects:  38% (7/18)        
remote: Counting objects:  44% (8/18)        
remote: Counting objects:  50% (9/18)        
remote: Counting objects:  55% (10/18)        
remote: Counting objects:  61% (11/18)        
remote: Counting objects:  66% (12/18)        
remote: Counting objects:  72% (13/18)        
remote: Counting objects:  77% (14/18)        
remote: Counting objects:  83% (15/18)        
remote: Counting objects:  88% (16/18)        
remote: Counting objects:  94% (17/18)        
remote: Counting objects: 100% (18/18)        
remote: Counting objects: 100% (18/18), done.        
remote: Compressing objects:  10% (1/10)        
remote: Compressing objects:  20% (2/10)        
remote: Compressing objects:  30% (3/10)        
remote: Compressing objects:  40% (4/10)        
remote: Compressing objects:  50% (5/10)        
remote: Compressing objects:  60% (6/10)        
remote: Compressing objects:  70% (7/10)        
remote: Compressing objects:  80% (8/10)        
remote: Compressing objects:  90% (9/10)        
remote: Compressing objects: 100% (10/10)        
remote: Compressing objects: 100% (10/10), done.        
remote: Total 10 (delta 8), reused 1 (delta 0), pack-reused 0        
From https://github.com/python/cpython
 * branch                  master     -> FETCH_HEAD
Reset branch 'master'

	not a dynamic executable

	not a dynamic executable

The test fails with 'unexpected success' error, perhaps expectedFailure() should not be applied for this configuration.
Investigating.

Or perhaps Gentoo is one of the cases where it succeeds, and the the expected failure wrapper should not be applied in that case.

#23514 is created for fix

Updated tests for the pty standard library

323fc9c

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

the-knights-who-say-ni added the CLA signed label Oct 25, 2020

bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Oct 25, 2020

8vasu mentioned this pull request Oct 25, 2020

bpo-41541: Make pty.spawn set window size #21861

Closed

In Lib/test/test_pty.py, wrap @expectedfailure to check if pty.STDIN_…

4bb22ca

…FILENO is a tty Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

8vasu force-pushed the new_pty_tests branch from 057f14b to 4bb22ca Compare October 25, 2020 07:36

In Lib/test/test_pty.py, add a check for "Darwin" in expectedFailureO…

a8723e7

…nBSD Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

8vasu force-pushed the new_pty_tests branch from 10f2db4 to a8723e7 Compare October 25, 2020 08:37

gpshead added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 26, 2020

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 26, 2020

gpshead reviewed Oct 26, 2020

View reviewed changes

Comment thread Lib/test/test_pty.py Outdated

gpshead requested review from aeros and ethanfurman October 26, 2020 06:10

Remove some comments.

26c0928

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

8vasu force-pushed the new_pty_tests branch from 2a3cba4 to 26c0928 Compare October 26, 2020 14:16

aeros reviewed Oct 26, 2020

View reviewed changes

aeros requested a review from Yhg1s October 30, 2020 18:03

Restructure the tests, perform cleanup.

9a6e70f

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

8vasu force-pushed the new_pty_tests branch from c431bd0 to 9a6e70f Compare November 2, 2020 21:47

8vasu added 3 commits November 2, 2020 16:16

Restructure test_openpty().

f6d01f8

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

Restructure test_openpty().

85f173b

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

Restructure test_openpty().

6546938

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

ethanfurman reviewed Nov 17, 2020

View reviewed changes

Remove/change some comments.

0cde3f2

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>

8vasu force-pushed the new_pty_tests branch from 4a3c962 to 0cde3f2 Compare November 20, 2020 14:39

📜🤖 Added by blurb_it.

e990b79

asvetlov approved these changes Nov 24, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Nov 24, 2020

asvetlov merged commit c13d899 into python:master Nov 25, 2020

bedevere-bot removed the awaiting merge label Nov 25, 2020

asvetlov mentioned this pull request Nov 25, 2020

bpo-41818: test_openpty succeed on Gentoo, don't expect to fail on this platform #23514

Merged

8vasu mentioned this pull request Nov 26, 2020

bpo-41818: Make test_openpty() avoid unexpected success due to number of rows and/or number of columns being == 0. #23526

Merged

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021

bpo-41818: Updated tests for the standard pty library (pythonGH-22962 )

267725e

8vasu mentioned this pull request Aug 12, 2021

bpo-41818: Add termios.tcgetwinsize(), termios.tcsetwinsize(). Update docs. #23686

Merged

8vasu mannequin mentioned this pull request May 5, 2022

Lib/pty.py major revision #85984

Open

bpo-41818: Updated tests for the standard pty library by 8vasu · Pull Request #22962 · python/cpython · GitHub

Repository navigation

bpo-41818: Updated tests for the standard pty library - #22962

bpo-41818: Updated tests for the standard pty library#22962asvetlov merged 10 commits intopython:masterpython/cpython:masterfrom 8vasu:new_pty_tests8vasu/cpython:new_pty_testsCopy head branch name to clipboard

Conversation

8vasu commented Oct 25, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

8vasu commented Oct 25, 2020

Uh oh!

bedevere-bot commented Oct 26, 2020

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

8vasu commented Oct 26, 2020

Uh oh!

aeros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

8vasu commented Oct 27, 2020

Uh oh!

8vasu commented Nov 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 25, 2020

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

bedevere-bot commented Nov 25, 2020

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

asvetlov commented Nov 25, 2020

Uh oh!

ethanfurman commented Nov 25, 2020

Uh oh!

asvetlov commented Nov 25, 2020

bpo-41818: Updated tests for the standard pty library#22962
asvetlov merged 10 commits into
python:masterfrom
8vasu:new_pty_tests

8vasu commented Oct 25, 2020 •
edited by bedevere-bot
Loading