Codecov Report

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

@@            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

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 Report

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

@@            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

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.

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

@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

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

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.

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"

@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!

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.

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

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

Oops I forgot

Thanks for your contribution!

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

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 👍

it's entirely docs / tests

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