Parameterize conditional raises #4679

arel · 2019-01-25T00:29:54Z

This PR addresses issues #4324 and #1830, which documents how to use pytest.raises conditionally together with pytest.mark.parametrize.

Also, adopting the suggestion from @RonnyPfannschmidt #1830 (comment) and others, I added a context manager pytest.does_not_raise to make usage more friendly.

Example:

import pytest

@pytest.mark.parametrize('example_input,expectation', [
    (3, pytest.does_not_raise()),
    (2, pytest.does_not_raise()),
    (1, pytest.does_not_raise()),
    (0, pytest.raises(ZeroDivisionError)),
])
def test_division(example_input, expectation):
    '''Test how much I know division.'''
    with expectation:
        assert (6 / example_input) is not None

Thanks! Let me know if I missed anything in the contribution guide.

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

Here's a quick checklist that should be present in PRs (you can delete this text from the final description, this is
just a guideline):

Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
Target the master branch for bug fixes, documentation updates and trivial changes.
Target the features branch for new features and removals/deprecations.
Include documentation when adding new features.
Include new tests or update existing tests when applicable.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

Add yourself to AUTHORS in alphabetical order;

Addressing issues pytest-dev#4324 and pytest-dev#1830

wimglenn · 2019-01-25T02:56:47Z

I'm -1 on putting this new name into pytest namespace. But I'd be +1 on adding the tests and including the simple example to the parametrized docs, modified to use an existing solution such as nullcontext.

codecov · 2019-01-25T05:28:10Z

Codecov Report

Merging #4679 into features will decrease coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           features   #4679      +/-   ##
===========================================
- Coverage      95.7%   95.7%   -0.01%     
===========================================
  Files           111     111              
  Lines         24786   24798      +12     
  Branches       2469    2469              
===========================================
+ Hits          23722   23732      +10     
  Misses          750     750              
- Partials        314     316       +2

Flag	Coverage Δ
#docs	`29.7% <75%> (+0.08%)`	⬆️
#doctesting	`29.7% <75%> (+0.08%)`	⬆️
#linting	`29.7% <75%> (+0.08%)`	⬆️
#linux	`95.53% <100%> (-0.01%)`	⬇️
#nobyte	`92.32% <100%> (ø)`	⬆️
#numpy	`93.14% <100%> (ø)`	⬆️
#pexpect	`42.12% <75%> (ø)`	⬆️
#py27	`93.71% <100%> (ø)`	⬆️
#py34	`91.82% <100%> (+0.06%)`	⬆️
#py35	`91.84% <100%> (+0.06%)`	⬆️
#py36	`91.86% <100%> (+0.06%)`	⬆️
#py37	`93.87% <100%> (-0.01%)`	⬇️
#trial	`93.14% <100%> (ø)`	⬆️
#windows	`93.88% <100%> (ø)`	⬆️
#xdist	`93.73% <100%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
src/pytest.py	`100% <100%> (ø)`	⬆️
testing/python/raises.py	`94.59% <100%> (+0.3%)`	⬆️
src/_pytest/python_api.py	`97.52% <100%> (+0.03%)`	⬆️
src/_pytest/cacheprovider.py	`95.75% <0%> (-0.95%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9905a73...b29f40a. Read the comment docs.

codecov · 2019-01-25T05:29:02Z

Codecov Report

Merging #4679 into features will decrease coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           features   #4679      +/-   ##
===========================================
- Coverage      95.7%   95.7%   -0.01%     
===========================================
  Files           111     111              
  Lines         24786   24798      +12     
  Branches       2469    2469              
===========================================
+ Hits          23722   23732      +10     
  Misses          750     750              
- Partials        314     316       +2

Flag	Coverage Δ
#docs	`29.7% <75%> (+0.08%)`	⬆️
#doctesting	`29.7% <75%> (+0.08%)`	⬆️
#linting	`29.7% <75%> (+0.08%)`	⬆️
#linux	`95.53% <100%> (-0.01%)`	⬇️
#nobyte	`92.32% <100%> (ø)`	⬆️
#numpy	`93.14% <100%> (ø)`	⬆️
#pexpect	`42.12% <75%> (ø)`	⬆️
#py27	`93.71% <100%> (ø)`	⬆️
#py34	`91.82% <100%> (+0.06%)`	⬆️
#py35	`91.84% <100%> (+0.06%)`	⬆️
#py36	`91.86% <100%> (+0.06%)`	⬆️
#py37	`93.87% <100%> (-0.01%)`	⬇️
#trial	`93.14% <100%> (ø)`	⬆️
#windows	`93.88% <100%> (ø)`	⬆️
#xdist	`93.73% <100%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
src/pytest.py	`100% <100%> (ø)`	⬆️
testing/python/raises.py	`94.59% <100%> (+0.3%)`	⬆️
src/_pytest/python_api.py	`97.52% <100%> (+0.03%)`	⬆️
src/_pytest/cacheprovider.py	`95.75% <0%> (-0.95%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9905a73...b29f40a. Read the comment docs.

codecov · 2019-01-25T05:29:45Z

Codecov Report

Merging #4679 into features will decrease coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           features   #4679      +/-   ##
===========================================
- Coverage      95.7%   95.7%   -0.01%     
===========================================
  Files           111     111              
  Lines         24786   24798      +12     
  Branches       2469    2469              
===========================================
+ Hits          23722   23732      +10     
  Misses          750     750              
- Partials        314     316       +2

Flag	Coverage Δ
#docs	`29.7% <75%> (+0.08%)`	⬆️
#doctesting	`29.7% <75%> (+0.08%)`	⬆️
#linting	`29.7% <75%> (+0.08%)`	⬆️
#linux	`95.53% <100%> (-0.01%)`	⬇️
#nobyte	`92.32% <100%> (ø)`	⬆️
#numpy	`93.14% <100%> (ø)`	⬆️
#pexpect	`42.12% <75%> (ø)`	⬆️
#py27	`93.71% <100%> (ø)`	⬆️
#py34	`91.82% <100%> (+0.06%)`	⬆️
#py35	`91.84% <100%> (+0.06%)`	⬆️
#py36	`91.86% <100%> (+0.06%)`	⬆️
#py37	`93.87% <100%> (-0.01%)`	⬇️
#trial	`93.14% <100%> (ø)`	⬆️
#windows	`93.88% <100%> (ø)`	⬆️
#xdist	`93.73% <100%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
src/pytest.py	`100% <100%> (ø)`	⬆️
testing/python/raises.py	`94.59% <100%> (+0.3%)`	⬆️
src/_pytest/python_api.py	`97.52% <100%> (+0.03%)`	⬆️
src/_pytest/cacheprovider.py	`95.75% <0%> (-0.95%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9905a73...77dcac8. Read the comment docs.

arel · 2019-01-25T14:06:51Z

Thanks, @wimglenn! The argument for including a null context manager (does_not_raise) in the pytest namespace is that:

nullcontext was not introduced until Python 3.7, and other alternatives (such as suppress) were not added until Python 3.5. The alternatives are to either:
- install a separate library (contextlib2)
- write your own null context manager
- repeatedly add if- statements to tests to handle the special case where some tests raise and others do not. All of these add extra friction to a valid use case of pytest.raises.
does_not_raise depends only on contextlib.contextmanager added in Python 2.5.
Being that pytest.raises is in the pytest namespace, having its complement (does_not_raise) in the same place seems reasonable. If you recall, @The-Compiler's original idea in RFC: parametrizing conditional raising with pytest.raises #1830 was to update raises to support raises(None), but @RonnyPfannschmidt made a good point about loosening the contract of that function, and he suggested adding a null context magager instead (wont_raise).

I believe it is worthwhile to be able to conditionally parameterize pytest.raises one way or another without needing to install additional packages or writing boilerplate code. I hope you'll reconsider.

nicoddemus · 2019-01-25T14:46:36Z

I was leaning against having this in pytest as well, but we do have to balance the cost of maintainability vs usability.

From the maintainability point of view this is really trivial to implement, and may be eventually become an alias to contextmanager.nullcontext when support for Python < 3.7 is dropped.

For users which need the functionality, having it available in pytest is definitely a good thing. As much as it is simple to implement this yourself, is still nice to have it ready for use.

The last point against adding this was the potential for misuse. The documentation should be very clear that the only use case for using does_not_raise is for parametrization.

So all in all I'm OK with this getting into the core.

nicoddemus · 2019-01-25T17:34:32Z

Btw many thanks @arel for your work on this. 👍

RonnyPfannschmidt · 2019-01-25T18:51:35Z

i consider this a great add - but i also want to ensure we document the intent and the limits correctly

RonnyPfannschmidt

this looks good, a small reference in the docs for raises and the note on the excinfo usage should turn this into something we can safely merge

good work :)

RonnyPfannschmidt · 2019-01-25T18:54:05Z

src/_pytest/python_api.py

+        def test_division(example_input, expectation):
+            '''Test how much I know division.'''
+            with expectation:
+                assert (6 / example_input) is not None


we should have a warning here for the usage like

with expectation as excinfo: .... ``` - that excinfo will be None

Thanks! I updated the sphinx documentation in 77dcac8 regarding execinfo and also added a reference in pytest.raises.

wimglenn · 2019-01-25T19:05:08Z

I'm aware of the use-cases, but still firmly against - it's adding a name that will be practically obsoleted by the end of this year. Beyond 2020+ that's just going to be another name hanging around which is a different way of doing the same thing. dir(pytest) is currently refreshingly clean and clear of cruft, can be nicer to keep it that way..

By the way, just because I'm curious - what is the import _pytest._code needed in the tests for?

arel · 2019-01-26T03:21:27Z

Thanks. The import _pytest._code lines were unnecessary; I removed them.

asottile · 2019-01-27T03:05:05Z

changelog/1830.feature.rst

@@ -0,0 +1 @@
+A context manager ``does_not_raise`` is added to complement ``raises`` in parametrized tests with conditional raises.


I thought we agreed that we were not going to add this? It encourages tests which are of bad form

#1830 (comment)

#4324 was a followup to #1830 that we document how to implement does_not_raises if you absolutely need it, not that we actually implement it.

I was and still am 👎 on this.

nicoddemus · 2019-01-27T03:24:13Z

Given that @asottile and @wimglenn are 👎 on this, I respect their opinion so I agree we should reject this proposal.

@arel we are really grateful for your work on this, but would you consider another PR updating the docs with an example on how to use nullcontext instead? Thanks a lot!

arel · 2019-01-27T06:37:36Z

@nicoddemus, before you let this PR get bike-shed completely, keep in mind the following things:

It's been three years that people have been waiting for this tiny little feature and have had to figure out ways to work around the limitation of raises() themselves
We're talking about 3 lines of code
The recipe @asottile gives for backporting nullcontext requires installing a separate module (contextlib2)
The "abuse" people fear of a null context manager is simply redunancy with negligable effect.
Just because Python 2 will not be developed past 2020 doesn't mean that adding does_not_raise will be obsolete or unhelpful, even if it's an alias for nullcontext.
While you could ask people to refactor all their tests so they all either raise or don't raise, that's not helpful when it would mean rewriting or duplicating code that could otherwise be avoided.
The PR is ready to go, and you could make pytest just a little bit more inclusive with one click of a button.

wimglenn · 2019-01-27T07:28:09Z

That is not a limitation in pytest.raises though: it's precisely a context-manager to assert that an indented block should raise. The proposal pytest.raises(None) is scope creep.

The problem with adding API surface area for a pytest.does_not_raise context manager is, in order to be a proper complement to pytest.raises, it would also be obliged^* to handle the case where indented block does raise (and produce a human-readable error message saying how this block raised something when it was not expected to do so).

*_{I'm not suggesting to implement such features in this PR. Python 2 users having to install a contextlib backport to use the recipe from the docs is no big deal. #4324 is the right place to continue any discussion.}

RonnyPfannschmidt · 2019-01-27T11:44:41Z

personally i'm quite sad that we don't have this as utility for symmetry and better comprehension of test specifications

the enhancement idea for adding a more comprehensible error descriptions would be a great add-on
personally i would have preferred adding the name now, and expanding the error indication from pass trough to argument later

nicoddemus · 2019-01-27T13:30:33Z

@arel I kind of lean towards your points, but given that we have strong 👎 from other maintainers, I feel we still have to reject the proposal. Sorry.

arel added 3 commits January 24, 2019 23:06

Adds does_not_raise context manager

286a7b3

Addressing issues pytest-dev#4324 and pytest-dev#1830

Documenting raises/does_not_raise + parametrize

3d62fd0

Adding changelog entries for does_not_raise

b29f40a

RonnyPfannschmidt reviewed Jan 25, 2019

View reviewed changes

Improving sphinx docs based on feedback

77dcac8

asottile requested changes Jan 27, 2019

View reviewed changes

nicoddemus closed this Jan 27, 2019

arel mentioned this pull request Jan 27, 2019

Document parametrizing conditional raises #4682

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameterize conditional raises #4679

Parameterize conditional raises #4679

arel commented Jan 25, 2019

wimglenn commented Jan 25, 2019 •

edited

Loading

codecov bot commented Jan 25, 2019

codecov bot commented Jan 25, 2019

codecov bot commented Jan 25, 2019 •

edited

Loading

arel commented Jan 25, 2019

nicoddemus commented Jan 25, 2019

nicoddemus commented Jan 25, 2019

RonnyPfannschmidt commented Jan 25, 2019

RonnyPfannschmidt left a comment

RonnyPfannschmidt Jan 25, 2019

arel Jan 25, 2019

wimglenn commented Jan 25, 2019

arel commented Jan 26, 2019

asottile Jan 27, 2019

nicoddemus commented Jan 27, 2019

arel commented Jan 27, 2019

wimglenn commented Jan 27, 2019 •

edited

Loading

RonnyPfannschmidt commented Jan 27, 2019

nicoddemus commented Jan 27, 2019

		@@ -0,0 +1 @@
		A context manager ``does_not_raise`` is added to complement ``raises`` in parametrized tests with conditional raises.

Parameterize conditional raises #4679

Parameterize conditional raises #4679

Conversation

arel commented Jan 25, 2019

wimglenn commented Jan 25, 2019 • edited Loading

codecov bot commented Jan 25, 2019

Codecov Report

codecov bot commented Jan 25, 2019

Codecov Report

codecov bot commented Jan 25, 2019 • edited Loading

Codecov Report

arel commented Jan 25, 2019

nicoddemus commented Jan 25, 2019

nicoddemus commented Jan 25, 2019

RonnyPfannschmidt commented Jan 25, 2019

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

RonnyPfannschmidt Jan 25, 2019

Choose a reason for hiding this comment

arel Jan 25, 2019

Choose a reason for hiding this comment

wimglenn commented Jan 25, 2019

arel commented Jan 26, 2019

asottile Jan 27, 2019

Choose a reason for hiding this comment

nicoddemus commented Jan 27, 2019

arel commented Jan 27, 2019

wimglenn commented Jan 27, 2019 • edited Loading

RonnyPfannschmidt commented Jan 27, 2019

nicoddemus commented Jan 27, 2019

wimglenn commented Jan 25, 2019 •

edited

Loading

codecov bot commented Jan 25, 2019 •

edited

Loading

wimglenn commented Jan 27, 2019 •

edited

Loading