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

Display exit code with InvocationErrors #290

Closed
pytoxbot opened this Issue Sep 17, 2016 · 20 comments

Comments

Projects
None yet
6 participants
@pytoxbot

pytoxbot commented Sep 17, 2016

When a command fails, you currently only get ERROR: InvocationError: '/usr/bin/false', but the exitcode itself is useful/important, e.g. with
py.test using 5 for "no tests collected".

The following patch would provide that, by using a custom __str__ method for
InvocationError - but it does not handle the case where that might be missing (not all instances / calls to it provide it).

Maybe a custom __init__ should be used to set the arguments as properties explicitly, and then use them?!

diff -r e9569646da4f tox/__init__.py
--- a/tox/__init__.py   Wed Nov 25 12:27:48 2015 +0100
+++ b/tox/__init__.py   Wed Nov 25 13:23:26 2015 +0100
@@ -17,6 +17,9 @@
         "signals that an interpreter could not be found"
     class InvocationError(Error):
         """ an error while invoking a script. """
+        def __str__(self):
+            return "%s: %s (exitcode %d)" % (self.__class__.__name__,
+                                             self.args[0], self.args[1])
     class MissingFile(Error):
         """ an error while invoking a script. """
     class MissingDirectory(Error):

This is related / similar to #192.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @The-Compiler

+1 for this feature - I've had tests segfault and no useful info from tox before. I got used to it, but the first time I saw it it took me a while to figure out what happened.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @hpk42

if you can provide a test and put this all into a PR i'll see to merge it.

@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @blueyed

Here is an updated patch that will pass tests at least:

--- a/tox/__init__.py   Wed Nov 25 12:27:48 2015 +0100
+++ b/tox/__init__.py   Wed Nov 25 18:37:41 2015 +0100
@@ -17,6 +17,20 @@
         "signals that an interpreter could not be found"
     class InvocationError(Error):
         """ an error while invoking a script. """
+        def __init__(self, *args):
+            super(exception.Error, self).__init__(*args)
+            self.command = args[0]
+            try:
+                self.exitcode = args[1]
+            except IndexError:
+                self.exitcode = None
+
+        def __str__(self):
+            if self.exitcode:
+                return "%s: %s (exitcode %d)" % (self.__class__.__name__,
+                                                 self.command, self.exitcode)
+            return "%s: %s" % (self.__class__.__name__, self.command)
+
     class MissingFile(Error):
         """ an error while invoking a script. """
     class MissingDirectory(Error):
@pytoxbot

This comment has been minimized.

pytoxbot commented Sep 17, 2016

Original comment by @blueyed

While at it, it might be useful to display it also here:

diff -r e9569646da4f tox/venv.py
--- a/tox/venv.py   Wed Nov 25 12:27:48 2015 +0100
+++ b/tox/venv.py   Wed Nov 25 14:50:51 2015 +0100
@@ -137,6 +137,7 @@
             return sys.exc_info()[1]
         try:
             self.install_deps(action)
+        # TODO: display InvocationError here?
         except tox.exception.InvocationError:
             v = sys.exc_info()[1]
             return "could not install deps %s; v = %r" % (
@obestwalter

This comment has been minimized.

Member

obestwalter commented Nov 4, 2016

@blueyed - still interested to make this happen?

@blueyed

This comment has been minimized.

Contributor

blueyed commented Nov 4, 2016

@obestwalter
In general, yes - but not in the near future.
Feel free to pick up the patch(es) from here, of course.

@obestwalter

This comment has been minimized.

Member

obestwalter commented Nov 4, 2016

@blueyed thanks - I might try to crystalize your drops of genius in this issue in a PR then :)

@ederag

This comment has been minimized.

Contributor

ederag commented Jan 14, 2018

This is useful, thanks !
The patch #290 (comment) did not apply to current repo. Here is an updated one (only the @@ line changed).

diff --git a/tox/__init__.py b/tox/__init__.py
index b7a1631..d872b32 100644
--- a/tox/__init__.py
+++ b/tox/__init__.py
@@ -34,6 +34,19 @@ class exception:
 
     class InvocationError(Error):
         """ an error while invoking a script. """
+        def __init__(self, *args):
+            super(exception.Error, self).__init__(*args)
+            self.command = args[0]
+            try:
+                self.exitcode = args[1]
+            except IndexError:
+                self.exitcode = None
+
+        def __str__(self):
+            if self.exitcode:
+                return "%s: %s (exitcode %d)" % (self.__class__.__name__,
+                                                 self.command, self.exitcode)
+            return "%s: %s" % (self.__class__.__name__, self.command)
 
     class MissingFile(Error):
         """ an error while invoking a script. """

@obestwalter

This comment has been minimized.

Member

obestwalter commented Jan 24, 2018

Hey @ederag thanks! Would you mind open a PR for this yourself?

@ederag

This comment has been minimized.

Contributor

ederag commented Jan 24, 2018

Well, that's your patch actually, but if it helps, OK.

  1. Any pointer for the tests ? Such as an example to mimic,
    or at least the destination (test_z_cmdline.py ?)
  2. Should it be mentioned in the docs ? Where ?
    It seems pretty self-explanatory,
    but it would be worth stressing that the absence of error code
    means that the command actually crashed ?
@obestwalter

This comment has been minimized.

Member

obestwalter commented Jan 25, 2018

Hi @ederag you must confuse me with @blueyed :)

Great that you want to pick it up! Good questions also, I would have to search around and think about these just as you, but at the moment I need to get the next release out of the way, so I won't go in too deep for now. Just quick of the top of my head:

test_z_cmdline.py are like the systemtests of tox: they always create a complete project and therefore are the slowest, but necessary to check complete functionality. They might be the best place to test this properly, but I would suggest to have a look around in the other tests to get inspiration about ohter ways to test this in a more targetted way.

About the docs:

If you find a good place to add that info, you could add it, but I don't see it as necessary, as the user gets more information after a crash and does not necessary need to be told that they would get that information in case of failure.

but it would be worth stressing that the absence of error code means that the command actually crashed?

I don't understand, what you mean by that. My understanding is that this patch is supposed to print also the error code returned by the program when it crashed (e.g. if pytest fails with error 5, we also print that number). If a programm crashes there always will be an error code. It is not possible to have no error code.

How about you just get started and open a PR and then we can fix it up together.

@The-Compiler

This comment has been minimized.

Contributor

The-Compiler commented Jan 25, 2018

When a program crashes rather than exits (e.g. because of a segfault), it only "kinda' has an exit code: It exits with 128 + number of the signal. That is, at least on Unix, not sure how things look on Windows.

It depends on how tox handles this in all places it raises an InvocationError. Here's how things would look ideally, IMHO:

  • If it exited with != 0, tox would show "InvocationError: (command) exited with status 1" or so (being a bit more verbose doesn't hurt here - I had dozens of people being confused about what InvocationError means and thought it was tox failing).
  • If it crashed, tox would show: "InvocationError: (command) crashed with SIGSEGV" or so.

That'd make things much clearer. I'm not sure how much effort it'd be though.

@ederag

This comment has been minimized.

Contributor

ederag commented Jan 25, 2018

Understood now; "Crashed" was not appropriate, I was referring to pytest-dev/pytest-qt#170 (comment), which was not a segmentation fault, but rather Qt aborting.
In such a case there is no output at all (the tox process itself is killed, IIUC), and a documentation would not hurt.

Thank you both for your inputs, enough to get started.

@The-Compiler Agreed, being more verbose is better here.
Intuitively, checking for os.getpid() in this exception would be too naive, and a quick grep on pid yields nothing.
So what about supplementing the error message when exitcode > 128, with something like
"On unix systems, an exit code larger than 128 often means a crash (e.g. segmentation fault)" ?
(this should be better than a crash message for applications issuing large custom error codes)

@ederag

This comment has been minimized.

Contributor

ederag commented Jan 29, 2018

As a first step, @blueyed patch has been a little bit completed, hopefully following @The-Compiler recommendations #290 (comment), and tests added.
test_z_cmdline.py definitely seems to be the right place. Safely replacing them with faster ones (mock) would require someone experienced with this technique.

This is in the exitcode branch of https://github.com/ederag/tox.
Just beware that until #753 is accepted, exitcode should be based on the alwayscopySuSE branch.

@blueyed Since you started all this, and wrote the most important part, would you please fork it and create the pull request ?

@blueyed

This comment has been minimized.

Contributor

blueyed commented Jan 29, 2018

@ederag
It is not important for me to take credit for it - if that's the reason for asking me.
I think it is better that you create the PR, since you have the branch/code already, and will keep on working on it I assume?!
Thanks!

ederag added a commit to ederag/tox that referenced this issue Jan 30, 2018

Display exitcode upon InvocationError.
Thanks go to Daniel Hahler (@blueyed): Issue tox-dev#290.
@ederag

This comment has been minimized.

Contributor

ederag commented Jan 30, 2018

@blueyed Thanks, so the commit message was changed to give you credits. Hope there are no mistakes ?

@blueyed

This comment has been minimized.

Contributor

blueyed commented Jan 30, 2018

@ederag
Thanks.
I would prefer not to be mentioned (@blueyed) there, since that triggers GitHub notifications.
You have not created a PR yet, have you?

ederag added a commit to ederag/tox that referenced this issue Jan 31, 2018

Display exitcode upon InvocationError.
Thanks go to Daniel Hahler (blueyed): Issue tox-dev#290.
@ederag

This comment has been minimized.

Contributor

ederag commented Jan 31, 2018

@blueyed
Sorry for the noise; This discussion would be better in a PR page.
But changing this commit requires force push, which "could corrupt the pull request".
Just stumbled on co-authoring a commit; What would you prefer after Co-authored-by: ?

@blueyed

This comment has been minimized.

Contributor

blueyed commented Feb 2, 2018

Daniel Hahler <REMOVED> would be fine I guess. (edited: you should have gotten it in an email)
But amending and force-pushing is not a problem usually.

ederag added a commit to ederag/tox that referenced this issue Feb 2, 2018

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

@ederag ederag referenced this issue Feb 2, 2018

Merged

Display exit code with InvocationErrors #760

6 of 6 tasks complete

gaborbernat added a commit that referenced this issue Feb 7, 2018

Display exit code with InvocationErrors (#760)
* Display exitcode upon InvocationError.

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

* hint for exitcode > 128
* add changelog fragment
* use keyword argument instead of try/except
* add documentation

gaborbernat added a commit that referenced this issue Feb 18, 2018

Hint for possible signal upon InvocationError (#766)
* Display exitcode upon InvocationError.

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

* hint for exitcode > 128

* add changelog fragment

* use keyword argument instead of try/except

* add documentation

* hints on the potential signal raised + better tests

* pep8 naming: exitcode => exit_code

* rename test_InvocationError => test_invocation_error

* exit code + signal name

* terser note

* remove blank line

* add changelog fragment

* remove "python" from expected_command_arg (fix appveyor)
@gaborbernat

This comment has been minimized.

Member

gaborbernat commented Sep 15, 2018

resolved by #766 Hint for possible signal upon InvocationError

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment