Introduce pytest.not_raises #2339
Conversation
| But this makes us repeat ourselves on the test function. | ||
|
|
||
| For these situations, where you need to ``parametrize`` a test function | ||
| so that sometimes it raises an exception and others do not, you can use |
There was a problem hiding this comment.
"and others do not" -> "and sometimes doesn't"?
There was a problem hiding this comment.
Thanks, that is better! 👍
|
LGTM apart from a sentence which sounds a bit weird 😉 Haven't tried it yet though. |
| @@ -131,6 +131,7 @@ def pytest_namespace(): | |||
| raises.Exception = pytest.fail.Exception | |||
| return { | |||
| 'raises': raises, | |||
| 'not_raises': NotRaisesContext, | |||
There was a problem hiding this comment.
i have a strong opinion on naming it wont_raise in order to give acroos the semantics better
There was a problem hiding this comment.
I like not_raises due to its similarity to raises, but I don't have a strong opinion on it. @The-Compiler do you mind changing to wont_raise?
There was a problem hiding this comment.
I think both sound weird, so no strong opinion here 😆
There was a problem hiding this comment.
oh, an additonal note, why do we make it a callable?
i believe making it a singleton context-manager named after exactly the context its going to encompass is the best we can do,
else we might see people wanting to creep in more specific behavior causing grief down the line
There was a problem hiding this comment.
oh, an additonal note, why do we make it a callable?
It felt more natural, because you can use pytest.raises(...). No problem changing it to a singleton if you feel it is more appropriate.
else we might see people wanting to creep in more specific behavior causing grief down the line
What do you mean? People asking to some weird parameter to it to give additional behavior? Can't we just say no?
There was a problem hiding this comment.
About wont_raise... I'm having some second thoughts that I would like you to consider @RonnyPfannschmidt:
pytest.raisesmaps well toassert raises, and so doespytest.not_raisesthat also maps well toassert not raises.pytest.wont_raisesis a contraction ofwill not, which is not so nice in general;
Hmm on second thought I think we should ask others for their opinion on this. It is important to nail this down right now than regret it later. I'll send an email to the list! 👍
There was a problem hiding this comment.
@nicoddemus wrt feature creep, sometimes it just happens, see the major mess non-strict xfail got us into because we did a too cheap job of handling flaky tests better
we should either clearly limit it, or clearly support passing exception types from the get go
There was a problem hiding this comment.
we should either clearly limit it, or clearly support passing exception types from the get go
OK I don't have a strong objection here.
Also asked in the mailing list if anyone sees a problem in making this a singleton. 👍
| ('1foo bar_', pytest.raises(ValueError)), | ||
| ]) | ||
| def test_check_python_identifier(ident, expectation): | ||
| with pytest.raises(expectation): |
There was a problem hiding this comment.
Shouldn't either the function signature be test_check_python_identifier(ident, valid): or the first argument to parameterize be ident, expectation?
Also, shouldn't the with statement be:
with expectation:
...
There was a problem hiding this comment.
Thanks @wcooley but I'm declining this PR for now.
There was a problem hiding this comment.
@nicoddemus So I noticed after finding & reading over the mailing list threads...
For the sake of posterity for anyone finding this PR and wondering what to do, (I believe but have not actually tried) this can be achieved with a simple no-op context manager:
import contextlib
@contextlib.contextmanager
def noop():
yield
Then:
@pytest.mark.parametrize('ident, expectation', [
('foobar', noop()),
('Foobar1', noop()),
('uh-oh', pytest.raises(Exception))])
def test_whatever(ident, expectation):
with expectation:
if 'oo' not in ident:
raise Exception('No OO')
|
Closing this PR because unfortunately we couldn't reach a consensus on the mailing list about a proper API. 😞 |
Fix #1830