-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Make capsys more like stdio streams in python3. Resolves #1407. #2265
Make capsys more like stdio streams in python3. Resolves #1407. #2265
Conversation
This seems to break python3.3, which is a bit perplexing! Lemme see if I can't figure out why (though python3.3 is getting pretty old (end of lifed in 2014)). |
Neat: I fixed py33 by making |
Not sure if this should go to |
Is it not a bug that capsys incorrectly mocks stdout / stderr (why I decided to PR master)? |
It probably is - however, it seems to me like there's quite some potential of breakage in some corner-cases here (like the one you already saw above 😉), and we're trying to avoid regressions in patch releases. History shows that usually, something weird ends up breaking without the pytest testsuite catching it, as pytest has a lot of users 😉 However, I don't have any strong feelings about this - let's see what others think. |
Looks fine to me. |
@asottile could you please take a look at the Windows tests? I agree with @The-Compiler that we should merge this into |
Fixed windows, it seems a little silly to be afraid to release bug fixes but it's not my project so I'll do as you please. I'm currently blocked on this fix, when is your next release? |
Hmm can you provide an example which demonstrates the problem without the fix? @RonnyPfannschmidt do you think this should go into |
There's a couple in my original issue: #1407 |
f = capture.CaptureIO() | ||
# In python3, stdout / stderr are text io wrappers (exposing a buffer | ||
# property of the underlying bytestream). | ||
getattr(f, 'buffer', f).write(b'foo\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it'd pass even if the object doesn't have a buffer
attribute (i.e. without any code changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> import io
>>> f = io.StringIO()
>>> getattr(f, 'buffer', f).write(b'foo\n')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: string argument expected, got 'bytes'
Nope! Thankfully io.StringIO
requires text (the prior implementation)
I found a workaround with |
Thanks for submitting a PR, your contribution is really appreciated!
Here's a quick checklist that should be present in PRs:
master
; for new features, targetfeatures
;Unless your change is trivial documentation fix (e.g., a typo or reword of a small section) please:
AUTHORS
;CHANGELOG.rst
CHANGELOG
, so please add a thank note to yourself ("Thanks @user for the PR") and a link to your GitHub profile. It may sound weird thanking yourself, but otherwise a maintainer would have to do it manually before or after merging instead of just using GitHub's merge button. This makes it easier on the maintainers to merge PRs.