GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-41818: Make test_openpty() avoid unexpected success due to number of rows and/or number of columns being == 0. by 8vasu · Pull Request #23526 · python/cpython

Conversation

@8vasu

@8vasu 8vasu commented Nov 26, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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 running

./python -c "import pty;pty.spawn('./python -m test -v test_pty'.split())"

unexpected 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 26, 2020
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Nov 26, 2020
@8vasu

8vasu commented Nov 26, 2020

Copy link
Copy Markdown
Contributor Author

@zware thank you for suggesting that helpful command. @asvetlov @ethanfurman here is my proposed proper fix.
zware reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@8vasu 8vasu mentioned this pull request Nov 27, 2020
bpo-41818: test_openpty succeed on Gentoo, don't expect to fail on this platform #23514
Merged
@asvetlov

asvetlov commented Nov 27, 2020

Copy link
Copy Markdown
Contributor

Let me check on the buildbot fleet. Just in case.
8vasu reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@asvetlov asvetlov added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 27, 2020
@bedevere-bot

bedevere-bot commented Nov 27, 2020

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 27, 2020
asvetlov
asvetlov requested changes Nov 27, 2020
View reviewed changes

@asvetlov asvetlov left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks good, a tiny polishing is needed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_pty.py Outdated

@asvetlov asvetlov Nov 27, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Drop this comment please, it is not required anymore

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@8vasu 8vasu Nov 27, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_pty.py Outdated

@asvetlov asvetlov Nov 27, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please add a few words to describe the resize trick and why it is necessary.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@8vasu 8vasu Nov 27, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I have added some comments. Please let me know if this looks good.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Nov 27, 2020
@bedevere-bot

bedevere-bot commented Nov 27, 2020

Copy link
Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@8vasu
Make test_openpty() avoid unexpected success due to number of rows an…
3e2a4cc 
…d/or number of columns being == 0.

Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu
8vasu force-pushed the test_openpty_fix branch from 1178d94 to 3e2a4cc Compare November 27, 2020 09:14
@8vasu

8vasu commented Nov 27, 2020

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again. Also, I have rebased so that there is only one clean commit.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Nov 27, 2020
@bedevere-bot

bedevere-bot commented Nov 27, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from asvetlov November 27, 2020 09:16
@blurb-it
📜🤖 Added by blurb_it.
278a7f8
asvetlov
asvetlov approved these changes Nov 27, 2020
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Nov 27, 2020
@asvetlov
asvetlov merged commit f5a19ea into python:master Nov 27, 2020
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 27, 2020
@asvetlov

asvetlov commented Nov 27, 2020

Copy link
Copy Markdown
Contributor

Cool, thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@8vasu

8vasu commented Nov 27, 2020

Copy link
Copy Markdown
Contributor Author

You're welcome!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@8vasu

8vasu commented Nov 27, 2020

Copy link
Copy Markdown
Contributor Author

The next pull request will make some additions to the tty module as preparation for making changes to pty.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@asvetlov

asvetlov commented Nov 27, 2020

Copy link
Copy Markdown
Contributor

@8vasu please also take a look at test_master_read() failure on Solaris reported by https://bugs.python.org/issue41818#msg381930

We need to suppress the error for the problematic platform at least if there is no idea why SunOS behaves differently.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@8vasu

8vasu commented Nov 27, 2020

Copy link
Copy Markdown
Contributor Author

@asvetlov I am working on it now :) I have posted a comment in the bpo issue about the details.
asvetlov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Nov 27, 2020
bpo-41818: test_master_read() in test_pty.py should be expected to fail on every platform that is not Linux; that includes Solaris. #23533
Closed
bpo-41818: Fix test_master_read() so that it succeeds on all platforms that either raise OSError or return b"" upon reading from master #23536
Merged
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@8vasu @adorilson
bpo-41818: Make test_openpty() avoid unexpected success due to number…
a571483 
… of rows and/or number of columns being == 0. (pythonGH-23526)
@8vasu 8vasu mentioned this pull request Aug 12, 2021
bpo-41818: Add termios.tcgetwinsize(), termios.tcsetwinsize(). Update docs. #23686
Merged
@8vasu 8vasu mannequin mentioned this pull request May 5, 2022
Lib/pty.py major revision #85984
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asvetlov asvetlov asvetlov approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@8vasu @asvetlov @bedevere-bot @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.