SysCaptureBinary: decode in writeorg #6880
Conversation
Upstream: pytest-dev#6880 Ref (fixes): pytest-dev#6871 (cherry picked from commit d90ae38)
Upstream: pytest-dev#6880 Ref (fixes): pytest-dev#6871
Upstream: pytest-dev#6880 Ref (fixes): pytest-dev#6871
Upstream: pytest-dev#6880 Ref (fixes): pytest-dev#6871
bluetech
left a comment
bluetech
left a comment
There was a problem hiding this comment.
I looked at the Capture types. My conclusion is that there is a lot of confusion there between str/bytes and TextIO/BinaryIO. And it's exacerbates by EncodedFile which is some weird cross between the two.
I think this should be "attacked" as follows:
- Rework
EncodedFileto be sane - I do this in #6899 (though still need to investigate some CI failure). - To fix the immediate bug, integrate a version of this PR. I think it needs to consider all of
snap()andwriteorg()for all of{FD,Sys}Capture{,Binary}. - Type annotate these classes. This will reveal the faulty inheritance design which breaks Liskov Substitution and makes things very confusing (mypy will complain). Not to be committed - see next step.
- Refactor to remove the faulty inhertiance and make everything properly typed, and ensuring the tests pass.
| self._state = "resumed" | ||
|
|
||
| def writeorg(self, data): | ||
| def writeorg(self, data: str) -> None: |
There was a problem hiding this comment.
This needs to be bytes. It seems like the data = data.... confused mypy so it didn't catch it.
| def writeorg(self, data): | ||
| def writeorg(self, data: str) -> None: | ||
| data = data.decode(self._old.encoding) | ||
| self._old.write(data) |
There was a problem hiding this comment.
Maybe write to self._old.buffer instead of doing the decode? Although it might not exist in case of "nested" patching of sys.stdout and friends (e.g. if someone patches sys.stdout to a io.StringIO, which doesn't have an underlying buffer, and then tries to capture it -- probably not a proper thing to do).
2 participants
Fixes #6871.