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

raises regexp #372

Closed
pytestbot opened this issue Oct 19, 2013 · 26 comments
Closed

raises regexp #372

pytestbot opened this issue Oct 19, 2013 · 26 comments
Labels
good first issue easy issue that is friendly to new contributor type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@pytestbot
Copy link
Contributor

Originally reported by: György Kiss (BitBucket: kissgyorgy, GitHub: kissgyorgy)


I used assertRaisesRegexp in unittest and I miss it from pytest. As I don't want to use unittest anymore :D I wrote a context manager:

#!python

import pytest
import py

class raises_regexp(object):
    def __init__(self, exception, message):
        self.exception = exception
        self.message = message
        self.excinfo = None

    def __enter__(self):
        self.excinfo = object.__new__(py.code.ExceptionInfo)
        return self.excinfo

    def __exit__(self, exc_type, exc_val, exc_tb):
        __tracebackhide__ = True
        if exc_type is None:
            pytest.fail('DID NOT RAISE %s' % self.exception)

        self.excinfo.__init__((exc_type, exc_val, exc_tb))

        if not issubclass(self.excinfo.type, self.exception):
            pytest.fail('%s RAISED instead of %s' % (exc_type, self.exception))

        if self.message not in str(exc_val):
            pytest.fail('message "%s" not found in "%s"' % (self.message, str(exc_val)))

        return True

Usage:

#!python

def test_some_exception_thrown():
    with raises_regexp(ExpectedException, "message contained"):
       # code to test

I think others could benefit from something like this and would be nice to have it in pytest!


@pytestbot
Copy link
Contributor Author

Original comment by György Kiss (BitBucket: kissgyorgy, GitHub: kissgyorgy):


I made a plugin for this if anyone interested: https://github.com/Walkman/pytest_raisesregexp

@pytestbot
Copy link
Contributor Author

Original comment by holger krekel (BitBucket: hpk42, GitHub: hpk42):


What about adding a keyword argument so this would work:

#!python

with pytest.raises(ValueError, regex=r'...'):
      ...

@pytestbot
Copy link
Contributor Author

Original comment by Ivan Smirnov (BitBucket: aldanor, GitHub: aldanor):


@walkman_ I'd modify your example code slightly for it to behave more like pytest.raises:

class raises_regexp(object):
    def __init__(self, exception, message, *args, **kwargs):
        self.exception = exception
        self.message = message
        self.excinfo = None
        if args:
            with self:
                args[0](*args[1], **kwargs)

@pytestbot
Copy link
Contributor Author

Original comment by Ivan Smirnov (BitBucket: aldanor, GitHub: aldanor):


@hpk42 What do you think about all this? This is one feature that's available in unittest but not available in pytest and it's quite useful.

I think adding a separate function would be cleaner since it would allow you use it same way as pytest.raises without a with block:

pytest.raises_regexp(ValueError, 'expected message', func, 1, 2, a=3, b=4)

Adding a regex keyword argument to pytest.raises wouldn't allow you do use it without a context manager due to ambiguity.

@pytestbot
Copy link
Contributor Author

Original comment by Floris Bruynooghe (BitBucket: flub, GitHub: flub):


Would it be very wrong to only support a new regex keyword when used as a context manager? I think the non-context manager only exists because when fist introduced python 2.5 (or earlier) was still supported, but now it should probably be discouraged.

@pytestbot
Copy link
Contributor Author

Original comment by Bruno Oliveira (BitBucket: nicoddemus, GitHub: nicoddemus):


I wouldn't mind very much about regex being supported only as a context manager, that's pretty much the only form I use these days...

Anyway, I miss this functionality too and would love to have it built-in, having turned to pytest-raisesregexp in the past. :)

@pytestbot
Copy link
Contributor Author

Original comment by Ivan Smirnov (BitBucket: aldanor, GitHub: aldanor):


I use a modified version of this pretty much exclusively without a context manager: less typing required, can use it like a (partial) function, etc :)

@pytestbot pytestbot added the type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature label Jun 15, 2015
@RonnyPfannschmidt
Copy link
Member

proposal;

with pytest.raises.matching(ValueError, '.*Regex'):
  ...

@RonnyPfannschmidt RonnyPfannschmidt added the good first issue easy issue that is friendly to new contributor label Jul 25, 2015
@RonnyPfannschmidt
Copy link
Member

the builtin solution should modify the context manager and use the regex condition based on the value of the regex attribute

@foxx
Copy link
Contributor

foxx commented Aug 26, 2015

I was almost tempted to use pytest, until I saw this issue :/ Any particular reason why this hasn't been added yet?

@The-Compiler
Copy link
Member

Probably a mixture between "nobody has looked at it yet" and "there is a plugin for it" and "it's easy enough to do without pytest support":

with pytest.raises(ValueError) as excinfo:
    func()

assert re.match(pattern, str(excinfo.value))

But since many people have said they'd appreciate such a feature, and nobody seems to have had time yet, I'm sure a contribution would we very welcome!

@nicoddemus
Copy link
Member

I could tackle this, but would like to know what is the preferred interface:

  1. From pytest-raisesregexp:

    with pytest.raises_regexp(ExpectedException, r".* 1560"):
          function_to_test(arg1)
    
    pytest.raises_regexp(ExpectedException, r".* 1560", function_to_test, arg1):
  2. Proposed by @RonnyPfannschmidt:

    with pytest.raises.matching(ValueError, 'regex'):
        function_to_test(arg1)
    
    pytest.raises.matching(ValueError, 'regex', function_to_test, arg1)
  3. Any other suggestion? 😄

What do you guys think?

@nicoddemus
Copy link
Member

(Just posted a tweet about this).

@The-Compiler
Copy link
Member

