This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Change script_helper to use universal_newlines=True in _assert_python
Type: enhancement Stage: patch review
Components: Versions: Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Pranav Deshpande, barry, ceronman, r.david.murray
Priority: normal Keywords: easy, patch

Created on 2013-06-25 12:31 by r.david.murray, last changed 2022-04-11 14:57 by admin.

Pull Requests
URL Status Linked Edit
PR 13847 closed shihai1991, 2019-06-05 17:11
PR 13908 closed shihai1991, 2019-06-08 06:54
Messages (9)
msg191854 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python triager) 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) * (Python committer) 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) * (Python triager) Date: 2019-06-13 09:51
spawn_python in script_helper is good enough, so this bug looks like cloud be closed.
History
Date User Action Args
2022-04-11 14:57:47adminsetgithub: 62499
2019-06-23 14:18:12shihai1991setnosy: - shihai1991
2019-06-13 12:09:00vstinnersetnosy: - vstinner
2019-06-13 09:51:07shihai1991setmessages: + msg345489
2019-06-08 06:54:15shihai1991setpull_requests: + pull_request13781
2019-06-07 17:30:31vstinnersetmessages: + msg344975
2019-06-05 17:11:49shihai1991setkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request13724
2019-06-05 16:51:37shihai1991setnosy: + shihai1991
messages: + msg344748
2019-02-24 21:05:18ceronmansetnosy: + ceronman
2017-05-19 18:07:51Pranav Deshpandesetmessages: + msg293963
2017-05-17 10:57:08BreamoreBoysetnosy: - BreamoreBoy
2017-05-16 21:25:39vstinnersetmessages: + msg293794
2017-05-16 21:20:14vstinnersetnosy: + vstinner
messages: + msg293792
2017-05-16 13:22:11Pranav Deshpandesetnosy: + Pranav Deshpande
messages: + msg293762
2014-10-05 17:35:09BreamoreBoysetnosy: + BreamoreBoy

messages: + msg228595
versions: + Python 3.5, - Python 3.4
2013-06-25 12:40:56barrysetnosy: + barry
2013-06-25 12:31:30r.david.murraycreate