-
Notifications
You must be signed in to change notification settings - Fork 113
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
Add matcher for exceptions in asyncio future #171
Conversation
Ah - doing this in a way which is compatible with Python versions prior to 3.9 is going to be... interesting. FYI, PyHamcrest works with versions 3.6 and up as of now - all currently supported versions. |
I don't think the await syntax should be a problem it was introduced in 3.4 (Or there about) IIRC, and before that there was the 'yield from' stuff that should work even if not as pretty to look at :) |
Oh, cool - but something's not working with older versions. FWIW, I usually do Nice change, BTW - I do love to see tests and docs in a PR. @offbyone has added towncrier to the build, so at some point we;'ll need an entry in |
So turns out there was two issues I'll update with some more tests etc later :) |
src/hamcrest/core/core/future.py
Outdated
match_description.append_text("%r of type %s was raised." % (exc, type(exc))) | ||
|
||
|
||
def future_exception( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder just from a literate coding PoV if this should be (or at least have as an alias) asynchronously_raises
instead of future_exception
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit torn on this but I'll admit I mostly find the literal coding concerns to be a distraction.
Technically it can match a Future object no matter how you produce it and it's not entirely correct to say it raises the exception because it was already raised but captured.
I'm happy to defer this decision to you though :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about assert_that(actual_future, is(future_raising(ValueError)))
, for clarity I think I'd prefer to have future still in the name.
Also, oops. did not mean to forget about this pull request for 2 years. I'll rebase if there's some interest in this approach :)
|
||
Examples:: | ||
|
||
assert_that(somefuture, future_exception(ValueError)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be leaning in to assert_that()
here, or conceiving of a different assert wrapper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about creating a variant that itself could by await
ed but I think that would be much more complicated and impact how matcher and everything works. This is really the huge problem with asyncio (and async code in general) it tends to just infect everything and spread to all corners of your code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not averse to seeing this merge, but I think what I'm missing before we do is documentation that shows how one would write a test with this matcher. Can you please add docs showing how it'll handle event loops, resolving the future, etc, not from the perspective of our side where we're implementing it, but from the perspective of the writer of tests using it.
src/hamcrest/core/core/future.py
Outdated
return False | ||
|
||
def describe_to(self, description: Description) -> None: | ||
description.append_text("Expected a future with exception %s" % self.expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading the above right, it'd be more true to say "Expected a completed future ..." etc, right?
Example of use ``` assert_that( await resolved(raise_exception()), future_raising(AssertionError)) ) ``` The resolved helper is used to create resolved future objects in async code. It takes a "future like" object and waits for it to complete. Ref hamcrest#155
Made an attempt at some docs not sure if the tutorial is the right place though. |
When will a new release be published with this change? |
That's very helpful feature. It would be nice to get it there. |
I just published 2.1.0 to PyPi for you; enjoy! |
Hamcrest 2.1.0 (2023-10-22) =========================== Features -------- - Add a matcher for exceptions in asyncio future (`#171 <https://github.com/hamcrest/PyHamcrest/issues/171>`_) Bugfixes -------- - Use the correct generic type in the internal ``describe_keyvalue`` method (`#182 <https://github.com/hamcrest/PyHamcrest/issues/182>`_)
WIP: Based on raises matcher but adapted to deal with future objects.
Example of use
The resolved helper is used to create resolved future objects in async
code. It takes a "future like" object and waits for it to complete.
Ref #155