testtools-dev team mailing list archive

[Jonathan Lange <jml@xxxxxxxxx>]

On Thu, Nov 11, 2010 at 9:52 AM, Robert Collins
<robertc@xxxxxxxxxxxxxxxxx> wrote:
> Robert Collins has proposed merging lp:~lifeless/testtools/matchers into lp:testtools.
>
> Requested reviews:
>  testtools developers (testtools-dev)
>
>
> This adds polished matchers to replace assertRaises, which I first prototyped in testrepository.
>
> The final commit on the branch uses the new Raises and MatchesException (ugh, perhaps a shorter name would be good?) throughout testtools own tests (except where we are testing assertRaises itself).
>

As I mention below, I don't see why Raises needs to take a matcher at
all. It could take an exception class / list of exception classes.

> I included this (largely mechanical) change in the branch so that the new matchers can be asessed properly. I considered a sugar to replace lambda: but actually found that the lambda: spelling grew on me pretty quickly: its much clearer about *what is being called* than the magic call-forward approach that assertRaises uses.

Well, I guess that's a matter of taste. I quite like the
wrapping_behaviour(function, *args, **kwargs) style. I'm not going to
block on it though, since people can still use assertRaises if they
want, and we can add a sugar later anyway.

Did you try applying this to assert_fails_with to see if it could help?

> === modified file 'MANUAL'
> --- MANUAL      2010-10-26 18:59:12 +0000
> +++ MANUAL      2010-11-11 09:52:45 +0000
> @@ -150,6 +150,13 @@
>         self.assertEqual('bob', error.username)
>         self.assertEqual('User bob cannot frobnicate', str(error))
>
> +Note that this is incompatible with the assertRaises in unittest2/Python2.7.
> +While we have no immediate plans to change to be compatible consider using the
> +new assertThat facility instead::
> +
> +        self.assertThat(thing.frobnicate,
> +            Raises(MatchesException(UnauthorisedError('bob')))
> +
>

Thanks for the manual update. Can you please include an example where
parameters are passed through?

>  TestCase.assertThat
>  ~~~~~~~~~~~~~~~~~~~
>
...
> === modified file 'testtools/matchers.py'
> --- testtools/matchers.py       2010-10-31 16:25:47 +0000
> +++ testtools/matchers.py       2010-11-11 09:52:45 +0000
...
> @@ -398,3 +439,46 @@
>
>     def describe(self):
>         return '%s: %s' % (self.mismatch.describe(), self.annotation)
> +
> +
> +class Raises(Matcher):
> +    """Match if the matchee raises an exception when called.
> +
> +    Exceptions which are not subclasses of Exception propogate out of the
> +    Raises.match call unless they are explicitly matched.
> +    """
> +
> +    def __init__(self, exception_matcher=None):
> +        """Create a Raises matcher.
> +
> +        :param exception_matcher: Optional validator for the exception raised
> +            by matchee. If supplied the exc_info tuple for the exception raised
> +            is passed into that matcher. If no exception_matcher is supplied
> +            then the simple fact of raising an exception is considered enough
> +            to match on.
> +        """
> +        self.exception_matcher = exception_matcher
> +

Why a matcher? All of the cases in the test suite have
Raises(MatchesException(FooError)). Why not Raises(FooError) and have
Raises simply use the MatchesException matcher?

...
> === modified file 'testtools/tests/test_content.py'
> --- testtools/tests/test_content.py     2010-10-28 22:06:41 +0000
> +++ testtools/tests/test_content.py     2010-11-11 09:52:45 +0000
> @@ -5,16 +5,21 @@
>  from testtools.compat import _u
>  from testtools.content import Content, TracebackContent, text_content
>  from testtools.content_type import ContentType, UTF8_TEXT
> +from testtools.matchers import MatchesException, Raises
>  from testtools.tests.helpers import an_exc_info
>
>
> +raises_value = Raises(MatchesException(ValueError))
> +

This should really be 'raises_value_error'. I missed the definition
and wondered "what value is this raising?"

> @@ -56,7 +61,7 @@
>  class TestTracebackContent(TestCase):
>
>     def test___init___None_errors(self):
> -        self.assertRaises(ValueError, TracebackContent, None, None)
> +        self.assertThat(lambda:TracebackContent(None, None), raises_value)
>
>     def test___init___sets_ivars(self):
>         content = TracebackContent(an_exc_info, self)
...

jml
-- 
https://code.launchpad.net/~lifeless/testtools/matchers/+merge/40606
Your team testtools developers is requested to review the proposed merge of lp:~lifeless/testtools/matchers into lp:testtools.