Closed rmartin16 closed 6 months ago
The only other issue I noticed is a weird one - the code clearly has 100% coverage, but I can't see how the
capture_output = False
case is being exercised. I'm guessing it's being tested implicitly by another test somewhere, but given it's at the core of how this works, it would be worth having an explicit test.
Would you consider the test_output()
test in tests/integrations/subprocess/test_PopenOutputStreamer.py
to cover this case? Or maybe something more?
To make sure I'm understanding what is going on here - the key to the problem is that the old Popen usage wasn't ever invoking
readline()
, which apparently causes Windows to lock up and not produce output. The code now always uses a background thread to poll and collect stdout (not just for thestream_output()
case). The Android usage previously only usedcommunicate()
to gather output in the case of an error, and otherwise ignored output. The new implementation does continuousreadline()
polling to get output as it is generated, which should (hopefully 🤞) unblock Windows.Have I got that right?
Right....the theory is if we read stdout
from the emulator, it'll stop blocking it from properly opening for users.
More broadly, this provides the option of creating a Popen
and instead of only being able to call Subprocess.stream_output()
to read stdout
while blocking, there's now Subprocess.stream_output_non_blocking()
to (obviously) not block while reading stdout
.
The only other issue I noticed is a weird one - the code clearly has 100% coverage, but I can't see how the
capture_output = False
case is being exercised. I'm guessing it's being tested implicitly by another test somewhere, but given it's at the core of how this works, it would be worth having an explicit test.Would you consider the
test_output()
test intests/integrations/subprocess/test_PopenOutputStreamer.py
to cover this case? Or maybe something more?
Ah - that was the case I was missing. It's relying on the default value of the argument.
Have I got that right?
Right....the theory is if we read
stdout
from the emulator, it'll stop blocking it from properly opening for users.
👍
More broadly, this provides the option of creating a
Popen
and instead of only being able to callSubprocess.stream_output()
to readstdout
while blocking, there's nowSubprocess.stream_output_non_blocking()
to (obviously) not block while readingstdout
.
Yeah - that's a nice API addition. Not sure if we'll have any use for it, but it's a nice club to have in the bag just in case, and the rest of the refactoring around adding that API are all nice improvements as well.
Changes
stdout
resolves the issuePopenOutputStreamer
for encapsulating the output streamThread
and adds the ability to capture instead of print outputSubprocess.stream_output_non_blocking()
alongside the existingSubprocess.steam_output()
to facilitate callers streaming (and conditionally capturing output) while doing other tasksPR Checklist: