#3290 improve monkeypatch

[feuillemorte]

Fixes #3290.

To handle #2180, we could just import open and other builtins explicitly at the module level of rewrite.py to shield us from users tampering with it.

I added to rewrite.py (not in this PR)

from builtins import open

but it has no effect :(

[coveralls]

Coverage increased (+0.04%) to 92.771% when pulling 283ac8b on feuillemorte:3290-improve-monkeypatch into 372bcdb on pytest-dev:features.

[RonnyPfannschmidt]

this can be constructed as unittest directly taking monkeypatch, no need to invoke a nested acceptance test

[nicoddemus]

Agreed. @feuillemorte please take a look at the other tests in this module for inspiration. 😁

[nicoddemus]

@feuillemorte please rebase on features because this is a new feature.

Thanks for all the hard work!

[nicoddemus]

Please add a docstring here, something along the lines:

"""
Context manager that returns a new :class:`MonkeyPatch` object which
undoes any patching done inside the ``with`` block upon exit:

.. code-block:: python

    import functools
    def test_partial(monkeypatch):
        with monkeypatch.context() as m:
            m.setattr(functools, "partial", 3)

Useful in situations where it is desired to undo some patches before the test ends,
such as mocking ``stdlib`` functions that might break pytest itself if mocked (for examples
of this see `#3290 <https://github.com/pytest-dev/pytest/issues/3290>`_.
"""

[feuillemorte]

Fixed

[nicoddemus]

I suggest here to change this to:

.. note::

    Mind that patching ``stdlib`` functions and some third-party libraries used by pytest might break pytest itself, therefore in those cases it is recommended to use :meth:`MonkeyPatch.context` to limit the patching to the block you want tested:

    .. code-block:: python
        import functools
        def test_partial(monkeypatch):
            with monkeypatch.context() as m:
                m.setattr(functools, "partial", 3)
                assert functools.partial == 3

    See issue `#3290 <https://github.com/pytest-dev/pytest/issues/3290>`_ for details.

[nicoddemus]

Agreed. @feuillemorte please take a look at the other tests in this module for inspiration. 😁

[nicoddemus]

This is a new feature so it should be named 3290.feature instead. Also I suggest using this text:

``monkeypatch`` now supports a ``context()`` function which acts as a context manager which undoes all patching done within the ``with`` block.

The fact that it can be used to avoid blowing up pytest itself is not the highlight of the new feature, just a recommended workaround.

[nicoddemus]

but it has no effect :(

What do you mean?

[feuillemorte]

What do you mean?

I mean that need to fix #2180 problem in this PR.

you wrote:

To handle #2180, we could just import open and other builtins explicitly at the module level of rewrite.py to shield us from users tampering with it.

So, I get a test:

import unittest
import requests
from unittest.mock import patch, mock_open


class PyTestIssue(unittest.TestCase):
    def test_open(self):
        with patch('builtins.open', mock_open(read_data='')):
            requests.get('http://www.google.com')

And try to fix an exception by importing open to rewrite directly:

from builtins import open

And it has no effect, test failed with exception.

[feuillemorte]

rebased to features

[RonnyPfannschmidt]

@feuillemorte if you import/alias a name to a new place, then patching it at the old place has no effect to the new place

[feuillemorte]

Sorry, I didn't get.

def test_open():
    with patch('builtins.open', mock_open(read_data='')): # here we mocking builtin function and go to context manager
        # here we have mocked open function 
        #and pytest also want to call open, but open is mocked 
        #and I have no idea how to split context manager and 
        #pytest functions. open() mocked globally in context 
        #manager and directly importing in pytest is not working here
        requests.get('http://www.google.com') 
        open() # here open() is also mocked and must to behave as mocked
    open() # here is normal behavior

[nicoddemus]

@feuillemorte I now noticed that #2180 definitely can't be fixed, the error occurs inside imp.py and there's nothing pytest can do to prevent that; the discussion in #2180 led to believe that it was a problem that could be solved in rewrite.py, but the error doesn't happen there (the traceback doesn't even contain any pytest related code).

So don't worry about it, I'm closing #2180 as "not fixable".

[feuillemorte]

All comments are fixed

[nicoddemus]

Thanks again @feuillemorte, awesome work! 👍