An unexpectedSuccess is like a failure not a success

[Martin Packman]

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)]

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())

[Martin Packman (gz)]

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.

[Jonathan Lange (jml)]

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)]

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.

[Martin Pool (mbp)]

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

[Robert Collins (lifeless)]

mgz, what does subunit need to do here?

[Martin Packman (gz)]

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.

[Robert Collins (lifeless)]

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.

[Martin Packman (gz)]

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.