Skip to content

gh-131178: Use support.captured_stdout and support.captured_stderr #137748

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

gh-131178: Use support.captured_stdout and support.captured_stderr #137748

wants to merge 5 commits into from
+7 −17

Conversation

donBarbos
Copy link
Contributor

@donBarbos donBarbos commented Aug 14, 2025 •
edited by bedevere-app bot
Loading

I replaced contextlib.redicrect_std{out,err} and support.captured_output("std{out,err}") with support.captured_std{out,err}

Inspired by #131287 (comment). I went beyond older PRs, I thought that in other tests this change is also relevant

@bedevere-app bedevere-app bot mentioned this pull request Aug 14, 2025
Open
9 tasks
picnixz
picnixz reviewed Aug 14, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Just change the recent CLI tests, not the rest. It produces too much churn otherwise. Serhiy was also against a global change

@donBarbos
Copy link
Contributor Author

Just change the recent CLI tests, not the rest. It produces too much churn otherwise. Serhiy was also against a global change

Ok, and I found that test_unknown_flag in test_platform has assertStartsWith inside assertRaises, I hope you don't mind fixing it in the current PR

@picnixz
Copy link
Member

picnixz commented Aug 14, 2025

Mmh, I really prefer separate commits as we could eventually not decide to change the capture_stdout() (I'll need to see the final changes and see if it's really a gain as there is also a backport question).

There won't be an opposition to fixing the test but there might be slight opposition in refactoring. I am travelling now so I suggest fixing the test first, pinging a core dev such as Adam Turner for the merge and wait for me to come back for the capture_stdout() change.

@inventshah
Copy link
Contributor

See #136632 for a previous discussion about changing tests to use the support.captured_* helpers.

@serhiy-storchaka
Copy link
Member

This is a code churn. Please leave it as is.

@AA-Turner AA-Turner closed this Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@picnixz picnixz picnixz left review comments

@rhettinger rhettinger Awaiting requested review from rhettinger

@gaogaotiantian gaogaotiantian Awaiting requested review from gaogaotiantian

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland

@Eclips4 Eclips4 Awaiting requested review from Eclips4 Eclips4 is a code owner

@tomasr8 tomasr8 Awaiting requested review from tomasr8 tomasr8 is a code owner

@pablogsal pablogsal Awaiting requested review from pablogsal pablogsal is a code owner

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner

@ambv ambv Awaiting requested review from ambv

@savannahostrowski savannahostrowski Awaiting requested review from savannahostrowski

Assignees
No one assigned
Labels
awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@donBarbos @picnixz @inventshah @serhiy-storchaka @AA-Turner