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

8vasu · 2020-10-25T06:47:20Z

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 [email protected]

https://bugs.python.org/issue41818

Signed-off-by: Soumendra Ganguly <[email protected]>

8vasu · 2020-10-25T07:11:38Z

@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 <[email protected]>

…nBSD Signed-off-by: Soumendra Ganguly <[email protected]>

bedevere-bot · 2020-10-26T06:05:10Z

🤖 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.

gpshead

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. :)

Lib/test/test_pty.py

Signed-off-by: Soumendra Ganguly <[email protected]>

8vasu · 2020-10-26T14:21:36Z

@gpshead I have removed those comments :)

aeros

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).

Lib/test/test_pty.py

aeros · 2020-10-26T20:55:31Z

Lib/test/test_pty.py

+                                 "pty.STDIN_FILENO window size unchanged")
+            except OSError:
+                # pty.STDIN_FILENO not a tty?
+                debug("Failed to set pty.STDIN_FILENO window size")


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).

aeros · 2020-10-26T20:58:57Z

Lib/test/test_pty.py

@@ -318,7 +407,6 @@ def test__copy_eof_on_all(self):
        with self.assertRaises(IndexError):
            pty._copy(masters[0])

-


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.

aeros · 2020-10-26T21:00:37Z

Misc/NEWS.d/next/Library/2020-10-26-14-17-44.bpo-41818.5MgaEc.rst

@@ -0,0 +1 @@
+Updated tests for the pty library.


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.

aeros · 2020-10-26T21:01:15Z

Lib/test/test_pty.py

+                                current_stdin_winsz.columns, 0, 0)
+            fcntl.ioctl(pty.STDIN_FILENO, TIOCSWINSZ, winsz)
+
+        # pty.openpty() passed.


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?

aeros · 2020-10-26T21:02:59Z

Lib/test/test_pty.py


+        # RELEVANT ANYMORE?


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.

aeros · 2020-10-26T21:06:34Z

Lib/test/test_pty.py



 # Marginal testing of pty suite. Cannot do extensive 'do or fail' testing
 # because pty code is not too portable.
 # XXX(nnorwitz):  these tests leak fds when there is an error.
+# Soumendra: test_openpty may leave tty in abnormal state upon failure.


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.

8vasu · 2020-10-27T03:03:21Z

@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 <[email protected]>

8vasu · 2020-11-02T22:12:54Z

@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 <[email protected]>

ethanfurman · 2020-11-17T22:05:31Z

Lib/test/test_pty.py

-            debug("Got slave_fd '%d'" % slave_fd)
+            mode = tty.tcgetattr(pty.STDIN_FILENO)
+        except tty.error:
+            # possible reason: current stdin is not a tty


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.

Signed-off-by: Soumendra Ganguly <[email protected]>

asvetlov

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

bedevere-bot · 2020-11-25T14:23:58Z

⚠️⚠️⚠️ 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.

bedevere-bot · 2020-11-25T14:36:25Z

⚠️⚠️⚠️ 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

asvetlov · 2020-11-25T14:49:17Z

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

ethanfurman · 2020-11-25T14:50:55Z

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

asvetlov · 2020-11-25T15:07:25Z

#23514 is created for fix

Updated tests for the pty standard library

323fc9c

Signed-off-by: Soumendra Ganguly <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

Lib/test/test_pty.py Outdated Show resolved Hide resolved

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

Remove some comments.

26c0928

Signed-off-by: Soumendra Ganguly <[email protected]>

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 <[email protected]>

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 <[email protected]>

Restructure test_openpty().

85f173b

Signed-off-by: Soumendra Ganguly <[email protected]>

Restructure test_openpty().

6546938

Signed-off-by: Soumendra Ganguly <[email protected]>

ethanfurman reviewed Nov 17, 2020

View reviewed changes

Remove/change some comments.

0cde3f2

Signed-off-by: Soumendra Ganguly <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

8vasu commented Oct 25, 2020 •

edited by bedevere-bot

Loading

8vasu commented Oct 25, 2020

bedevere-bot commented Oct 26, 2020

gpshead left a comment

8vasu commented Oct 26, 2020

aeros left a comment

aeros Oct 26, 2020

8vasu Oct 27, 2020

aeros Oct 30, 2020

aeros Oct 26, 2020

8vasu Oct 27, 2020

aeros Oct 26, 2020

8vasu Oct 27, 2020

aeros Oct 26, 2020

8vasu Oct 27, 2020

aeros Oct 26, 2020

8vasu Oct 27, 2020

8vasu Oct 27, 2020

8vasu Nov 2, 2020

aeros Oct 26, 2020

8vasu Oct 27, 2020

8vasu commented Oct 27, 2020

8vasu commented Nov 2, 2020

ethanfurman Nov 17, 2020

8vasu Nov 19, 2020

ethanfurman Nov 20, 2020

8vasu Nov 20, 2020

asvetlov left a comment

bedevere-bot commented Nov 25, 2020

bedevere-bot commented Nov 25, 2020

asvetlov commented Nov 25, 2020

ethanfurman commented Nov 25, 2020

asvetlov commented Nov 25, 2020

		@@ -318,7 +407,6 @@ def test__copy_eof_on_all(self):
		with self.assertRaises(IndexError):
		pty._copy(masters[0])

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

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

Conversation

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

8vasu commented Oct 25, 2020

bedevere-bot commented Oct 26, 2020

gpshead left a comment

Choose a reason for hiding this comment

8vasu commented Oct 26, 2020

aeros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

8vasu commented Oct 27, 2020

8vasu commented Nov 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov left a comment

Choose a reason for hiding this comment

bedevere-bot commented Nov 25, 2020

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

bedevere-bot commented Nov 25, 2020

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

asvetlov commented Nov 25, 2020

ethanfurman commented Nov 25, 2020

asvetlov commented Nov 25, 2020

8vasu commented Oct 25, 2020 •

edited by bedevere-bot

Loading