◐ Shell
reader mode source ↗
Skip to content

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
asvetlov merged 2 commits into
python:masterfrom
8vasu:fix_test_master_read
Nov 28, 2020
Merged

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
asvetlov merged 2 commits into
python:masterfrom
8vasu:fix_test_master_read

Conversation

@8vasu

@8vasu 8vasu commented Nov 28, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

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

@8vasu
Fix test_master_read() so that it succeeds on all platforms that …
a550369 
either raise OSError or return b"" upon reading from master

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Nov 28, 2020
@8vasu

8vasu commented Nov 28, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

kulikjak
kulikjak approved these changes Nov 28, 2020
View reviewed changes

@kulikjak kulikjak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

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

bedevere-bot commented Nov 28, 2020

Copy link
Copy Markdown

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

@asvetlov

asvetlov commented Nov 28, 2020

Copy link
Copy Markdown
Contributor

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

Done

@asvetlov asvetlov merged commit 74311ae into python:master Nov 28, 2020
@asvetlov

asvetlov commented Nov 28, 2020

Copy link
Copy Markdown
Contributor

Thanks!

@8vasu

8vasu commented Nov 28, 2020

Copy link
Copy Markdown
Contributor Author

You're welcome!

@8vasu

8vasu commented Nov 29, 2020

Copy link
Copy Markdown
Contributor Author

@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 ].

@8vasu 8vasu mentioned this pull request Nov 29, 2020
Closed
This was referenced Dec 8, 2020
Merged
Closed
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@8vasu @adorilson
bpo-41818: Fix test_master_read() so that it succeeds on all platform… …
1779aa3 
…s that either raise OSError or return b"" upon reading from master (pythonGH-23536)

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu 8vasu mentioned this pull request Nov 20, 2021
Merged
@8vasu 8vasu mannequin mentioned this pull request May 5, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asvetlov asvetlov

+1 more reviewer

@kulikjak kulikjak kulikjak approved these changes

Reviewers whose approvals may not affect merge requirements

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.

5 participants

@8vasu @bedevere-bot @asvetlov @kulikjak @the-knights-who-say-ni