Unittest unexpected success should be translated to XPASS

[bluetech]

pytest version: 5.4.x & current master, Python version: 3.8

xfail background

pytest's unittest plugin tries to translate unittest concepts to pytest concept. One concept in pytest is xfail/XFAIL/XPASS (see docs).

Example:

import pytest

@pytest.mark.xfail
def test_expected_failure():
    assert False

@pytest.mark.xfail
def test_unexpected_success():
    assert True

running:

x.py::test_expected_failure XFAIL
x.py::test_unexpected_success XPASS

also, if running with strict xfail, this becomes

x.py::test_expected_failure XFAIL
x.py::test_unexpected_success FAILED

============== FAILURES =============
______ test_unexpected_success ______
[XPASS(strict)] 

unittest background

unittest also has expected-failure/unexpected-success support:

import unittest

class ExpectedFailureTestCase(unittest.TestCase):
    @unittest.expectedFailure
    def test_expected_failure(self):
        self.assertTrue(False)

    @unittest.expectedFailure
    def test_unexpected_success(self):
        self.assertTrue(True)

running:

xu
----------------------------------------------------------------------
Ran 2 tests in 0.000s

FAILED (expected failures=1, unexpected successes=1)
ran@ran:~/src/pytest$ py -m unittest -v ut.py 
test_expected_failure (ut.ExpectedFailureTestCase) ... expected failure
test_unexpected_success (ut.ExpectedFailureTestCase) ... unexpected success

----------------------------------------------------------------------
Ran 2 tests in 0.000s

FAILED (expected failures=1, unexpected successes=1)

pytest unittest plugin

Given the correspondence described above, when running the unittest under pytest I would expect test_expected_failure to be translated to an XFAIL -- this happens, and test_unexpected_success to be translated to an XPASS -- this doesn't happen:

ut.py::ExpectedFailureTestCase::test_expected_failure XFAIL                     [ 50%]
ut.py::ExpectedFailureTestCase::test_unexpected_success FAILED                  [100%]

====================================== FAILURES =======================================
___________________ ExpectedFailureTestCase.test_unexpected_success ___________________
Unexpected success
=============================== short test summary info ===============================
FAILED ut.py::ExpectedFailureTestCase::test_unexpected_success
============================ 1 failed, 1 xfailed in 0.13s =============================

To preserve the unittest behavior of unexpected-success being considered a failure, pytest should probably translate to xfail strict=True.

Implementation notes

The current behavior is implemented in the skipping and unittest plugins using a unexpectedsuccess_key hack. Ideally this hack would be removed in favor of using straight xfail, this would also be a good code cleanup. Not sure how hard that would be to achieve.

[bluetech]

While reviewing @antonblr's PR #8231, and trying to understand the (existing) code a bit more, I looked at the history around this.

Originally the skipping plugin didn't handle addUnexpectedSuccess, support was added in commit 58933aa as:

            # we need to translate into how py.test encodes xpass
            rep.keywords['xfail'] = "reason: " + item._unexpectedsuccess
            rep.outcome = "failed"

i.e. basically a straight FAILED.

Then there was PR #1795 (issue #1546), which changed the non-unittest code paths, but explicitly kept the unittest code path as failing

        if item._unexpectedsuccess:
            rep.longrepr = "Unexpected success: {0}".format(item._unexpectedsuccess)
        else:
            rep.longrepr = "Unexpected success"
         rep.outcome = "failed"

The reason being that this how unittest's expectedFailure behaves on success - it fails the test suite.

And now that I think about it again, I agree - for the unittest plugin, compatibility with unittest is more important than compatibility with pytest. So my issue was misguided here.

Really sorry @antonblr that it only comes up now, after you already prepared the PR :(

Nonetheless, I still think we can improve the situation on the current master -- instead of being a simple failure, it should be an XPASS(strict) failure, i.e. unittest should just force the strict=True flag. This way we're still compatible with unittest's behavior, but also use the proper pytest outcome. Although people might not understand what the "strict" is about... WDYT? (If we go that way, I think the PR could stay the same, only adding strict=True to the mark).

[antonblr]

I stepped back to rethink the case and I agree - unittest/twister tests should behave the same under both runners. If people using @expectedFailure, but want xfail behavior - perhaps they should replace marks with pytest's xfail.

Also, having [XPASS(strict)] ... in the outcome message for unuttests / twisted tests could be confusing. I looked a bit more and propose to use fail() directly in addUnexpectedSuccess, see updated PR