| msg191854 - (view) |
Author: R. David Murray (r.david.murray) *  |
Date: 2013-06-25 12:31 |
| A look at a random selection of tests that use script_helper indicates that using universal_newlines=True would either simplify or make no difference to the usage of the script_helper assert_python functions in the majority of the tests that use it. 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.) |
|
|
| msg228595 - (view) |
Author: Mark Lawrence (BreamoreBoy) * |
Date: 2014-10-05 17:35 |
| Just a gentle reminder guys. |
|
|
| msg293762 - (view) |
Author: Pranav Deshpande (Pranav Deshpande) |
Date: 2017-05-16 13:22 |
| Hello, I would like to work on this issue. Could you guide me on so as how to proceed with this? |
|
|
| msg293792 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-05-16 21:20 |
| I really hate script_helper API: keyword arguments are only used to pass environment variables, so the function uses hackish parameteres like __cleanenv=True... I would prefer to pass keywords unchanged to Popen() to be able to use universal_newlines=True for example. Since we have +10k tests, I suggest to not touch the existing API but add yet another API: a new function in support/script_helper.py. What do you think? |
|
|
| msg293794 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-05-16 21:25 |
| > 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? |
|
|
| msg293963 - (view) |
Author: Pranav Deshpande (Pranav Deshpande) |
Date: 2017-05-19 18:07 |
| I am afraid I didn't make myself clear. I am a beginner when it comes to open source contribution and have decided to take up this issue. I did some basic research about the issue and found this file: cpython/Lib/test/support/script_helper.py The function _assert_python calls run_python_until_end which calls subprocess.Popen which takes the parameter universal_newlines=True. That is all I could discover. Could you guide me further regarding this? |
|
|
| msg344748 - (view) |
Author: Hai Shi (shihai1991) *  |
Date: 2019-06-05 16:51 |
| >it's a pain to extend the API (I don't want to use yet another __xxx custom keyword) Adding __xxx in run_python_until_end function would increase the complexity but it looks like a unified function. |
|
|
| msg344975 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-06-07 17:30 |
| I dislike script_helper API. I doesn't allow to pass arbitrary keyword parameters to subprocess.Popen. IMHO a new API should be added, but I didn't check if script_helper already contains such API or not. |
|
|
| msg345489 - (view) |
Author: Hai Shi (shihai1991) * |
Date: 2019-06-13 09:51 |
| spawn_python in script_helper is good enough, so this bug looks like cloud be closed. |
|
|