Refactor Capture classes: move/rename CaptureIO classes#6765
Refactor Capture classes: move/rename CaptureIO classes#6765bluetech merged 1 commit intopytest-dev:masterfrom
Conversation
blueyed
force-pushed
the
capture-refactor-1
branch
2 times, most recently
from
db5feae to
f2a3b1d
Compare
blueyed
changed the title
bluetech
left a comment
bluetech
left a comment
There was a problem hiding this comment.
Makes sense to me to move from compat.
I left a couple of comments on the other changes.
src/_pytest/capture.py
Outdated
| return self.buffer.getvalue().decode("UTF-8") | ||
|
|
||
|
|
||
| class PassthroughCaptureIO(CaptureIO): |
There was a problem hiding this comment.
I think the previous name was more clear. This new name sounds like a "no-op capture" (i.e. just passthough without capturing).
Alternative name would be TeeCaptureIO.
There was a problem hiding this comment.
TeeCaptureIO sounds good - I've mainly wanted to have CaptureIO at the end.
src/_pytest/capture.py
Outdated
| else: | ||
| tmpfile = CaptureAndPassthroughIO(self._old) | ||
| self.tmpfile = tmpfile | ||
| def __init__(self, fd: int) -> None: |
There was a problem hiding this comment.
I'm not sure this is really better, still duplicates some logic, and now it relies on SysCaptureBinary implementation instead of being independent.
Two possible alternatives, both are not ideal too but a bit better IMO:
- Add a
passthrough: boolargument toSysCaptureBinaryand usePassthroughCaptureIOif true. - Add class attribute
CaptureIOClass: Type[CaptureIO] = CaptureIOtoSysCaptureBinaryand use that in__init__, and override toPassthroughCaptureIOinTeeSysCapture.
There was a problem hiding this comment.
Well, I think it is quite simple/good.
Keep in mind that the "old" is only used for an assert, and can be removed.
As for your 2nd suggestion: this is basically what passing in tmpfile does, isn't it?
There was a problem hiding this comment.
Keep in mind that the "old" is only used for an assert, and can be removed.
You mean _old right?
As for your 2nd suggestion: this is basically what passing in tmpfile does, isn't it?
<offtopic>
The tmpfile argument exists for by FDCaptureBinary. I was surprised initially to see that FDCaptureBinary uses SysCapture under the hood. Turns out FDCaptureBinary not only redirects the FDs to a tempfile, it also additionally patches the sys file objects to redirect to the tempfile (this is where the argument is needed). It seemed redundant to me -- if we already redirect the FDs, there is no need to patch sys since the data will reach the tempfile anyway. So I tried to remove the sys patching from FDCaptureBinary. It actually works fine, but the problem is the flushing - if we don't patch sys, then data that remains in the file object buffer doesn't end up captured, i.e. an explicit flush() is sometimes needed before the capture fixture can be examined.
If backward compatibility wasn't a concern, I think that this is actually the better behavior for capfd. The implicit flush()s prevent a test from being able to verify that data is actually present in the underlying file at a point of time.
But, we can't break it now... So maybe it makes sense to add a new "fdonly" mode, and make fd an alias for "fd+sys" mode (for clarity).
</offtopic>
TL;DR: I think the tmpfile argument should go away eventually, so wouldn't want to add new uses for it...
|
So I think moving the Capture classes over to So to move forward on this PR, I'll remove the Tee changes, but keep the move. |
Move {Passthrough,CaptureIO} to capture module, and rename Passthrough
-> Tee to match the existing terminology.
Co-authored-by: Ran Benita <ran@unusedvar.com>
bluetech
force-pushed
the
capture-refactor-1
branch
from
f2a3b1d to
7647d1c
Compare
2 participants
Pulled out of #6671