conversion of exit codes to enum + exposure #5420

RonnyPfannschmidt · 2019-06-07T11:11:06Z

nicoddemus · 2019-06-07T11:30:25Z

Are you back from your hiatus? 😁

RonnyPfannschmidt · 2019-06-07T11:35:52Z

no, im currently on childcare leave since the rest of the family is sick and in bed, since they finally sleep i picked something easy that still allows me to retain my code-base familiarity

RonnyPfannschmidt · 2019-06-08T10:31:01Z

the last value failure is tricky, there is no simple way not to break the api wrt giving full allowance over the exit code anyway

RonnyPfannschmidt · 2019-06-09T07:54:25Z

for now i do a unconditional warning, im wondering if that should be the case or if the code should be silent about it for now

in future i'd like to arrive at a stage where pytest return codes are well defined and enumerated

RonnyPfannschmidt · 2019-06-09T08:36:35Z

im not sure where to best document this

nicoddemus · 2019-06-10T19:47:09Z

im not sure where to best document this

How about in reference.rst?

nicoddemus · 2019-06-13T23:31:52Z

Thanks @RonnyPfannschmidt!

While the solution is clever, I'm a little concerned to introduce a exit_code public variable which is very similar to exitstatus, I'm sure it will bring confusion to users.

I've taken one of your earliest commits (b7cd089) and managed to fix the issue locally in xdist with this trivial change:

diff --git a/src/xdist/remote.py b/src/xdist/remote.py
index d87ff4c..1643a64 100644
--- a/src/xdist/remote.py
+++ b/src/xdist/remote.py
@@ -41,7 +41,7 @@ class WorkerInteractor(object):

     @pytest.hookimpl(hookwrapper=True)
     def pytest_sessionfinish(self, exitstatus):
-        self.config.workeroutput["exitstatus"] = exitstatus
+        self.config.workeroutput["exitstatus"] = int(exitstatus)
         yield
         self.sendevent("workerfinished", workeroutput=self.config.workeroutput)

So I think we are better off fixing this in pytest-xdist rather than introduce a new confusing public attribute. I volunteer to fix this in pytest-xdist and make a new release. What do you think?

nicoddemus · 2019-06-13T23:36:34Z

docs note: there's a Possible exit codes section in usage.rst which should refer to the ExitCode enum in reference.rst.

RonnyPfannschmidt · 2019-06-14T04:19:55Z

implementing it the proposed way is a breaking api change, i'd much rather introduce a new name for the new behavior and then deprecate the old before removing it later

while at it we also should introduce errro codes where plugins and test-suites can indicate errors within them and forbid free error codes

nicoddemus · 2019-06-14T10:50:59Z

implementing it the proposed way is a breaking api change

The whole purpose of IntEnum is to change old constant-integers API we had to the new IntEnum in a backward compatible way. Can you provide a code sample of where changing exitstatus to an IntEnum breaks existing code, other than someone doing highly unlikely as expecting int(session.status) is int to be True?

Even if it is a breaking API change, we will be releasing it in 5.0, which is a major release and breaking changes are possible (not that I really thing this will break anything, mind you).

i'd much rather introduce a new name for the new behavior and then deprecate the old before removing it later

TBH I would rather then keep the old constants and publish them properly as attributes of sessionas suggested initially, rather than introducing a confusing new name just and deprecating the old existing name later.

nicoddemus · 2019-06-14T13:16:20Z

Why closing @RonnyPfannschmidt? I think the change to IntEnum could go as a "breaking API change" into 5.0. I doubt it would bring any fallout TBH.

RonnyPfannschmidt · 2019-06-14T13:55:36Z

i missunderstood then - but in that case i'd like to prohibit non-declared exit codes in general

nicoddemus · 2019-06-14T14:05:48Z

i missunderstood then

Hmm sorry then, let me try to be clear about my point of view:

I think it is OK to introduce the ExitCode enum as the type of exitstatus in 5.0, noting that this might break esoteric cases but we don't expect major problems for users, as the IntEnum is highly compatible with ints.

The small xdist problem is trivially fixable and it is not a good example of problems that code out there can encounter caused by this change because xdist is really a special case because of execnet.

Having said that, if you agree, these changes are required before we can merge:

Implement and release the small xdist change I posted before
Remove the exit_code attribute
Add docs: reference.rst and usage.rst

I'm happy to do 1).

What do you think?

RonnyPfannschmidt · 2019-06-14T14:14:06Z

i'll pick the branch up later today, i just noted its on my desktop, and not the work laptop i have with me as im currently somewhere else - git branch sync is such a pain ^^

nicoddemus · 2019-06-14T14:16:47Z

Sounds great. 👍

Related to pytest-dev/pytest#5420

nicoddemus · 2019-06-14T14:24:07Z

Done: pytest-dev/pytest-xdist#442 👍

RonnyPfannschmidt · 2019-06-15T05:06:44Z

i returned back to the state where passing a integer is still valid, but the type is now primaryly the enum, that gives some wiggle room until we deprecate user defined random values for real

RonnyPfannschmidt · 2019-06-15T06:04:10Z

passes with new xdist t seems :) 👍 @nicoddemus

nicoddemus

Looks great, thanks!

I think we are only missing docs now:

Add ExitCode to reference.rst
Link ExitCode in the "Possible exit codes" section in usage.rst

src/_pytest/main.py

nicoddemus · 2019-06-15T13:21:37Z

changelog/5125.removal.rst

@@ -0,0 +1,2 @@
+Introduce the ``pytest.ExitCode`` Enum and make session.exitcode an instance of it.
+User defined exit codes are still valid, but consumers need to take the enum into account.


"but consumers need to take the enum into account."

Not clear to me what this means, what consumers would need to do specifically?

users of that api, like the xdist release you just made to support it

Ahh OK, I got confused because I thought the first and second sentence were related, but the "but consumers take the enum into account" phrase was actually referring to the "Introduce the pytestExitCode" phase.

How about:

``Session.exitcode`` values are now coded in ``pytest.ExitCode``, an ``IntEnum``. This makes the exit code available for consumer code and are more explicit other than just documentation. User defined exit codes are still valid, but should be used with caution. The team doesn't expect this change to break test suites or plugins in general, except in esoteric/specific scenarios. **pytest-xdist** users should upgrade to ``1.29.0`` or later, as ``pytest-xdist`` required a compatibility fix because of this change.

(can't use the "suggestion" feature as this spams multiple lines)

nicoddemus · 2019-06-15T15:03:33Z

src/_pytest/main.py

+class ExitCode(enum.IntEnum):
+    OK = 0
+    TESTS_FAILED = 1
+    INTERRUPTED = 2


Just occurred to me: is there a consensus about how to name the enumerate members? test_failed, TestsFailed or TESTS_FAILED? I've not seen many enums in the wild so I'm not sure. 🤔

Hmm all examples in the enum docs show ALL_CAPS, so I think we are fine. 👍

using PEP8 for constants

basically enum members are constants, thus follow the same rules as constants ^^

nicoddemus · 2019-06-15T17:51:52Z

src/_pytest/main.py

+
+class ExitCode(enum.IntEnum):
+    """
+    encodes the valid exit codes of pytest


This paragraph is not looking too good:

Suggestion:

Encodes the valid exit codes by pytest. Currently users and plugins may supply other exit codes as well.

nicoddemus

Also please add a link to usage.rst. 😁

nicoddemus

Thanks!

blueyed · 2019-06-16T23:47:09Z

doc/en/usage.rst

@@ -33,6 +33,8 @@ Running ``pytest`` can result in six different exit codes:
 :Exit code 4: pytest command line usage error
 :Exit code 5: No tests were collected

+They are repressended by the :class:`_pytest.main.ExitCode` enum.


*represented

blueyed · 2019-06-16T23:49:04Z

src/_pytest/main.py

+    INTERRUPTED = 2
+    #: an internal error got in the way
+    INTERNAL_ERROR = 3
+    #: pytest was missused


blueyed · 2019-06-16T23:49:17Z

src/_pytest/main.py

+    INTERNAL_ERROR = 3
+    #: pytest was missused
+    USAGE_ERROR = 4
+    #: pytest couldnt find tests


blueyed · 2019-06-16T23:49:54Z

src/_pytest/pytester.py

        return res

    def genitems(self, colitems):
-        """Generate all test items from a collection node.
+        """Generate all test items from a collection node.src/_pytest/main.py


paste error?

RonnyPfannschmidt · 2019-06-17T04:15:38Z

yikes, good finds

blueyed · 2019-06-17T11:42:11Z

yikes, good finds

Will you fix them?

RonnyPfannschmidt · 2019-06-17T12:41:05Z

i can get around it mid week

Zac-HD · 2019-06-22T07:36:54Z

#3144 might also be related?

RonnyPfannschmidt force-pushed the exitcode-enum branch from c9f7dea to 94b4ead Compare June 7, 2019 11:15

This comment has been minimized.

Sign in to view

RonnyPfannschmidt changed the title ~~WIP: conversion of exit codes to enum + exposure~~ conversion of exit codes to enum + exposure Jun 9, 2019

RonnyPfannschmidt closed this Jun 14, 2019

RonnyPfannschmidt reopened this Jun 14, 2019

nicoddemus added a commit to nicoddemus/pytest-xdist that referenced this pull request Jun 14, 2019

Compatibility for IntEnum change in upcoming pytest 5.0

db1c3e7

Related to pytest-dev/pytest#5420

nicoddemus mentioned this pull request Jun 14, 2019

Compatibility for IntEnum change in upcoming pytest 5.0 pytest-dev/pytest-xdist#442

Merged

nicoddemus added a commit to nicoddemus/pytest-xdist that referenced this pull request Jun 14, 2019

Compatibility for IntEnum change in upcoming pytest 5.0

fb40056

Related to pytest-dev/pytest#5420

This comment has been minimized.

Sign in to view

RonnyPfannschmidt added 2 commits June 15, 2019 06:48

initial conversion of exit codes to enum

2b92fee

add changelog

2bd619e

RonnyPfannschmidt force-pushed the exitcode-enum branch from d660499 to 2bd619e Compare June 15, 2019 05:05

nicoddemus requested changes Jun 15, 2019

View reviewed changes

nicoddemus added this to the 5.0 milestone Jun 15, 2019

nicoddemus reviewed Jun 15, 2019

View reviewed changes

RonnyPfannschmidt added 3 commits June 15, 2019 17:03

update cangelog to fit review suggestion

065fa17

document exitcode members

103d614

pre-commit

8b3b10b

nicoddemus reviewed Jun 15, 2019

View reviewed changes

nicoddemus requested changes Jun 15, 2019

View reviewed changes

add ExitCode reference in usage

1cfea5f

nicoddemus approved these changes Jun 15, 2019

View reviewed changes

Improve ExitCode docstring

ab6ed38

nicoddemus merged commit bbfc8d1 into pytest-dev:master Jun 16, 2019

blueyed reviewed Jun 16, 2019

View reviewed changes

yashtodi94 mentioned this pull request Aug 4, 2019

Added info to access exitcodes directly #5694

Merged

blueyed mentioned this pull request Oct 21, 2019

ExitCode: support custom codes #6022

Closed

		@@ -0,0 +1,2 @@
		Introduce the ``pytest.ExitCode`` Enum and make session.exitcode an instance of it.
		User defined exit codes are still valid, but consumers need to take the enum into account.

conversion of exit codes to enum + exposure #5420

conversion of exit codes to enum + exposure #5420

Conversation

RonnyPfannschmidt commented Jun 7, 2019

nicoddemus commented Jun 7, 2019

RonnyPfannschmidt commented Jun 7, 2019

This comment has been minimized.

RonnyPfannschmidt commented Jun 8, 2019

RonnyPfannschmidt commented Jun 9, 2019

RonnyPfannschmidt commented Jun 9, 2019

nicoddemus commented Jun 10, 2019

nicoddemus commented Jun 13, 2019 • edited Loading

nicoddemus commented Jun 13, 2019

RonnyPfannschmidt commented Jun 14, 2019

nicoddemus commented Jun 14, 2019 • edited Loading

nicoddemus commented Jun 14, 2019 • edited Loading

RonnyPfannschmidt commented Jun 14, 2019

nicoddemus commented Jun 14, 2019

RonnyPfannschmidt commented Jun 14, 2019

nicoddemus commented Jun 14, 2019

nicoddemus commented Jun 14, 2019

This comment has been minimized.

This comment has been minimized.

RonnyPfannschmidt commented Jun 15, 2019

RonnyPfannschmidt commented Jun 15, 2019

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonnyPfannschmidt commented Jun 17, 2019

blueyed commented Jun 17, 2019

RonnyPfannschmidt commented Jun 17, 2019

Zac-HD commented Jun 22, 2019

nicoddemus commented Jun 13, 2019 •

edited

Loading

nicoddemus commented Jun 14, 2019 •

edited

Loading

nicoddemus commented Jun 14, 2019 •

edited

Loading