Fix potential off by one error

[rhoban13]

In #134 support was added for --exitfirst. I think we might have an off by one error, this PR just illustrating with this small change to the tests.

I think the issue is that self.request.session.shouldfail does not get set until we call pytest_runtest_logreport which is after the flag is checked. The result is the tests get stopped after the 2nd failing subtest instead of the first.

[rhoban13]

We can probably simply move the early return False inside __exit__ down to after all of the reporting is done?

[rhoban13]

This doesn't seem right, but seems to leave the parent test off the test result entirely.

[rhoban13]

I updated the test to reflect what I think is desired behavior - which leaves the parent test off the test report entirely. I took a stab at a fix, but it calls pytest.exit which I suspect isn't great. One alternative I could think of might be to implement one of the relevant hooks, but haven't tried it.

[nicoddemus]

Hi @rhoban13,

Here's my example for testing:

def test_foo(subtests):
    with subtests.test(i=0):
        assert False

    with subtests.test(i=1):
        assert False

    with subtests.test(i=2):
        assert False

Executing:

λ pytest .tmp\test_foo.py --no-header
======================== test session starts ========================
collected 1 item

.tmp\test_foo.py uuu.                                          [100%]

============================= FAILURES ==============================
__________________________ test_foo (i=0) ___________________________

subtests = SubTests(ihook=<_pytest.config.compat.PathAwareHookProxy object at 0x00000250A68F6E90>, suspend_capture_ctx=<bound met..._state='started' _in_suspended=False> _capture_fixture=None>>, request=<SubRequest 'subtests' for <Function test_foo>>)

    def test_foo(subtests):
        with subtests.test(i=0):
>           assert False
E           assert False

.tmp\test_foo.py:6: AssertionError
------------------------- Captured log call -------------------------

__________________________ test_foo (i=1) ___________________________

subtests = SubTests(ihook=<_pytest.config.compat.PathAwareHookProxy object at 0x00000250A68F6E90>, suspend_capture_ctx=<bound met..._state='started' _in_suspended=False> _capture_fixture=None>>, request=<SubRequest 'subtests' for <Function test_foo>>)

    def test_foo(subtests):
        with subtests.test(i=0):
            assert False

        with subtests.test(i=1):
>           assert False
E           assert False

.tmp\test_foo.py:9: AssertionError
------------------------- Captured log call -------------------------

__________________________ test_foo (i=2) ___________________________

subtests = SubTests(ihook=<_pytest.config.compat.PathAwareHookProxy object at 0x00000250A68F6E90>, suspend_capture_ctx=<bound met..._state='started' _in_suspended=False> _capture_fixture=None>>, request=<SubRequest 'subtests' for <Function test_foo>>)

    def test_foo(subtests):
        with subtests.test(i=0):
            assert False

        with subtests.test(i=1):
            assert False

        with subtests.test(i=2):
>           assert False
E           assert False

.tmp\test_foo.py:12: AssertionError
------------------------- Captured log call -------------------------

====================== short test summary info ======================
(i=0) SUBFAIL .tmp/test_foo.py::test_foo - assert False
(i=1) SUBFAIL .tmp/test_foo.py::test_foo - assert False
(i=2) SUBFAIL .tmp/test_foo.py::test_foo - assert False
==================== 3 failed, 1 passed in 0.20s ====================

So the parent test does run.

Implementing your original suggestion (moving the check to the end and returning False) we get:

λ pytest .tmp\test_foo.py --no-header -x
======================== test session starts ========================
collected 1 item

.tmp\test_foo.py uF                                            [100%]

============================= FAILURES ==============================
__________________________ test_foo (i=0) ___________________________

subtests = SubTests(ihook=<_pytest.config.compat.PathAwareHookProxy object at 0x00000204DA906EC0>, suspend_capture_ctx=<bound met..._state='started' _in_suspended=False> _capture_fixture=None>>, request=<SubRequest 'subtests' for <Function test_foo>>)

    def test_foo(subtests):
        with subtests.test(i=0):
>           assert False
E           assert False

.tmp\test_foo.py:6: AssertionError
------------------------- Captured log call -------------------------

_____________________________ test_foo ______________________________

subtests = SubTests(ihook=<_pytest.config.compat.PathAwareHookProxy object at 0x00000204DA906EC0>, suspend_capture_ctx=<bound met...tate='suspended' _in_suspended=False> _capture_fixture=None>>, request=<SubRequest 'subtests' for <Function test_foo>>)

    def test_foo(subtests):
        with subtests.test(i=0):
>           assert False
E           assert False

.tmp\test_foo.py:6: AssertionError
====================== short test summary info ======================
(i=0) SUBFAIL .tmp/test_foo.py::test_foo - assert False
FAILED .tmp/test_foo.py::test_foo - assert False
!!!!!!!!!!!!!!!!!!!!! stopping after 2 failures !!!!!!!!!!!!!!!!!!!!!
========================= 2 failed in 0.19s =========================

I think having the parent test on the report is acceptable when using -x, IMHO... calling pytest.exit() is not something that should be done lightly.

In summary I think your original idea of moving the check and return False to the end is the way to go.

[rhoban13]

Updated per above - I also think it's good to have the parent test on the report. The only thing perhaps a little odd is that with -x a failing subtest implies the parent test is also considered failing, and that's not the case without -x. However I think that's something anyone should be able to reasonably understand.

[nicoddemus]

Thanks @rhoban13, appreciate it!