Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add returncode argument to pytest.exit #4145

Merged
merged 12 commits into from
Oct 15, 2018
Merged

Conversation

cacoze
Copy link
Contributor

@cacoze cacoze commented Oct 14, 2018

Solves #4098

If the argument is not None, it'll raise a SystemExit exception to
cleanly exit pytest.
It checks that a SystemError was raised and the SystemError code
is the same as the returncode argument.
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cacoze for the PR!

Please take a look at my comments. 👍

if returncode:
raise SystemExit(returncode)
else:
raise Exit(msg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should always raise Exit, but pass the argument along to it: Exit(msg, returncode), and treat it accordingly in main.py:

pytest/src/_pytest/main.py

Lines 185 to 188 in 7e1fac5

if initstate < 2 and isinstance(excinfo.value, exit.Exception):
sys.stderr.write("{}: {}\n".format(excinfo.typename, excinfo.value.msg))
config.hook.pytest_keyboard_interrupt(excinfo=excinfo)
session.exitstatus = EXIT_INTERRUPTED

Like this:

existstatus = EXIT_INTERRUPTED
if initstate < 2 and isinstance(excinfo.value, exit.Exception):
    sys.stderr.write("{}: {}\n".format(excinfo.typename, excinfo.value.msg))
    exitstatus = excinfo.value.exitstatus
config.hook.pytest_keyboard_interrupt(excinfo=excinfo)
session.exitstatus = exitstatus

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please update the docstring for pytest.exit

@@ -570,6 +570,15 @@ def pytest_configure(config):
result.stderr.fnmatch_lines(["Exit: oh noes"])


def test_pytest_exit_returncode():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following my suggestion above, this will need to be changed to a test using testdir, something like:

def test_pytest_exit_returncode(testdir):
    testdir.makepyfile("""
        import pytest
        def test_foo():
            pytest.exit("some exit msg", 99)
    """)
    result = testdir.runpytest()
    assert result.ret == 99

@@ -0,0 +1 @@
Add to pytest.exit a returncode argument to cleanly exit pytest.
Copy link
Member

@nicoddemus nicoddemus Oct 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this reads better:

Add returncode argument to pytest.exit() to exit pytest with a specific return code.

@coveralls
Copy link

coveralls commented Oct 14, 2018

Coverage Status

Coverage increased (+0.02%) to 93.782% when pulling 27d932e on labcodes:4098 into 0be84cd on pytest-dev:features.

@codecov
Copy link

codecov bot commented Oct 14, 2018

Codecov Report

Merging #4145 into features will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #4145      +/-   ##
============================================
+ Coverage     94.45%   94.49%   +0.04%     
============================================
  Files           109      109              
  Lines         24169    24177       +8     
  Branches       2381     2382       +1     
============================================
+ Hits          22829    22847      +18     
+ Misses         1021     1015       -6     
+ Partials        319      315       -4
Flag Coverage Δ
#linux 94.34% <100%> (+0.04%) ⬆️
#nobyte 0% <0%> (ø) ⬆️
#numpy 28.21% <15.38%> (+0.05%) ⬆️
#pexpect 0% <0%> (ø) ⬆️
#py27 92.64% <100%> (+0.04%) ⬆️
#py34 92.07% <100%> (+0.03%) ⬆️
#py35 92.09% <100%> (+0.03%) ⬆️
#py36 92.65% <100%> (+0.04%) ⬆️
#py37 92.29% <100%> (+0.03%) ⬆️
#trial 31.31% <15.38%> (+0.05%) ⬆️
#windows 93.76% <100%> (ø) ⬆️
#xdist 18.92% <15.38%> (+0.01%) ⬆️
Impacted Files Coverage Δ
src/_pytest/outcomes.py 95.94% <100%> (+0.05%) ⬆️
testing/test_runner.py 96.84% <100%> (+0.02%) ⬆️
src/_pytest/main.py 96.22% <100%> (+0.02%) ⬆️
src/_pytest/terminal.py 91.62% <0%> (+1.74%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0be84cd...27d932e. Read the comment docs.

@cacoze
Copy link
Contributor Author

cacoze commented Oct 14, 2018

Thanks for the feedback @nicoddemus . I'm still learning about the project and had no idea that I could use _pytest/main.py to set the return code from the test.

src/_pytest/main.py Show resolved Hide resolved
@@ -49,18 +49,23 @@ class Failed(OutcomeException):
class Exit(KeyboardInterrupt):
""" raised for immediate program exits (no tracebacks/summaries)"""

def __init__(self, msg="unknown reason"):
def __init__(self, returncode=None, msg="unknown reason"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add returncode to the end of the parameter list to keep backward compatibility; technically users should not raise this explicitly, but it doesn't hurt to keep compatibility in this case.

src/_pytest/outcomes.py Show resolved Hide resolved
src/_pytest/main.py Show resolved Hide resolved
"""
Exit testing process as if KeyboardInterrupt was triggered.

:param int returncode: return code to be used when exiting pytest..
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're at it, please add a description for the msg parameter as well:

:param str msg: message to display upon exit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there's an extra period at the end of this sentence.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! 😁

@crazymerlyn crazymerlyn merged commit 141c51f into pytest-dev:features Oct 15, 2018
@blueyed
Copy link
Contributor

blueyed commented Oct 22, 2018

Just a btw from reading commits on master: squashing this would have made a lot of sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants