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
Merged

Implement ExceptionInfo.match() method #1502

merged 2 commits into from
Apr 3, 2016

Conversation

omarkohl
Copy link
Contributor

@omarkohl 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)

This implements similar functionality to
unittest.TestCase.assertRegexpMatches()

closes #372
@nicoddemus
Copy link
Member

Excellent! Many thanks! 😁

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

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
Copy link
Member

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
Copy link
Member

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
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
Copy link
Member

@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
Copy link
Contributor Author

omarkohl commented Apr 3, 2016

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

@nicoddemus
Copy link
Member

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

@omarkohl
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 excinfo_match branch April 3, 2016 17:43
@RonnyPfannschmidt
Copy link
Member

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
Copy link
Member

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

@The-Compiler
Copy link
Member

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants