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

Implement ExceptionInfo.match() method #1502

Merged
merged 2 commits into from Apr 3, 2016

Conversation

Projects
None yet
4 participants
@omarkohl
Copy link
Contributor

omarkohl commented Apr 3, 2016

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

  • Target: for bug or doc fixes, target master; for new features, target features
  • Make sure to include one or more tests for your change
  • Add yourself to AUTHORS
  • Add a new entry to the CHANGELOG (choose any open position to avoid merge conflicts with other PRs)

omarkohl added some commits Apr 2, 2016

Implement ExceptionInfo.match() to match regexp on str(exception)
This implements similar functionality to
unittest.TestCase.assertRegexpMatches()

closes #372
@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Apr 3, 2016

Excellent! Many thanks! 😁

@nicoddemus nicoddemus merged commit 909d72b into pytest-dev:features Apr 3, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Apr 3, 2016

i just noticed this, we should name it must_match at least, the method name doesn't show this is an exception

i propose a matches method to just do the search and a must_match method to do the assertion

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Apr 3, 2016

i just noticed this, we should name it must_match at least, the method name doesn't show this is an exception

Hmm that's a good point.

i propose a matches method to just do the search and a must_match method to do the assertion

I would rather have a single one... how about just renaming it to must_match?

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Apr 3, 2016

well, its without a method only doing the matching it wont be possible to assert negative matches
this is why i proposed 2 methods, one for matching, one for assertion, at some point we'd like to get booleans with the explanation embedded instead of methods raising assertion errors

@omarkohl

This comment has been minimized.

Copy link
Contributor Author

omarkohl commented Apr 3, 2016

match (or must_match) doesn't have to raise an exception... I did it because to quote @flub

assert excinfo.match(r'...')
(the assert is optional there but I think it should be supported)

This is how I interpret the assert is optional.

But the assert could be mandatory as well (i.e. excinfo.match(r'...') returns True or False). The disadvantage is of course that the error message will not be as precise when it doesn't match:

with pytest.raises(Exception) as excinfo:
    function_to_test()
assert excinfo.match(r'...') is True
@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Apr 3, 2016

@omarkohl there is a plan for booleans that explain their cause

so

with pytest.raises(Exception) as excinfo:
    function_to_test()
assert excinfo.matches(r'...')

could generate explanations just like the current custom assertion does

@omarkohl

This comment has been minimized.

Copy link
Contributor Author

omarkohl commented Apr 3, 2016

If you agree on something I'll implement it :-)

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Apr 3, 2016

@RonnyPfannschmidt would have to elaborate on what he means, as I'm not entirely sure myself

@omarkohl

This comment has been minimized.

Copy link
Contributor Author

omarkohl commented Apr 3, 2016

@RonnyPfannschmidt does have a point that maybe the name match does not convey that if it doesn't match an AssertionError is raised. And also it is difficult to assert negative matches (verify an exception does not match a certain regex).

Points to consider:

  • Won't it become quickly obvious that match raises an exception and people will get used to it?
  • The method is supposed to simplify this use-case. It is always possible to do import re;re.match(r'...', str(excinfo.value)) to implement complex logic
  • Is it necessary to assert negative matches? Would that not be so specialized that importing re and implementing the logic by hand would be fine?

@omarkohl omarkohl deleted the omarkohl:excinfo_match branch Apr 3, 2016

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Apr 3, 2016

my personal opinion is that there should be 2 methods, one for matching, one for the commonly used assertion

as for the consideration points

  • it should never be expected of an api user to learn about surprises
  • correct, its always possible to implement those since they are simple
  • the ability for negation is just an issue of composition and ease of use,
    its not strictly necessary, just part of a symmetric system to ease use
    by fitting with commenly held expectations
@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Apr 3, 2016

TBH I don't see much need for two methods... but I'm not against having both either.

@The-Compiler

This comment has been minimized.

Copy link
Member

The-Compiler commented Apr 4, 2016

I don't think it's a good idea to have two methods which do the same thing, except they subtly don't 😉

I'm also okay with excinfo.match doing the assertion, just like pytest.raises, or LineMatcher.fnmatch_lines do already. Why not be consistent with what we have already and what people are used to?

Also, if someone forgets the assert with a non-asserting version (because they're used to how e.g. fnmatch_lines works) their test will silently pass, which is never a good thing.

And I'm with @omarkohl here - is there really a usecase for a non-asserting ExcInfo.match? I don't think so, and if people really want it, it's trivial to implement it by themselves using re.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.