Fix crash when printing while capsysbinary is active #6926
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Zac-HD
approved these changes
Mar 15, 2020
nicoddemus
reviewed
Mar 16, 2020
| @@ -591,6 +589,11 @@ def snap(self): | |||
| self.tmpfile.truncate() | |||
| return res | |||
|
|
|||
| def writeorg(self, data): | |||
| """ write to original file descriptor. """ | |||
| data = data.encode("utf-8") # XXX use encoding of original stream | |||
There was a problem hiding this comment.
Hmmm not related to this code, but curious how FDCaptureBinary still creates tmpfile with encoding="UTF-8"...
There was a problem hiding this comment.
If you mean why FDCaptureBinary wraps the temporary file with EncodedFile (== TextIOWrapper), it's for two reasons:
- In FD capturing, in addition to the FD redirection itself, it also does sys capturing. And the sys capturing requires a file object which looks like
sys.stdoutand friends, which are text-mode. - The
FDCapturesubclass uses it in itssnap()implementation.
I have some plans to untangle all of this, but thought it'd be better not to mix refactorings with bug fixes.
Previously, writing to sys.stdout/stderr in text-mode (e.g.
`print('foo')`) while a `capsysbinary` fixture is active, would crash
with:
/usr/lib/python3.7/contextlib.py:119: in __exit__
next(self.gen)
E TypeError: write() argument must be str, not bytes
This is due to some confusion in the types. The relevant functions are
`snap()` and `writeorg()`. The function `snap()` returns what was
captured, and the return type should be `bytes` for the binary captures
and `str` for the regular ones. The `snap()` return value is eventually
passed to `writeorg()` to be written to the original file, so it's input
type should correspond to `snap()`. But this was incorrect for
`SysCaptureBinary`, which handled it like `str`.
To fix this, be explicit in the `snap()` and `writeorg()`
implementations, also of the other Capture types.
We can't add type annotations yet, because the current inheritance
scheme breaks Liskov Substitution and mypy would complain. To be
refactored later.
Fixes: pytest-dev#6871
Co-authored-by: Ran Benita (some modifications & commit message)
bluetech
force-pushed
the
fix-capsysbin-print
branch
from
e3e10bd
to
1fda861
Compare
nicoddemus
approved these changes
Mar 16, 2020
|
Sorry, I just realized we have forgotten to backport this to |
|
Thanks for the great work! :) Just tested this from the current master 2d9dac9 by switching from |
bluetech
added
the
needs backport
|
Backport in #7002. Should be fixed in (upcoming) 5.4.2. |
jktjkt
added a commit
to Telecominfraproject/oopt-gnpy
that referenced
this pull request
Jun 11, 2020
I would like to avoid that extra fork to a child Python interpreter (it looks like something that can be easily avoided). It's something that's possible now that the code ships just some trivial wrappers (which are, in turn, needed for setuptools' `console_scripts`). This cannot use the `capsysbinary` fixture for wrapping of stdout/stderr due to something in pytest which already got fixed, but has not been released yet (May 2020). Let's use `capfdbinary` which works fine. Change-Id: Ic4a124a5cbe2bd24c56e4565d27d313fe4da703f See-also: pytest-dev/pytest#6926
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #6880 by replacing that PR with some modifications as described in #6880 (review) (step 2).
Previously, writing to sys.stdout/stderr in text-mode (e.g.
print('foo')) while acapsysbinaryfixture is active, would crashwith:
This is due to some confusion in the types. The relevant functions are
snap()andwriteorg(). The functionsnap()returns what wascaptured, and the return type should be
bytesfor the binary capturesand
strfor the regular ones. Thesnap()return value is eventuallypassed to
writeorg()to be written to the original file, so it's inputtype should correspond to
snap(). But this was incorrect forSysCaptureBinary, which handled it likestr.To fix this, be explicit in the
snap()andwriteorg()implementations, also of the other Capture types.
We can't add type annotations yet, because the current inheritance
scheme breaks Liskov Substitution and mypy would complain. To be
refactored later.
Fixes: #6871
Co-authored-by: Ran Benita (some modifications & commit message)