bpo-41818: Make test_openpty() avoid unexpected success due to number of rows and/or number of columns being == 0. #23526
Conversation
|
@zware thank you for suggesting that helpful command. @asvetlov @ethanfurman here is my proposed proper fix. |
|
Let me check on the buildbot fleet. Just in case. |
asvetlov
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @asvetlov for commit 1178d94873b7a72bcc1426cd4db7d525d7db1cec 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
Lib/test/test_pty.py
Outdated
| # Should not apply @unittest.expectedFailure() on Gentoo | ||
| # to keep the buildbot fleet happy. | ||
| return fun | ||
| # PLATFORM = platform.system() |
There was a problem hiding this comment.
Drop this comment please, it is not required anymore
| target_stdin_winsz = struct.pack("HHHH", self.stdin_rows//5, | ||
| self.stdin_cols//5, 0, 0) | ||
| debug(f"original size: (rows={self.stdin_rows}, cols={self.stdin_cols})") | ||
| target_stdin_rows = self.stdin_rows + 1 |
There was a problem hiding this comment.
Please add a few words to describe the resize trick and why it is necessary.
There was a problem hiding this comment.
I have added some comments. Please let me know if this looks good.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
…d/or number of columns being == 0. Signed-off-by: Soumendra Ganguly <[email protected]>
8vasu
force-pushed
the
test_openpty_fix
branch
from
1178d94
to
3e2a4cc
Compare
|
I have made the requested changes; please review again. Also, I have rebased so that there is only one clean commit. |
|
Thanks for making the requested changes! @asvetlov: please review the changes made to this pull request. |
|
Cool, thanks! |
|
You're welcome! |
|
The next pull request will make some additions to the tty module as preparation for making changes to pty. |
|
@8vasu please also take a look at We need to suppress the error for the problematic platform at least if there is no idea why SunOS behaves differently. |
|
@asvetlov I am working on it now :) I have posted a comment in the bpo issue about the details. |
… of rows and/or number of columns being == 0. (pythonGH-23526)
Proposed proper fix for buildbot failures introduced after #22962 merging.
To check if
pty.openpty()properly sets pty slave size based on the size of current stdin,PtyTest.test_openpty()was modifying the size of current stdin first. Let 'x' be the number of rows of stdin; then, we set the number of rows of stdin to some f(x) such that the function f does not have any fixed points; that is, f(x) != x for all x. In #22962, f(x) := x//5; unfortunately, in that case, f does have 0 as a fixed point [ 0//5 = 0 ]. Upon runningunexpected success was reported on Debian; this indeed was due to the # of (rows, cols) being (0,0). The Gentoo buildbot was probably also running with stdin being a tty of size (0,0). The new choice of f(x) := x+1 solves the problem, since this f has no fixed points [ x+1=x has no solution. ]
TL;DR: instead of letting rows and cols of stdin be the floor of 1/5 of their original values, we now only increment their respective values by 1. Note, however, that the former is a more noticeable change when # of rows, cols are fairly large.
Signed-off-by: Soumendra Ganguly [email protected]
https://bugs.python.org/issue41818