Skip to content
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.

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

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
merged 2 commits into from
Nov 28, 2020

Conversation

8vasu
Copy link
Contributor

@8vasu 8vasu commented Nov 28, 2020

This replaces #23533. Build was failing on Solaris since, like the BSDs and Darwin, Solaris does not raise OSError in test_master_read(). Therefore, now we make test_master_read() succeed on all platforms that either raise OSError [ such as Linux ] or return b"" [ such as BSDs, Darwin, and possibly Solaris ] upon reading from master when the slave is closed. Any platform that does not exhibit such behavior can now be detected using test_master_read(). On any given platform, exiting from pty.spawn()'s copy loop depends on our exact knowledge of the behavior of test_master_read() on that platform.

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

https://bugs.python.org/issue41818

either raise OSError or return b"" upon reading from master

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu
Copy link
Contributor Author

8vasu commented Nov 28, 2020

@asvetlov this follows #23526. My original solution was #23533, but this is much better.

Edit: to be extra safe, I request you to run the buildbots again.

Copy link
Contributor

@kulikjak kulikjak left a comment

Choose a reason for hiding this comment

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

With this patch, test_pty on Solaris is green again.

@asvetlov asvetlov added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 28, 2020
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @asvetlov for commit f994d09 🤖

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 28, 2020
@asvetlov
Copy link
Contributor

to be extra safe, I request you to run the buildbots again.

Done

@asvetlov
Copy link
Contributor

Thanks!

@8vasu
Copy link
Contributor Author

8vasu commented Nov 28, 2020

You're welcome!

@8vasu
Copy link
Contributor Author

8vasu commented Nov 29, 2020

@asvetlov #23546 is the next one. Of course, you can take your time to review it :D The tty module will now need a documentation update; I have not done that yet. Also, please let me know if I need to make tests for the tty module. After we finish working on that one, and before starting to make changes to the pty module, I will add one more nice test for the pty module which will check if pty.spawn() is capable of adjusting slave window size dynamically [ SIGWINCH ].

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
…s that either raise OSError or return b"" upon reading from master (pythonGH-23536)

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu 8vasu mannequin mentioned this pull request May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants