Code review comment for lp:~lifeless/subunit/xfail

On Thu, Aug 6, 2009 at 10:10 AM, Robert
Collins<email address hidden> wrote:
> Robert Collins has proposed merging lp:~lifeless/subunit/xfail into lp:subunit.
>
> Requested reviews:
>    Subunit Developers (subunit)
>
> make xfail use addExpectedFailure

Hey Rob,

Only a few comments here.

> === modified file 'NEWS'
> --- NEWS 2009-08-02 22:55:36 +0000
> +++ NEWS 2009-08-06 08:48:01 +0000
> @@ -32,6 +32,11 @@
>
> BUG FIXES:
>
> + * ``xfail`` outcomes are now passed to python TestResult's via
> + addExpectedFailure if it is present on the TestResult. Python 2.6 and
> + earlier which do not have this function will have ``xfail`` outcomes
> + passed through as success outcomes as earlier versions of subunit did.
> +
> API CHANGES:
>
> * When a progress: directive is encountered in a subunit stream, the
>
> === modified file 'python/subunit/__init__.py'
> --- python/subunit/__init__.py 2009-08-02 22:55:36 +0000
> +++ python/subunit/__init__.py 2009-08-06 08:48:01 +0000
> @@ -120,7 +120,11 @@
> self.current_test_description == line[offset:-1]):
> self.state = TestProtocolServer.OUTSIDE_TEST
> self.current_test_description = None
> - self.client.addSuccess(self._current_test)
> + xfail = getattr(self.client, 'addExpectedFailure', None)
> + if callable(xfail):

Is there a way to avoid using 'callable' here? It's a little look before you
leap, and is not present in Python 3.

One way to do it would be to call it if not None, and catch a TypeError.

> + xfail(self._current_test, RemoteError())
> + else:
> + self.client.addSuccess(self._current_test)
> self.client.stopTest(self._current_test)
> elif (self.state == TestProtocolServer.TEST_STARTED and
> self.current_test_description + " [" == line[offset:-1]):
> @@ -203,10 +207,16 @@
> self.current_test_description = None
> self._skip_or_error(self._message)
> self.client.stopTest(self._current_test)
> - elif self.state in (
> - TestProtocolServer.READING_SUCCESS,
> - TestProtocolServer.READING_XFAIL,
> - ):
> + elif self.state == TestProtocolServer.READING_XFAIL:
> + self.state = TestProtocolServer.OUTSIDE_TEST
> + self.current_test_description = None
> + xfail = getattr(self.client, 'addExpectedFailure', None)
> + if callable(xfail):
> + xfail(self._current_test, RemoteError(self._message))

This duplicates a bit of logic above.

> + else:
> + self.client.addSuccess(self._current_test)
> + self.client.stopTest(self._current_test)
> + elif self.state == TestProtocolServer.READING_SUCCESS:
> self._succeedTest()
> else:
> self.stdOutLineReceived(line)
>
> === modified file 'python/subunit/tests/test_test_protocol.py'
> --- python/subunit/tests/test_test_protocol.py 2009-08-02 22:55:36 +0000
> +++ python/subunit/tests/test_test_protocol.py 2009-08-06 08:48:01 +0000
> @@ -28,7 +28,11 @@
>
>
> class MockTestProtocolServerClient(object):
> - """A mock protocol server client to test callbacks."""
> + """A mock protocol server client to test callbacks.
> +
> + Note that this is deliberately not python 2.7 complete, to allow

'Python' not 'python'.

> + testing compatibility.

Can you please elaborate on _why_ not supporting Python 2.7 completely allows
testing compatibility?

> + """
>
> def __init__(self):
> self.end_calls = []

Thanks,
jml