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 bang to invert exit code #3271

Merged
merged 2 commits into from
Apr 26, 2024
Merged

Conversation

sillydan1
Copy link
Contributor

@sillydan1 sillydan1 commented Apr 25, 2024

This makes it so that you can provide a "!" before the command line to invert the success/failure of a command. Just like with regular POSIX shell. This is useful if you expect that the command should crash / return an error.

Currently, to achieve this behavior, you'd have to do wrap it in a shell command: sh -c "! python3 -m foo" and that won't necessarily use the correct virtualenv.

  • ran the linter to address style issues (tox -e fix)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

@sillydan1 sillydan1 requested a review from gaborbernat as a code owner April 25, 2024 08:40
@jugmac00
Copy link
Member

jugmac00 commented Apr 25, 2024

@sillydan1 Thanks for the pull request. Ideally we would have had a quick discussion before a pull request was created.

Are you aware of that you could already ignore the exit code for commands, see https://tox.wiki/en/4.14.2/faq.html#ignoring-the-exit-code-of-a-given-command?

Could you give us a couple of real world examples why and when inverting the exit code would make sense? I have a hard time to come up with examples.

@sillydan1
Copy link
Contributor Author

Ideally we would have had a quick discussion before a pull request was created.

Sorry about that. I can create a new discussion if this is inappropriate 😄

Are you aware of that you could already ignore the exit code for commands, see https://tox.wiki/en/4.14.2/faq.html#ignoring-the-exit-code-of-a-given-command?

Yes I am aware 😄 . The problem with that is I want the task to fail if the command succeeds and pass if the command fails and ignoring the exit code will always succeed.

This is useful when you want to assert that some some usage explicitly fails. Say you're writing a unit testing framework and you want to make sure that when a test fails, the exit code should be non-zero. Simply ignoring the exit code won't catch the case where the invocation erroneously exits with code zero.

@jugmac00
Copy link
Member

I see.

We also need to test the exit code when tox fails and thus returns a non-zero exit code.

For that we use the pytest.raises(SystemExit) paradigm, also see e.g. https://stackoverflow.com/a/62427689/672833.

I would recommend that way, as then you can also explicitly check for the exit code - as in exit code number/value.

I am very hesitant to add new features to tox, especially those changing the syntax, as with the wide user base we need to be very careful not to break other users' configuration.

I would like to have @gaborbernat's opinion on that.

P.S:: FWIW I think you could also make use of the envpython placeholder value to make sure you access the correct Python interpreter.

@sillydan1
Copy link
Contributor Author

For that we use the pytest.raises(SystemExit) paradigm, also see e.g. https://stackoverflow.com/a/62427689/672833.

I am aware of this, but I am not invoking the command from a python context (or pytest for that matter). I might be wrong, but as far as I know, the commands field in my tox.ini file are just regular program calls no?

In a more concrete example, I have a tox file that looks something like this:

[testenv:invocations]
description = Run my own custom tailor made testing framework that is not pytest or anything else.
basepython = py310
commands =
    python3 -m my_testing_framework test/these_pass.py    # This invocation will exit with code 0
    python3 -m my_testing_framework test/these_fail.py    # This invocation will exit with a non-zero exit code - on purpose.
    - python3 -m my_testing_framework test/these_fail.py  # Here we simply ignore the exit code, but if my_testing_framework exits with code 0, then the tox pipeline errorniously succeeds

In a regular shell, I can use the bang-operator to achieve this exit-code inversion behavior like so:

$ python3 -m my_testing_framework test/these_pass.py
$ echo $?
0

$ python3 -m my_testing_framework test/these_fail.py
$ echo $?
1

$ ! python3 -m my_testing_framework test/these_fail.py
$ echo $? 
0          # the shell invocation exit code is success!

As I mentioned in the PR description, I can emulate this by wrapping it in a sh -c command:

[testenv:invocations]
description = Run my own custom tailor made testing framework that is not pytest or anything else.
basepython = py310
allowlist_external = sh  # required now
commands =
    python3 -m my_testing_framework test/these_pass.py    # This invocation will exit with code 0
    sh -c "! python3 -m my_testing_framework test/these_fail.py" # Kinda odd and inelegant, but technically works (missing: envpython usage). 

@jugmac00
Copy link
Member

I think the problem is now well understood. The question is whether we want to add this feature, and I lean towards not doing so, but as mentioned I would like to hear the feedback of @gaborbernat.

tox is meant to run other test and linting tools, which themselves do the error handling. In your case, you directly test your application with tox - that way is not one we had anticipated.

@sillydan1
Copy link
Contributor Author

tox is meant to run other test and linting tools, which themselves do the error handling. In your case, you directly test your application with tox - that way is not one we had anticipated.

That sounds fair to me 😄 I'm looking forward to the feedback

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

I'm personally okay to add this.

This makes it so that you can provide a "!" before the command line to
invert the success/failure of a command. Just like with regular POSIX
shell.
@sillydan1 sillydan1 force-pushed the feature/invert-error branch from 830ea40 to 0ed33e4 Compare April 25, 2024 14:04
@sillydan1 sillydan1 requested a review from gaborbernat April 25, 2024 14:05
@gaborbernat
Copy link
Member

Can you fix the CI?

@sillydan1
Copy link
Contributor Author

Can you fix the CI?

It seems that the only thing that is failing is a link to the github user tjsmart
https://github.com/tox-dev/tox/actions/runs/8834033929/job/24254800610?pr=3271

Maybe just run the job again?

@gaborbernat gaborbernat merged commit 809e10f into tox-dev:main Apr 26, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants