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

Comments

Projects
None yet
9 participants
@pytestbot

pytestbot commented Oct 19, 2013

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

This comment has been minimized.

pytestbot commented Oct 19, 2013

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

This comment has been minimized.

pytestbot commented Oct 21, 2013

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

This comment has been minimized.

pytestbot commented Sep 11, 2014

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

This comment has been minimized.

pytestbot commented Sep 11, 2014

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

This comment has been minimized.

pytestbot commented Sep 11, 2014

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

This comment has been minimized.

pytestbot commented Sep 16, 2014

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

This comment has been minimized.

pytestbot commented Sep 16, 2014

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 :)

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Jul 25, 2015

proposal;

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

This comment has been minimized.

Member

RonnyPfannschmidt commented Jul 25, 2015

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

@foxx

This comment has been minimized.

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

This comment has been minimized.

Member

The-Compiler commented Aug 26, 2015

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

This comment has been minimized.

Member

nicoddemus commented Aug 27, 2015

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

This comment has been minimized.

Member

nicoddemus commented Aug 27, 2015

(Just posted a tweet about this).

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Aug 27, 2015

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

This comment has been minimized.

Contributor

sscherfke commented Aug 27, 2015

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

This comment has been minimized.

Member

nicoddemus commented Aug 27, 2015

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

This comment has been minimized.

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

This comment has been minimized.

Member

The-Compiler commented Aug 27, 2015

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

This comment has been minimized.

Member

nicoddemus commented Aug 27, 2015

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

This comment has been minimized.

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

This comment has been minimized.

Member

RonnyPfannschmidt commented Aug 27, 2015

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

@HolgerPeters

This comment has been minimized.

Contributor

HolgerPeters commented Aug 27, 2015

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

This comment has been minimized.

Member

RonnyPfannschmidt commented Aug 27, 2015

with the match method inside of the excinfo

implementing all other convenience variants is trivial

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Aug 27, 2015

👍 on implementing match in ExceptionInfo. 😄

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Aug 27, 2015

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

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Apr 20, 2016

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