Message293794
> Even if there turn out to be a few uses where having bytes is required, it would probably be worth catering to the common case and making those exceptional cases use Popen directly. (Or alternatively provide assert_python_xxx_binary helpers.)
I would prefer the opposite: *always* use script_helper rather than Popen() directly in tests. We have to check why some tests use directly Popen?
I know that in test_faulthandler for example, I chose to use directly Popen because script_helper always enable faulthandler, there is no option to disable this behaviour, and as I wrote in my previous comment, it's a pain to extend the API (I don't want to use yet another __xxx custom keyword).
Maybe we need differently API levels in script helper, the lowest level would return a Popen object but add -I, -E and/or -X faulthandler.
By the way, I'm using more and more functions like the one I added to Lib/test/eintrdata/eintr_tester.py:
@contextlib.contextmanager
def kill_on_error(proc):
"""Context manager killing the subprocess if a Python exception is raised."""
with proc:
try:
yield proc
except:
proc.kill()
raise
Such helper should also be moved to script_helper.
For examle, in my perf project I have these two helper functions:
@contextlib.contextmanager
def popen_killer(proc):
try:
yield
except:
# Close pipes
if proc.stdin:
proc.stdin.close()
if proc.stdout:
proc.stdout.close()
if proc.stderr:
proc.stderr.close()
try:
proc.kill()
except OSError:
# process already terminated
pass
proc.wait()
raise
def popen_communicate(proc):
with popen_killer(proc):
return proc.communicate()
Or maybe we should add back these features directly into the subprocess module? |
|
Date |
User |
Action |
Args |
2017-05-16 21:25:39 | vstinner | set | recipients:
+ vstinner, barry, r.david.murray, BreamoreBoy, Pranav Deshpande |
2017-05-16 21:25:39 | vstinner | set | messageid: <1494969939.54.0.0751433582497.issue18299@psf.upfronthosting.co.za> |
2017-05-16 21:25:39 | vstinner | link | issue18299 messages |
2017-05-16 21:25:39 | vstinner | create | |
|