Another proposal from @hpk42 was to add a keyword argument to pytest.raises:

with pytest.raises(ValueError, regex=r'...'):
      ...

That has the downside of being only usable in context manager form.

I'd still prefer this form, though, as I don't think the non-contextmanager form is used widely anyways.

pytest.raises.matching(...) looks good, but I think it might be confusing for people when there's a function as an attribute of another function.

@sscherfke
Copy link
Contributor

I would also prefer @hpk42 ’s proposal.

But in this case you'd have to check if the second argument is a non-keyword argument, which would mean that it is the function to be called. In that case, the regex kwarg would be an argument for that function (and not for raises()).

This seems a little dirty to me, but on the other hand, it would also make it possible to add additional keyword arguments like func, args and kwargs to do stuff like:

# "regex" as argument for "myfunc":
pytest.raises(ValueError, myfunc, regex=r'spam')

# "regex" as argument for "raises":
pytest.raises(
    ValueError,
    regex=r'spam',
    func=myfunc,
    args=(23, 'foo'),
    kwargs={'spam': 42})

@nicoddemus
Copy link
Member

Another proposal from @hpk42 was to add a keyword argument to pytest.raises

Thanks for the reminder @The-Compiler! We can call this option 3, if people want to reference the options by number:

with pytest.raises(ValueError, regex=r'...'):
      function_to_test()
# no functional form supported

Initially I liked that proposal, but later I realized that having a special case where suddenly adding a keyword argument breaks the functional form would be surprising to people. Having this inconsistency doesn't seem too appealing to me, and we have other proposals that fit the requirements just as well.

Proposal 1 (pytest.raises_regexp) has the advantages of being similar to unittest's self.assertRaisesRegexp and already having a plugin available which means there are test suites out there already using it. Going forward with this proposal would be just a matter of merging an existing plugin into the core (mostly).

Personally I think 2 (pytest.raises.matching) reads better, and while it might seem weird at first glance, I think people are used to it by now due to mock.patch.object.

So in the end, my vote would go to 1, with 2 coming as a very close second.

@flub
Copy link
Member

flub commented Aug 27, 2015

Since the oldest supported python version is now 2.6 I don't think supporting the functional-style is very important anymore.

Using pytest.raises_regexp() means projects using the existing plugin will probably need changes, so that's not a good plan either.

I'm not sure why pytest.raises(ValueError, func, args, kwargs, regex=r'...') would not work btw? This would be fully backwards compatible, no?

I think out of the existing proposals I probably like the keyword option most, closely followed by pytest.raises.matching().

And just to add to the confusion, I'll add another proposal:

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

(the assert is optional there but I think it should be supported) It essentially just saves you from doing an import but will also provide nicer messages. I do like it because it matches how you catch exceptions normally, in an except statement you can also only ever specify the types you want to catch so it seems normal to keep that signature the same. It is also a way more extensible approach then keeping adding to the signature, but that's a pretty weak argument.

@The-Compiler
Copy link
Member

Using pytest.raises_regexp() means projects using the existing plugin will probably need changes, so that's not a good plan either.

What changes? Wouldn't the plugin just be blacklisted and the functionality works just as before?

I'm not sure why pytest.raises(ValueError, func, args, kwargs, regex=r'...') would not work btw? This would be fully backwards compatible, no?

The problem is that the signature is pytest.raises(ExpectedException, func, *args, **kwargs), so regex would be a part of **kwargs. By the way, this function already has three different signatures:

  • pytest.raises(ExpectedException) (as context manager)
  • pytest.raises(ExpectedException, func, *args, **kwargs)
  • pytest.raises(ExpectedException, "func(*args, **kwargs)")

If we now add a fourth one (but only in the context manager form!), that's going to be... a bad idea.

And just to add to the confusion, I'll add another proposal:

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

(the assert is optional there but I think it should be supported)

I like that! 👍 to both.

@nicoddemus
Copy link
Member

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

Oh my! I love this. 😍

  • No need to introduce another function

  • Supports both with and functional form

  • Fits on how we do matching using excinfo anyway:

    with pytest.raises(Exception) as excinfo:
        function_to_test()
    assert str(excinfo.value) == 'expected msg'
    
    with pytest.raises(Exception) as excinfo:
        function_to_test()
    assert excinfo.match('expected msg')

I see only benefits! To me this proposal wins hands down. 😄

@ionelmc
Copy link
Member

ionelmc commented Aug 27, 2015

I like this one, although it's a bit crufty:

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

This looks a bit better:

with pytest.raises_regex(Exception, r'...') as excinfo:
   function_to_test()
  • pytest.raises_regex better than pytest.raises.matches - people won't expect that a callable would have that attribute. pytest.raises_regex is more obvious.
  • similar to unittest - in theory people would just type regex and IDE autocompletes. Very easy and clear.
  • already used? (the plugin)

@RonnyPfannschmidt
Copy link
Member

lets add a match method to execinfo after pulling py.code into pytest, then we can pretty much pick the external facade afterwards

@HolgerPeters
Copy link
Contributor

I prefer the match method of excinfo. Regular expressions for matching the error message tend to get kind of long and that makes them a bit awkward to use within with .... :. asserting them later on feels more natural.

@RonnyPfannschmidt
Copy link
Member

with the match method inside of the excinfo

implementing all other convenience variants is trivial

@nicoddemus
Copy link
Member

👍 on implementing match in ExceptionInfo. 😄

@nicoddemus
Copy link
Member

Adding match to ExceptionInfo would have to be done after #103 is complete.

@RonnyPfannschmidt
Copy link
Member

fixed in c578226

fkohlgrueber pushed a commit to fkohlgrueber/pytest that referenced this issue Oct 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue easy issue that is friendly to new contributor type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

No branches or pull requests

9 participants