Skip to content

Document parametrizing conditional raises #4682

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

Merged

Conversation

arel
Copy link
Contributor

@arel arel commented Jan 27, 2019

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

I added documentation on how to use a null context manager to complement pytest.raises as well as added tests for the documented code.

Based on the discussion in #4679, I removed does_not_raise from the core library, and instead will rely on users copying and pasting the boilerplate.

I used:

@contextmanager
def does_not_raise():
    yield

instead of

from contextlib import nullcontext as does_not_raise

because it works from Python 2.5 and up without installing additional libraries or backporting, whereas nullcontext is only introduced in Python 3.7.

Closes #4324


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;

@codecov
Copy link

codecov bot commented Jan 27, 2019

Codecov Report

Merging #4682 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4682      +/-   ##
=========================================
+ Coverage   95.69%   95.7%   +<.01%     
=========================================
  Files         113     113              
  Lines       24902   24910       +8     
  Branches     2459    2459              
=========================================
+ Hits        23830   23840      +10     
+ Misses        757     755       -2     
  Partials      315     315
Flag Coverage Δ
#docs 29.57% <ø> (+0.07%) ⬆️
#doctesting 29.57% <ø> (+0.07%) ⬆️
#linting 29.57% <ø> (+0.07%) ⬆️
#linux 95.53% <100%> (ø) ⬆️
#nobyte 92.32% <100%> (ø) ⬆️
#numpy 93.13% <100%> (+0.03%) ⬆️
#pexpect 42.08% <ø> (+0.02%) ⬆️
#py27 93.7% <100%> (+0.02%) ⬆️
#py34 91.82% <100%> (+0.06%) ⬆️
#py35 91.84% <100%> (+0.06%) ⬆️
#py36 91.87% <100%> (+0.06%) ⬆️
#py37 93.85% <100%> (+0.01%) ⬆️
#trial 93.13% <100%> (+0.03%) ⬆️
#windows 93.9% <100%> (ø) ⬆️
#xdist 93.74% <100%> (-0.02%) ⬇️
Impacted Files Coverage Δ
src/_pytest/python_api.py 97.48% <ø> (ø) ⬆️
testing/python/raises.py 94.59% <100%> (+0.3%) ⬆️
src/_pytest/cacheprovider.py 95.75% <0%> (-1.42%) ⬇️
src/_pytest/terminal.py 91.36% <0%> (+0.83%) ⬆️

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 16f8cda...fd4289d. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 27, 2019

Codecov Report

Merging #4682 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4682      +/-   ##
==========================================
- Coverage   95.69%   95.68%   -0.02%     
==========================================
  Files         113      113              
  Lines       24902    24970      +68     
  Branches     2459     2479      +20     
==========================================
+ Hits        23830    23892      +62     
- Misses        757      762       +5     
- Partials      315      316       +1
Flag Coverage Δ
#docs 29.66% <ø> (+0.17%) ⬆️
#doctesting 29.66% <ø> (+0.17%) ⬆️
#linting 29.66% <ø> (+0.17%) ⬆️
#linux 95.5% <100%> (-0.03%) ⬇️
#nobyte 92.29% <100%> (-0.03%) ⬇️
#numpy 93.11% <100%> (ø) ⬆️
#pexpect 42.09% <ø> (+0.02%) ⬆️
#py27 93.69% <100%> (ø) ⬆️
#py34 91.76% <100%> (ø) ⬆️
#py35 91.76% <100%> (-0.01%) ⬇️
#py36 91.78% <100%> (-0.02%) ⬇️
#py37 93.8% <100%> (-0.06%) ⬇️
#trial 93.11% <100%> (ø) ⬆️
#windows 93.87% <100%> (-0.04%) ⬇️
#xdist 93.71% <100%> (-0.06%) ⬇️
Impacted Files Coverage Δ
src/_pytest/python_api.py 97.48% <ø> (ø) ⬆️
testing/python/raises.py 94.59% <100%> (+0.3%) ⬆️
src/_pytest/resultlog.py 87.14% <0%> (-2.86%) ⬇️
src/_pytest/debugging.py 80.79% <0%> (-0.97%) ⬇️
src/_pytest/python.py 92.42% <0%> (-0.59%) ⬇️
src/_pytest/reports.py 97.46% <0%> (-0.21%) ⬇️
testing/test_junitxml.py 97.8% <0%> (-0.16%) ⬇️
src/_pytest/unittest.py 94.17% <0%> (-0.11%) ⬇️
src/_pytest/_code/code.py 95.54% <0%> (-0.02%) ⬇️
src/_pytest/fixtures.py 97.91% <0%> (-0.01%) ⬇️
... and 19 more

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 16f8cda...7ec1a14. Read the comment docs.

@asottile
Copy link
Member

I think the recipe in #1830 (comment) is more complete. The python2 block can be implemented in the same way as this (if needed)

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a ton @arel!

@arel
Copy link
Contributor Author

arel commented Jan 28, 2019

@asottile I don't quite follow. The recipe you linked to is more verbose and requires backporting contextlib and only supports Python 2.6 and above:

if sys.version_info >= (3, 7):
    from contextlib import nullcontext
elif sys.version_info >= (3, 3):
    from contextlib import ExitStack as nullcontext
else:
    from contextlib2 import ExitStack as nullcontext

whereas, the recipe below uses only built-in features and supports Python 2.5 and above:

from contextlib import contextmanager

@contextmanager
def does_not_raise():
    yield

@asottile
Copy link
Member

@arel pytest doesn't support 2.6 and the recipe improves ootb support for other versions

@nicoddemus
Copy link
Member

IMHO the current example is good enough. We could also post @asottile's solution beside the current one, but I feel this would add complexity to what was supposed to be a simple example... but if @asottile feels strongly about it, let's add it alongside it then.

@asottile
Copy link
Member

asottile commented Jan 29, 2019

I don't even think you need the recipe, but the important information is basically (something like this):

If you need to parametrize based on "raising" you can implement a ``does_not_raise`` contextmanager using a noop contextmanager.

If you're only supporting python3.7+ you can use:

.. code-block:: python

    from contextlib import nullcontext as does_not_raise

If you're only supporting python3.3+ you can use:

.. code-block:: python

    from contextlib import ExitStack as does_not_raise

If you need to support older versions you can either use the `contextlib2` backport:

.. code-block:: python

    from contextlib2 import ExitStack as does_not_raise

or you can define your own noop contextmanager

.. code-block:: python

    from contextlib import contextmanager

    @contextmanager
    def does_not_raise():
        yield

I might even go so far as to put some opinions in there as well. Something like "You might be over-engineering your tests with parametrize a bit -- it will likely read better if you split your testcases into successful cases and failure cases instead of trying to bake conditional logic into parametrize"

@nicoddemus
Copy link
Member

@asottile perhaps you should mark this PR as "request changes" so it is clear in the Pull Requests view that this PR still has required work? Thanks!

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(see above)

@arel
Copy link
Contributor Author

arel commented Jan 30, 2019

I feel strongly that the example as-is is clearer than the alternative, and I fail to see a benefit in complicating it. I don't believe there is a correctness benefit, performance benefit, or pedagogical benefit in using ExitStack or nullcontext, and we already mention nullcontext as an alternative in the docs.

Maybe I wasn't clear, but the contextmanager approach works not only in Python 2.5+ but also Python 3.0–3.7+, and it also works out of the box.

The main point someone needs to get from the documentation is: in order to parametrize raises(), you should use a null context manager. Any null context manager will do.

@asottile
Copy link
Member

Correct, but it's better to use the builtin things where you have to write zero code. pytest doesn't care about python 2.5 / 2.6 / 3.0 / 3.1 / 3.2 / 3.3 support and soon won't care about python2.x support and so documenting a pattern in a way that's soon to become obsolete should be the last goal. Put the newest things first, and the "ok well if you absolutely must support old stuff use this" last please :)

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close enough -- thanks!

@arel
Copy link
Contributor Author

arel commented Feb 2, 2019

Great! I don't have write access, so I'll leave merging up to one of you. Thanks!

@asottile asottile merged commit 2264db7 into pytest-dev:master Feb 2, 2019
@asottile
Copy link
Member

asottile commented Feb 2, 2019

Oops I forgot

@blueyed
Copy link
Contributor

blueyed commented Feb 3, 2019

Thanks for your contribution!

I just wondered if / why this wasn't done for the features branch?

@asottile
Copy link
Member

asottile commented Feb 3, 2019

Thanks for your contribution!

I just wondered if / why this wasn't done for the features branch?

it's entirely docs / tests -- it'll make it into features when we merge master into features 👍

@blueyed
Copy link
Contributor

blueyed commented Feb 3, 2019

it's entirely docs / tests

Oh, missed that afe9fd5 was "reverted"/removed again (when looking at new commits on master).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants