An unexpectedSuccess is like a failure not a success

Reported by Martin Packman on 2010-10-04
10
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Bazaar
Medium
Martin Packman
subunit
Wishlist
Robert Collins
testtools
Wishlist
Jonathan Lange

Bug Description

When the function passed to expectedFailure did not raise an assertion, bzr used to do:

    self.fail('Unexpected success. Should have failed: %s' % reason)

With testtools, bazaar now treats that as a success, as jam noted in February:
<https://lists.ubuntu.com/archives/bazaar/2010q1/066898.html>

And in September he pointed out the same thing, but with subunit:
<https://lists.ubuntu.com/archives/bazaar/2010q3/070192.html>

Subunit needs to roundtrip unexpected successes now that the standard library and testtools support them.

Martin Packman (gz) wrote :

What unittest does in Python 2.7:

    except _UnexpectedSuccess:
        addUnexpectedSuccess = getattr(result, 'addUnexpectedSuccess', None)
        if addUnexpectedSuccess is not None:
            addUnexpectedSuccess(self)
        else:
            warnings.warn("TestResult has no addUnexpectedSuccess method, reporting as failures",
                          RuntimeWarning)
            result.addFailure(self, sys.exc_info())

Changed in bzr:
importance: Undecided → Medium
status: New → Confirmed
Jonathan Lange (jml) on 2010-10-18
Changed in testtools:
status: New → Triaged
importance: Undecided → Low
Jonathan Lange (jml) on 2010-10-24
Changed in testtools:
status: Triaged → In Progress
assignee: nobody → Jonathan Lange (jml)
Jonathan Lange (jml) on 2010-10-25
Changed in testtools:
status: In Progress → Fix Committed
milestone: none → next
Martin Packman (gz) wrote :

The branch landed on testtools trunk is a start, but an unexpected success still passes silently. A TestResult should return False for wasSuccessful if an unexpected success has occured, and the TextTestRunner should print details of which tests these were.

Changed in testtools:
status: Fix Committed → In Progress
Jonathan Lange (jml) wrote :

I need to recap my understanding of the situation in order to be able to fix it. Please correct me if I've missed anything, got anything wrong or added unnecessary stuff:
  * We've fixed the problem of addUnexpectedSuccess falling back to addSuccess. It now falls back to addFailure.
  * An unexpected success in testtools will not fail the test run
  * Unexpected successes will not be printed by TextTestRunner

Looking at unittest in Python 2.7:
  * An unexpected success will not fail the test run
  * Unexpected successes will be printed in the incremental output of TextTestRunner
  * Unexpected successes will *not* be included in the summary of TextTestRunner

I'm genuinely not sure whether we should differ from unittest here. Rob, what are your thoughts? I'll try to knock up a patch in the mean time.

Robert Collins (lifeless) wrote :

I think its reasonable for unexpected successes to fail the test run (though a weaker statement is ok for some folk).

Separately I hold the view that that which fails the test run should be have its details shown to the user and be counted in any aggregates done.

Whatever happens, it ought to be documented and at the moment it
apparently is not.

Consistency with python unittest is worth something.

I see at the moment it is raised by expectFailure when the assertion
fails. I think it is a bad idea to have test expressions that can
silently or nearly silently change from passing to failing.

--
Martin

Jonathan Lange (jml) on 2010-11-29
Changed in testtools:
status: In Progress → Fix Committed
Robert Collins (lifeless) wrote :

mgz, what does subunit need to do here?

Changed in testtools:
importance: Low → Wishlist
Changed in testtools:
status: Fix Committed → Fix Released
Jonathan Lange (jml) on 2011-02-13
Changed in subunit:
status: New → Incomplete
Martin Packman (gz) wrote :

Specifically, TestTestProtocolClient methods test_add_unexpected_success and test_add_unexpected_success_details should fail if the output streams begin with "successful" as they do currently rather than "failure" or similar.

Changed in subunit:
status: Incomplete → Confirmed
Robert Collins (lifeless) wrote :

Ok, so you want to add another status code to the stream.

unexpectedsuccess is a combination of:
 - behaviour (the test did not raise an assertion)
 - expectation (the test was expected to raise an assertion)

We have the following status codes:
success (no exception, none expected)
failure ('assertion' exception, none expected)
error (other exception, none expected)
skip (not executed, no exception expected)
xfail (any exception, exception expected)

so adding a new status code does round that out. I remain dissatisfied with the modelling though; I would kind of like to separate the observed behaviour and the expected behaviour; a bit like jelmers patch to coerce xfail to success.

Something like:
status code becomes pure behavior:
completed
raised exception
not executed

expected behaviour becomes:
not supplied
completes
raises exception
not executed

unexpected success then is:
(outcome=completed, expected_outcome=raises exception)
xfail is:
(outcome=raises exception, expected_outcome=raises exception)

One problem with this is that xfail may be quite specific (raises /this/ exception) but subunit doesn't know enough to process that. A subject for TIP I think.

description: updated
Changed in subunit:
assignee: nobody → Robert Collins (lifeless)
importance: Undecided → Wishlist
Changed in subunit:
status: Confirmed → Fix Committed
milestone: none → next
Martin Packman (gz) wrote :

That sounds reasonable Robert. One thing I'm still not clear on with subunit is how free you are to change details of the format versus maintaining compatibility with the existing spec... which I couldn't find anywhere last time I looked.

Martin Packman (gz) on 2011-05-16
Changed in bzr:
assignee: nobody → Martin [gz] (gz)
status: Confirmed → In Progress
Martin Packman (gz) on 2011-05-18
Changed in bzr:
milestone: none → 2.4b3
status: In Progress → Fix Released
Changed in subunit:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers