Display exit code with InvocationErrors #760

ederag · 2018-02-02T23:07:54Z

This is a PR for @blueyed patch (Issue #290), supplemented with tests.
For exit codes larger than 128, a hint is given: on unix systems this often means a fatal error.
(Not always, since an application might exit with a large custom error code)

Not showing this hint for windows platform was considered, but discarded to keep code short and
simplify tests.

Before:

ERROR: InvocationError: '/home/ederag/share/prog/python/tox/.tox/exitcode/bin/python3.6 -c import sys; sys.exit(5)'

After:

ERROR: InvocationError for command '/home/ederag/share/prog/python/tox/.tox/exitcode/bin/python3.6 -c import sys; sys.exit(5)' (exited with code 5)

or, for an exit code larger than 128:

ERROR: InvocationError for command '/home/ederag/share/prog/python/tox/.tox/exitcode/bin/python3.6 -c import sys; sys.exit(129)' (exited with code 129)
Note: On unix systems, an exit code larger than 128 often means a fatal error (e.g. 139=128+11: segmentation fault)

To generate these outputs, a section was added to tox.ini:

[testenv:exitcode]
basepython = python3.6
description = commands with several exit codes
skip_install = True
commands = python3.6 -c "import sys; sys.exit(5)"

and run with python3 -m tox -e exitcode.
The modified section is too specific to be included in the PR, isn't it ?

Tests have been added to test_z_cmdline.py.
Safely replacing these tests with faster ones (mock) would require someone experienced with this technique.

The documentation might be completed (still have to think about it).
Probably add a section "Understanding InvocationError exit codes"
in example/general.html
with links to pytest exit codes, to http://www.faqs.org/docs/abs/HTML/exitcodes.html,
and explaining that no exit code is shown when the process has been aborted (like Qt does).
And for good measure, giving a link to ignoring-a-command-exit-code.

Contribution checklist:

wrote descriptive pull request text
added/updated test(s)
updated/extended the documentation
added relevant issue keyword
in message body
added news fragment in changelog folder
- fragment name: <issue number>.<type>.rst for example (588.bugfix.rst)
- <type> is must be one of bugfix, feature, deprecation,breaking, doc, misc
- if pr has no issue: consider creating one first or change it to the pr number after creating the pr
- "sign" fragment with "by @"
- please use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files - by @superuser."
- also see examples
added yourself to CONTRIBUTORS (preserving alphabetical order)

Issue tox-dev#290. Co-authored-by: Daniel Hahler <git@thequod.de>

codecov · 2018-02-02T23:31:51Z

Codecov Report

Merging #760 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #760      +/-   ##
==========================================
- Coverage   94.88%   94.82%   -0.06%     
==========================================
  Files          11       11              
  Lines        2386     2397      +11     
==========================================
+ Hits         2264     2273       +9     
- Misses        122      124       +2

Impacted Files	Coverage Δ
tox/__init__.py	`95.12% <100%> (+1.78%)`	⬆️
tox/_pytestplugin.py	`95.55% <0%> (-0.89%)`	⬇️

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 a65a1c1...6632a5f. Read the comment docs.

asottile · 2018-02-04T01:42:14Z

tox/__init__.py

+            try:
+                self.exitcode = args[1]
+            except IndexError:
+                self.exitcode = None


I think this would be a little more clear as:

def __init__(self, command, exitcode=None): super(Error, self).__init__(command, exitcode) self.command = command self.exitcode = exitcode

Thanks a lot. Just had to use exception.Error.

oops yeah, sorry my sample wasn't completely copy-pastable 😆

asottile · 2018-02-04T01:52:52Z

tox/__init__.py

+                str_ += " (exited with code %d)" % (self.exitcode)
+                if self.exitcode > 128:
+                    str_ += ("\nNote: On unix systems, an exit code larger than 128 "
+                             "often means a fatal error (e.g. 139=128+11: segmentation fault)")


>128 means it exited due to a fatal signal

You can see the various signals that might trigger this in the table at the bottom of this

We could also compute this and show it to the user:

$ python -c 'import time; time.sleep(100)' ^\Quit (core dumped) $ echo $? 131 $ python -c ' > import signal > sigs = {n: s for s, n in vars(signal).items() if s.startswith("SIG")} > print(sigs[131 - 128]) > ' SIGQUIT

A KeyError on sigs (in my example) means it's probably not a fatal signal that killed it (also note that although ^C sends SIGINT -- python sets a signal handler for this and reraises it as KeyboardInterrupt leading to an exit code of 1 instead of the expected 130)

Thanks for the link, added to the documentation that was in preparation.
Giving the signal value/name could be nice, since python uses directly the relevant signals.h; got to think about it.

For reference: subprocess actually returns negative exit codes for signals.
See #1401 for a followup/fix.

asottile · 2018-02-07T15:56:26Z

@gaborbernat I had an outstanding issue, I don't think this was ready for merge

This reverts commit c1db8a8.

gaborbernat · 2018-02-07T16:48:15Z

@asottile which one?

asottile · 2018-02-07T16:59:26Z

I suggested an alternate wording and some additional helpful output here

ederag · 2018-02-10T22:19:57Z

Summary: done, works, but could be better.
The commit does not appear. Is this PR broken ?

About the proposed wording "means it exited due to a fatal signal", it is true that "signal" is more precise than "error". Finally, the wording "fatal error signal" of faqs.org, seems good (especially for segmentation fault, since it originates from an error in the program, in the first place).

The note now gives the corresponding signal if one is found, hopefully matching your suggestion.
Issuing that message only for unix would require mocking sys.platform in tests.
Note sure it is worth the trouble though. And it would require pytest.mock.

Now testing the InvocationError itself is done more extensively, and much faster, in test_result.py. Obviously, it is not a tox.result, but kind of a result. Moreover, no other file (except the slow test_z_cmdline.py) seemed adequate.
That InvocationError is actually called or not, is still tested in test_z_cmdline.py. These tests look for a specific string in the tox output.
Two tests - exit code 0 or non-zero - remain. To be faster, it would be possible to use both commands in the same test. But it seemed safer to keep these two separate.
One fast and safe way would have been to use mock, but it did not work because exceptions can not be mocked like that. One way to circumvent that would be to create a separate function tox.exceptions.InvocationError._str(command, exitcode), called from tox.exceptions.InvocationError.__str__, and use the pytest.mock spy on the _str function only.
This would allow to gain about 3 seconds of test time for each python version tested.

Questions:
Just in case, would it be acceptable to add pytest.mock to the extra_require['testing'] in setup.py ?

Finally, a minor question from the initial post is still pending:

The modified section is too specific to be included in the PR, isn't it ?

gaborbernat · 2018-02-12T16:34:39Z

@ederag I think it's because the PR is closed, please open a new one 👍 adding test dependencies is fine

ederag · 2018-02-13T17:39:24Z

Waiting for revert-760-exitcode to be merged into master.

gaborbernat · 2018-02-13T18:54:47Z

no need to have the merge revert, just create a PR against the master.

ederag · 2018-02-14T20:04:57Z

Strange. If you won't merge revert-760-exitcode, then why revert the PR in the first place ?

gaborbernat · 2018-02-15T09:00:54Z

Was a miss click creation of that branch, sorry if it caused confusion, the revert was never merged on master https://github.com/tox-dev/tox/commits/master, I consider the extra changes here an improvement over what we have here. Hence why saying just create a new PR 👍

`subprocess` reports a negative exit code for signals (the assumption in tox-dev#760 (comment) was wrong). TODO: - [ ] faster test (takes >3s)

ederag added 3 commits February 2, 2018 19:41

Display exitcode upon InvocationError.

4981941

Issue tox-dev#290. Co-authored-by: Daniel Hahler <git@thequod.de>

hint for exitcode > 128

7ab2b66

add changelog fragment

309ae93

asottile reviewed Feb 4, 2018

View reviewed changes

ederag added 2 commits February 5, 2018 19:17

use keyword argument instead of try/except

f1cf958

add documentation

6632a5f

gaborbernat self-requested a review February 7, 2018 12:34

gaborbernat approved these changes Feb 7, 2018

View reviewed changes

gaborbernat merged commit c1db8a8 into tox-dev:master Feb 7, 2018

gaborbernat added a commit that referenced this pull request Feb 7, 2018

Revert "Display exit code with InvocationErrors (#760)"

f1b1331

This reverts commit c1db8a8.

ederag mentioned this pull request Feb 16, 2018

Hint for possible signal upon InvocationError #766

Merged

5 tasks

blueyed added a commit to blueyed/tox that referenced this pull request Aug 20, 2019

Fix reporting exit due to signal

fe85a00

`subprocess` reports a negative exit code for signals (the assumption in tox-dev#760 (comment) was wrong). TODO: - [ ] faster test (takes >3s)

blueyed added a commit to blueyed/tox that referenced this pull request Aug 20, 2019

Fix reporting exit due to signal

82e3a8b

`subprocess` reports a negative exit code for signals (the assumption in tox-dev#760 (comment) was wrong). TODO: - [ ] faster test (takes >3s)

blueyed mentioned this pull request Aug 20, 2019

Fix reporting exit due to (real) signal #1401

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display exit code with InvocationErrors #760

Display exit code with InvocationErrors #760

ederag commented Feb 2, 2018 •

edited

Loading

codecov bot commented Feb 2, 2018 •

edited

Loading

asottile Feb 4, 2018

ederag Feb 5, 2018

asottile Feb 5, 2018

asottile Feb 4, 2018 •

edited

Loading

ederag Feb 5, 2018

blueyed Aug 20, 2019

asottile commented Feb 7, 2018

gaborbernat commented Feb 7, 2018

asottile commented Feb 7, 2018

ederag commented Feb 10, 2018 •

edited

Loading

gaborbernat commented Feb 12, 2018 •

edited

Loading

ederag commented Feb 13, 2018 •

edited

Loading

gaborbernat commented Feb 13, 2018 •

edited

Loading

ederag commented Feb 14, 2018

gaborbernat commented Feb 15, 2018

Display exit code with InvocationErrors #760

Display exit code with InvocationErrors #760

Conversation

ederag commented Feb 2, 2018 • edited Loading

Contribution checklist:

codecov bot commented Feb 2, 2018 • edited Loading

Codecov Report

asottile Feb 4, 2018

Choose a reason for hiding this comment

ederag Feb 5, 2018

Choose a reason for hiding this comment

asottile Feb 5, 2018

Choose a reason for hiding this comment

asottile Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

ederag Feb 5, 2018

Choose a reason for hiding this comment

blueyed Aug 20, 2019

Choose a reason for hiding this comment

asottile commented Feb 7, 2018

gaborbernat commented Feb 7, 2018

asottile commented Feb 7, 2018

ederag commented Feb 10, 2018 • edited Loading

gaborbernat commented Feb 12, 2018 • edited Loading

ederag commented Feb 13, 2018 • edited Loading

gaborbernat commented Feb 13, 2018 • edited Loading

ederag commented Feb 14, 2018

gaborbernat commented Feb 15, 2018

ederag commented Feb 2, 2018 •

edited

Loading

codecov bot commented Feb 2, 2018 •

edited

Loading

asottile Feb 4, 2018 •

edited

Loading

ederag commented Feb 10, 2018 •

edited

Loading

gaborbernat commented Feb 12, 2018 •

edited

Loading

ederag commented Feb 13, 2018 •

edited

Loading

gaborbernat commented Feb 13, 2018 •

edited

Loading