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

gh-116402: Avoid readline in test_builtin TTY input tests #122447

Merged
merged 5 commits into from
Jul 30, 2024

Conversation

ambv
Copy link
Contributor

@ambv ambv commented Jul 30, 2024

The intention here is to make the TTY input() tests run in as many scenarios as possible. For this to happen, we need to temporarily remove the function pointer set to call_readline in PyInit_readline(). This pointer is normally NULL without readline and PyOS_Readline uses a basic implementation of its own in this case. This is what we want.

We could try and avoid importing readline in tests, but we have played this whack-a-mole for a long time. For instance, now that pdb uses rlcompleter, test_builtin imports that always (since it imports doctest in load_tests() and doctest imports pdb as it subclasses it). I think we should stop trying to not load readline, and just detach its function temporarily. This avoids discussions of "unimporting" readline, which would be problematic as PyInit_readline() sets global state on the C side through setup_readline.

While CI is famously not run with a TTY and those tests will be skipped there, people running tests sequentially from their terminal will now be able to have those tests run in many more situations than before.

I did the function pointer unsetting with ctypes for simplicity. If that's unavailable, the context manager simply skips the tests.

@ambv
Copy link
Contributor Author

ambv commented Jul 30, 2024

@Eclips4, can you confirm this fixes the issue for you?

@Eclips4
Copy link
Member

Eclips4 commented Jul 30, 2024

@Eclips4, can you confirm this fixes the issue for you?

Yes! Thanks for the fix!

@ambv ambv added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 30, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @ambv for commit fa61261 🤖

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 30, 2024
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, this also fixes test_builtin for me locally.

Lib/test/test_builtin.py Outdated Show resolved Hide resolved
Lib/test/test_builtin.py Show resolved Hide resolved
@ambv
Copy link
Contributor Author

ambv commented Jul 30, 2024

All buildbots (including refleaks and bigmem) passed save for "iOS ARM64 Simulator" with an unrelated failure. Rebasing on main with the try:finally: suggestion and landing as soon as CI passes.

@ambv ambv enabled auto-merge (squash) July 30, 2024 16:33
@ambv ambv merged commit 1d8e453 into python:main Jul 30, 2024
32 checks passed
@miss-islington-app
Copy link

Thanks @ambv for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 30, 2024
…onGH-122447)

(cherry picked from commit 1d8e453)

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
@bedevere-app
Copy link

bedevere-app bot commented Jul 30, 2024

GH-122472 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 30, 2024
ambv added a commit that referenced this pull request Jul 30, 2024
…122447) (GH-122472)

(cherry picked from commit 1d8e453)

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
@ambv ambv deleted the gh-116402 branch October 7, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants