Skip to content

Include output in passing tests; fix expected failures; adjust verbose output #186

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Mar 17, 2019
Merged

Include output in passing tests; fix expected failures; adjust verbose output #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23f4038
always include output and info for testcases
danchr Mar 15, 2019
476b2f0
adjust output to correspond to unittest
danchr Mar 15, 2019
b3b8fc2
_test_xmlrunner: allow passing a BytesIO as outdir
danchr Mar 15, 2019
7c4d6a2
properly include expected failures & unexpected successes in output
danchr Mar 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 47 additions & 4 deletions tests/testsuite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,18 +185,25 @@ def setUp(self):
self.runner_kwargs = {}
self.addCleanup(rmtree, self.outdir)

def _test_xmlrunner(self, suite, runner=None):
outdir = self.outdir
def _test_xmlrunner(self, suite, runner=None, outdir=None):
if outdir is None:
outdir = self.outdir
stream = self.stream
verbosity = self.verbosity
runner_kwargs = self.runner_kwargs
if runner is None:
runner = xmlrunner.XMLTestRunner(
stream=stream, output=outdir, verbosity=verbosity,
**runner_kwargs)
self.assertEqual(0, len(glob(os.path.join(outdir, '*xml'))))
if isinstance(outdir, BytesIO):
self.assertFalse(outdir.getvalue())
else:
self.assertEqual(0, len(glob(os.path.join(outdir, '*xml'))))
runner.run(suite)
self.assertEqual(1, len(glob(os.path.join(outdir, '*xml'))))
if isinstance(outdir, BytesIO):
self.assertTrue(outdir.getvalue())
else:
self.assertEqual(1, len(glob(os.path.join(outdir, '*xml'))))
return runner

def test_basic_unittest_constructs(self):
Expand Down Expand Up @@ -230,6 +237,42 @@ def test_classnames(self):
r'classname="tests\.testsuite\.(XMLTestRunnerTestCase\.)?'
r'DummySubTest" name="test_subTest_pass"'.encode('utf8'))

def test_expected_failure(self):
suite = unittest.TestSuite()
suite.addTest(self.DummyTest('test_expected_failure'))
outdir = BytesIO()

self._test_xmlrunner(suite, outdir=outdir)

self.assertNotIn(b'<failure', outdir.getvalue())
self.assertNotIn(b'<error', outdir.getvalue())
self.assertIn(b'<skip', outdir.getvalue())

def test_unexpected_success(self):
suite = unittest.TestSuite()
suite.addTest(self.DummyTest('test_unexpected_success'))
outdir = BytesIO()

self._test_xmlrunner(suite, outdir=outdir)

self.assertNotIn(b'<failure', outdir.getvalue())
self.assertIn(b'<error', outdir.getvalue())
self.assertNotIn(b'<skip', outdir.getvalue())

def test_xmlrunner_safe_xml_encoding_name(self):
Copy link
Member

@dnozay dnozay Mar 15, 2019

Choose a reason for hiding this comment

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

duplicate test, issue with coverage

suite = unittest.TestSuite()
suite.addTest(self.DummyTest('test_pass'))
outdir = BytesIO()
runner = xmlrunner.XMLTestRunner(
stream=self.stream, output=outdir, verbosity=self.verbosity,
**self.runner_kwargs)
runner.run(suite)
outdir.seek(0)
output = outdir.read()
firstline = output.splitlines()[0]
# test for issue #74
self.assertIn('encoding="UTF-8"'.encode('utf8'), firstline)

def test_xmlrunner_non_ascii(self):
suite = unittest.TestSuite()
suite.addTest(self.DummyTest('test_non_ascii_skip'))
Expand Down
75 changes: 49 additions & 26 deletions xmlrunner/result.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,13 @@ class _TestInfo(object):
# Possible test outcomes
(SUCCESS, FAILURE, ERROR, SKIP) = range(4)

OUTCOME_ELEMENTS = {
SUCCESS: None,
FAILURE: 'failure',
ERROR: 'error',
SKIP: 'skipped',
}

def __init__(self, test_result, test_method, outcome=SUCCESS, err=None, subTest=None):
self.test_result = test_result
self.outcome = outcome
Expand Down Expand Up @@ -306,7 +313,7 @@ def addSuccess(self, test):
"""
self._save_output_data()
self._prepare_callback(
self.infoclass(self, test), self.successes, 'OK', '.'
self.infoclass(self, test), self.successes, 'ok', '.'
)

@failfast
Expand Down Expand Up @@ -373,21 +380,23 @@ def addSkip(self, test, reason):
self._save_output_data()
testinfo = self.infoclass(
self, test, self.infoclass.SKIP, reason)
testinfo.test_exception_name = 'skip'
testinfo.test_exception_message = reason
self.skipped.append((testinfo, reason))
self._prepare_callback(testinfo, [], 'SKIP', 'S')
self._prepare_callback(testinfo, [], 'skip', 's')

def addExpectedFailure(self, test, err):
"""
Missing in xmlrunner, copy-pasted from xmlrunner addError.
"""
self._save_output_data()

testinfo = self.infoclass(self, test, self.infoclass.ERROR, err)
testinfo.test_exception_name = 'ExpectedFailure'
testinfo.test_exception_message = 'EXPECTED FAILURE: {}'.format(testinfo.test_exception_message)
testinfo = self.infoclass(self, test, self.infoclass.SKIP, err)
Copy link
Member

@dnozay dnozay Mar 15, 2019

Choose a reason for hiding this comment

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

https://docs.python.org/3/library/unittest.html#unittest.expectedFailure

Mark the test as an expected failure. If the test fails it will be considered a success. If the test passes, it will be considered a failure.

Can you please elaborate why it should be marked as SKIP instead of ERROR or FAILURE?
To me a failure is a failure, expected or not; so I'm not sure I understand the change.

Copy link
Member

@dnozay dnozay Mar 15, 2019

Choose a reason for hiding this comment

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

Can you actually split the changes and address this semantic change separately? I'm okay merging the rest.

Copy link
Contributor

@timgraham timgraham Mar 16, 2019

Choose a reason for hiding this comment

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

I'll say that for Django's test suite, a recent change in unittest-xml-reporting
is making Jenkins marking our builds as "Unstable" rather than "Success" due to tests marked with @unittest.expectedFailure failing (as exected).

Copy link
Member

@dnozay dnozay Mar 16, 2019

Choose a reason for hiding this comment

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

@timgraham, could be unrelated to this PR but related to some of my changes. please file a ticket with more info... I don't see django/django in travis, so I'm not sure which CI is used.

Copy link
Contributor

@timgraham timgraham Mar 16, 2019

Choose a reason for hiding this comment

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

What I meant is that I think this change would fix that issue. Possibly the regression is due to cc05679. Our CI is at https://djangoci.com/.

Copy link
Contributor Author

@danchr danchr Mar 18, 2019

Choose a reason for hiding this comment

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

https://docs.python.org/3/library/unittest.html#unittest.expectedFailure

Mark the test as an expected failure. If the test fails it will be considered a success. If the test passes, it will be considered a failure.

Can you please elaborate why it should be marked as SKIP instead of ERROR or FAILURE?
To me a failure is a failure, expected or not; so I'm not sure I understand the change.

An expected failure is a way of marking as “incorrect” — a good example might be a test that is in fact correct, but which tests functionality not currently working. The entire point of an expected failure is to ensure that the test suite remains passing while semantically marking a test as “wrong” or “broken”.

Compared to the obvious alternative of skipping the test, the main feature of expectedFailure is that the test run actually fails if the test suddenly starts working, preventing it from happening accidentally. From the CPython 3.7 sources:

    def wasSuccessful(self):
        """Tells whether or not this result was a success."""
[snip]
        return ((len(self.failures) == len(self.errors) == 0) and
                (not hasattr(self, 'unexpectedSuccesses') or
                 len(self.unexpectedSuccesses) == 0))

Given that the JUnit XML does not support this feature, a “skip” is reasonably close approximation, although one might also mark it as a success. The latter feels a bit wrong to me. An unexpected success, however, is a clear failure.

As with Django, the motivation for me was that our test suite suddenly started becoming “unstable” on Jenkins — I'm suspect that wasn't the case with the code I originally submitted, but you never know…

testinfo.test_exception_name = 'XFAIL'
testinfo.test_exception_message = 'expected failure: {}'.format(testinfo.test_exception_message)

self.expectedFailures.append((testinfo, self._exc_info_to_string(err, test)))
self._prepare_callback(testinfo, [], 'EXPECTED FAILURE', 'X')
self._prepare_callback(testinfo, [], 'expected failure', 'x')

@failfast
def addUnexpectedSuccess(self, test):
Expand All @@ -400,11 +409,11 @@ def addUnexpectedSuccess(self, test):
testinfo.outcome = self.infoclass.ERROR
# But since we want to have error outcome, we need to provide additional fields:
testinfo.test_exception_name = 'UnexpectedSuccess'
testinfo.test_exception_message = ('UNEXPECTED SUCCESS: This test was marked as expected failure but passed, '
testinfo.test_exception_message = ('Unexpected success: This test was marked as expected failure but passed, '
'please review it')

self.unexpectedSuccesses.append(testinfo)
self._prepare_callback(testinfo, [], 'UNEXPECTED SUCCESS', 'U')
self.unexpectedSuccesses.append((testinfo, 'unexpected success'))
self._prepare_callback(testinfo, [], 'unexpected success', 'u')

def printErrorList(self, flavour, errors):
"""
Expand Down Expand Up @@ -531,25 +540,39 @@ def _report_testcase(test_result, xml_testsuite, xml_document):
testcase.setAttribute('time', '%.3f' % test_result.elapsed_time)
testcase.setAttribute('timestamp', test_result.timestamp)

if (test_result.outcome != test_result.SUCCESS):
elem_name = ('failure', 'error', 'skipped')[test_result.outcome-1]
failure = xml_document.createElement(elem_name)
testcase.appendChild(failure)
if test_result.outcome != test_result.SKIP:
failure.setAttribute(
'type',
test_result.test_exception_name
)
failure.setAttribute(
'message',
test_result.test_exception_message
)
if test_result.stdout:
Copy link
Member

@dnozay dnozay Mar 15, 2019

Choose a reason for hiding this comment

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

nit: pep8, we shouldn't use if x when we mean if x is not None

systemout = xml_document.createElement('system-out')
testcase.appendChild(systemout)

_XMLTestResult._createCDATAsections(xml_document, systemout,
test_result.stdout)

if test_result.stderr:
systemerr = xml_document.createElement('system-err')
testcase.appendChild(systemerr)

_XMLTestResult._createCDATAsections(xml_document, systemerr,
test_result.stderr)


result_elem_name = test_result.OUTCOME_ELEMENTS[test_result.outcome]

if result_elem_name:
result_elem = xml_document.createElement(result_elem_name)
testcase.appendChild(result_elem)

result_elem.setAttribute(
'type',
test_result.test_exception_name
)
result_elem.setAttribute(
'message',
test_result.test_exception_message
)
if test_result.get_error_info():
error_info = safe_unicode(test_result.get_error_info())
_XMLTestResult._createCDATAsections(
xml_document, failure, error_info)
else:
failure.setAttribute('type', 'skip')
failure.setAttribute('message', test_result.test_exception_message)
xml_document, result_elem, error_info)

if test_result.stdout:
systemout = xml_document.createElement('system-out')
Expand Down