Skip to content
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.

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

Created calling/raises matcher #30

Closed
wants to merge 7 commits into from

Conversation

perfa
Copy link
Contributor

@perfa perfa commented Jan 25, 2013

New functional implementation of assert raises that uses the matcher style of hamcrest to check that exceptions are being raised as expected with the new matcher raises. There is a helper called calling that wraps a call with arguments to delay execution to where the matcher can catch any raised exception, but you can happily use raises on any function that takes no arguments. As a bonus, you can provide a regex to raises and it will look for that pattern in the stringification of the assertion - important in cases where a verification stage may throw variations of the same assertion.

Accepts any subclass of the exception provided. Does not try to run uncallable actuals.

assert_that(calling(parse, bad_data), raises(ValueError))
assert_that(calling(translate).with_(curse_words), raises(LanguageError, "\w+very naughty"))
assert_that(broken, raises(Exception))
# This will fail and complain that 23 is not callable
# assert_that(23, raises(IOError))

@offbyone
Copy link
Member

Your exception syntax is broken for 2.5 (sadly); check https://travis-ci.org/hamcrest/PyHamcrest/jobs/4385994.

I'm still looking at the code, though.

@perfa
Copy link
Contributor Author

perfa commented Jan 27, 2013

I knew that exception handling with as was new, and yet I appearantly couldn't remember to look up the compatible way to do it :P My bad.

The Jython build seems to have died due to config issues with travis(??)
/home/travis/build.sh: line 82: /home/travis/virtualenv/pythonjython2.5/bin/activate: No such file or directory

@offbyone
Copy link
Member

Yeah, Jython hasn't been working too well; that's why it's marked as an allowed failure.

Okay, so, I'm going to cut a 1.8 feature branch in the near future, and at that point I'll probably merge this in. I like the syntax, but I'm not completely wowed by the tests yet. I'll have to see what I think of them.

I also like with_args a bit more than with_ for the calling wrapper, and DeferredCallable for the wrapper itself.

assert_that(calling(callable).with_args(*pos_args, **kwargs), raises(...))

Lastly, what happens if one does this?:

assert_that(calling(callable, a, b, c=d).with_args(e, f=g), ...)

That should be checked for, or disallowed. I'm inclined to disallow creating a deferred callable with initial arguments completely; so, this would not work:

assert_that(calling(callable, a, b, c=d), ...)

And only this would:

assert_that(calling(callable).with_args(*pos_args, **kwargs),

description.append_text('Expected %s' % self.expected)

def describe_mismatch(self, item, description):
if not callable(item):
Copy link
Member

Choose a reason for hiding this comment

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

There's a bit of an issue here; callable is notably missing in Python 3 until (I believe) 3.2. We'll need to implement one of the alternatives suggested along these lines: http://stackoverflow.com/questions/4383414/alternative-to-callable-for-use-in-python-3

@perfa
Copy link
Contributor Author

perfa commented Jan 29, 2013

I'm out of the country all week on business. I'll take a look at what the issues when I get back.

@perfa
Copy link
Contributor Author

perfa commented Feb 10, 2013

I'm aboard for the name changes, defferedCallable and with_args. The motivation for the two ways of proving arguments was to provide a short-hand for simpler calls, but I can agree that it's confusing with both and not terribly pretty passing it in to the constructor.

You briefly mention the tests, do you recall what disturbed you? The coverage, the quality or the style?

@offbyone
Copy link
Member

State is the issue, both in the tests and in the matcher itself. Basically, the design of Hamcrest is such that a matcher can be created once and reused multiple times, passed into other contexts, and makes no guarantees about when and how its public methods will be called.

So, it's not a great place to be, retaining the resulting exception from the callable as you have in self.actual on the matcher. Obviously, the matcher must only call the callable it's provided once. Otherwise side effects may occur, and while that's probably okay in a testing case, it's not acceptable if we're using these matchers in production code.

Basically, you need some way to get a match / description result together, and to get the same value separately -- I should be able to call matcher.matches(item) and later call matcher.mismatch_description(another_item, description) without having to create a new matcher.

I'm not sure I'm being totally clear here; it's early and I'm still not fully caffeinated :)

@perfa
Copy link
Contributor Author

perfa commented Feb 10, 2013

Ah! I see, I didn't think about the re-use potential of the matcher. That does make things trickier...

So you (may) need to make the call/catch in mismatch_description, but if a call has side-effects you obviously don't want to do a second call. The only real solution that comes to mind is as you mentioned to create a weakref to item in matches and then see if that same one is being used in the mismatch_description, and if not call the newly provided another_item to set the self.actual. I'm not sure I really grasp the use case you're outlining, so I'll leave it to you to say if that seems like a workable path or if it's back to the drawing board.

@offbyone
Copy link
Member

Another approach might be to make a weakref to the deferred callable, maybe? I'm not sure where the scope should be, to be honest; my garbage-collector-fu is a bit weak for Python -- I'm not sure where the weakref needs to be to handle this.

Honestly, I'm interested to see the solution, in part because I'm not sure what it should be :)

@keis
Copy link
Contributor

keis commented Mar 13, 2013

In one project that used hamcrest for the tests I took another approach to this which I think worked well. It steals the contextmanager idea from unittest and I could write code like

with assert_raises(instance_of(SomeException)):
    call_some_method()

not sure if that is something that fits with the bigger design of the api as it introduces another assert function besides assert_that. But I think it yields more readable code as the body that will raise is just plain python, which get even more important if you want to test that you get a KeyError when doing some foo['lookup'] which could look really ugly otherwise.

the implementation looked like this

@contextmanager
def assert_raises(matcher=None, message=''):
    # Short hand for instance_of matcher
    if isinstance(matcher, (type,)):
        matcher = instance_of(matcher)

    context = RaisesContext()
    try:
        yield context
    except Exception as e:
        context.exception = e

    assert_that(context.exception, matcher, message)

@offbyone
Copy link
Member

@keis What you've described there doesn't require anything new in hamcrest, it can be acheived using the unittest assertRaises context manager and the hamcrest.library.integration.match_equality.match_equality matcher:

from hamcrest import match_equality
class UT(unittest.TestCase):
    def test_foo(self):
        with self.assertRaises(match_equality(some_matcher)):
            exception_raiser()

Certainly, it should work like that; if it doesn't that's a bug in the integration wrapper.

@keis
Copy link
Contributor

keis commented Mar 13, 2013

I've might have missed what the problem this pull was trying to solve is. Was just trying to point out that a small helper donig try/catch/assert could already cover the same usecases and might be a easier "fix".

And as you point if using unittest.TestCase its even less code so there's not even a need for a helper.

@perfa
Copy link
Contributor Author

perfa commented Apr 28, 2013

I've broken out the actually try/except to a function and retain a weakref to the original callable. This lets the matcher's describe_mismatch be called with any other callable with correct results without locking up memory. In theory anyway, what works correctly with weakref does vary from Python versions, though I'm reasonable sure that when used with the deferred callable it will work as intended for all supported versions.

__license__ = "BSD, see License.txt"


def _is_callable(function):
Copy link
Member

Choose a reason for hiding this comment

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

is_callable has an implementation in Python 2.x and in 3.3+; you should use that. Ideally, we may want to have a hamcrest/core/compat.py that provides the right is_callable for us.

@perfa
Copy link
Contributor Author

perfa commented May 22, 2013

I started on a compat.py but ran into questions when fixing up the documentation. My personal preference is for not wrapping callable in a method, just assigning it to is_callable when available. However, this make automodule not work in the rst file. Adding an all variable with 'is_callable' works as a work-around, but the question is does the PyHamcrest project has a preference for avoiding that if possible? (In that case I can just wrap callable in a proper method definition.)

@offbyone
Copy link
Member

Honestly, I'd rather see is_callable excluded from the API documentation completely, since it's just an internal utility method.

@offbyone
Copy link
Member

Also, in our back-and-forth over this, you've had some good examples that could go in the tutorial section of the doc for how to use this. Could you add those there?

@offbyone offbyone closed this in 9bd9cde Jul 7, 2013
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ef7d987 on perfa:feature_raises_matcher into * on hamcrest:master*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants