Skip to content

gh-85984: Utilize new "winsize" functions from termios in pty tests. - #101831

Merged
gpshead merged 5 commits into
python:mainfrom
8vasu:test_pty_winsize
Feb 12, 2023
Merged

gh-85984: Utilize new "winsize" functions from termios in pty tests.#101831
gpshead merged 5 commits into
python:mainfrom
8vasu:test_pty_winsize

Conversation

@8vasu

@8vasu 8vasu commented Feb 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This follows #92365. This is one in a series of PRs aimed at cleaning-up, fixing bugs in, introducing new features in, and updating the code in "Lib/pty.py".

This is a dependency of #101833.

#101832 is another dependency of #101833.

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

@bedevere-bot bedevere-bot mentioned this pull request Feb 11, 2023
Open
@bedevere-bot bedevere-bot added the tests Tests in the Lib/test dir label Feb 11, 2023
@8vasu 8vasu mentioned this pull request Feb 11, 2023
Closed
@8vasu

ghost commented Feb 11, 2023

Copy link
Copy Markdown
Contributor Author

@gpshead Thank you for being a major part of this project for so long. This is another PR for you. There is no rush; please take a look at this when you are free.

8vasu
8vasu commented Feb 11, 2023
View reviewed changes
Comment thread Lib/test/test_pty.py Outdated

# We expect the function to return without error.
self.assertEqual(pty._copy(masters[0]), None)
# def test__copy_eof_on_all(self):

ghost Feb 11, 2023 •
edited by 8vasu
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

test__copy_eof_on_all probably does not make sense due to the current structure of the _copy() function. Please correct me if I am wrong.

ghost Feb 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agreed, this test doesn't make sense as written, please delete it.

ghost Feb 12, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Soumendra Ganguly
Utilize new functions termios.tcgetwinsize() and termios.tcsetwinsize…
63fb4d8 
… in test_pty.py.

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Comment thread Lib/test/test_pty.py Resolved
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Comment thread Lib/test/test_pty.py Resolved
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Comment thread Lib/test/test_pty.py Resolved
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Comment thread Lib/test/test_pty.py Resolved
gpshead
gpshead reviewed Feb 11, 2023
View reviewed changes
Comment thread Lib/test/test_pty.py Resolved
gpshead
gpshead requested changes Feb 11, 2023
View reviewed changes
Comment thread Lib/test/test_pty.py Outdated

# We expect the function to return without error.
self.assertEqual(pty._copy(masters[0]), None)
# def test__copy_eof_on_all(self):

ghost Feb 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agreed, this test doesn't make sense as written, please delete it.

@bedevere-bot

ghost commented Feb 11, 2023

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.

@gpshead gpshead self-assigned this Feb 11, 2023
Soumendra Ganguly added 2 commits February 11, 2023 20:07
Soumendra Ganguly
Remove import_module("fcntl") and test__copy_eof_on_all().
cd0742c 
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
Soumendra Ganguly
Merge branch 'test_pty_winsize' of https://github.com/8vasu/cpython
4ae71ad 
Get the News entry.
@8vasu

ghost commented Feb 12, 2023

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

@bedevere-bot

ghost commented Feb 12, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

@bedevere-bot
bedevere-bot requested a review from gpshead February 12, 2023 02:44
@gpshead

ghost commented Feb 12, 2023

Copy link
Copy Markdown
Member

!buildbot wasm32

@bedevere-bot

ghost commented Feb 12, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @gpshead for commit 4ae71ad 🤖

The command will test the builders whose names match following regular expression: wasm32

The builders matched are:

  • wasm32-wasi PR
  • wasm32-emscripten node (dynamic linking) PR
  • wasm32-emscripten node (pthreads) PR
  • wasm32-emscripten browser (dynamic linking, no tests) PR

@gpshead

ghost commented Feb 12, 2023

Copy link
Copy Markdown
Member

!buildbot wasm32

@bedevere-bot

ghost commented Feb 12, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @gpshead for commit aaff7ce 🤖

The command will test the builders whose names match following regular expression: wasm32

The builders matched are:

  • wasm32-wasi PR
  • wasm32-emscripten node (dynamic linking) PR
  • wasm32-emscripten node (pthreads) PR
  • wasm32-emscripten browser (dynamic linking, no tests) PR

@gpshead

ghost commented Feb 12, 2023

Copy link
Copy Markdown
Member

(specifically it was the wasm32 encripten node bots that crashed on test_pty)

@gpshead
gpshead merged commit da2fb92 into python:main Feb 12, 2023
@8vasu

ghost commented Feb 12, 2023

Copy link
Copy Markdown
Contributor Author

@gpshead Thank you for the final change. Some info: maybe the reason why it failed with both the Python fcntl module and the current termios.tc{get/set}winsize() functions is that ioctl()+TIOCGWINSZ/TIOCSWINSZ is problematic on wasm32-emcripten. When I googled "emscripten ioctl", this was the very second link: emscripten-core/emscripten#6463.

@8vasu 8vasu mentioned this pull request Mar 4, 2023
Merged
@ambv ambv added the needs backport to 3.11 only security fixes label May 19, 2023
@miss-islington

ghost commented May 19, 2023

Copy link
Copy Markdown
Contributor

Thanks @8vasu for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington

ghost commented May 19, 2023

Copy link
Copy Markdown
Contributor

Sorry @8vasu and @gpshead, I had trouble checking out the 3.11 backport branch.
Please retry by removing and re-adding the "needs backport to 3.11" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker da2fb9264315dc30ac3012b4dbf5ba76d3f34433 3.11

@ambv ambv added needs backport to 3.11 only security fixes and removed needs backport to 3.11 only security fixes labels May 19, 2023
@miss-islington

ghost commented May 19, 2023

Copy link
Copy Markdown
Contributor

Thanks @8vasu for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot

ghost commented May 19, 2023

Copy link
Copy Markdown

GH-104652 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead Awaiting requested review from gpshead

Assignees

@gpshead gpshead

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 @gpshead @miss-islington @ambv