Pytest: Create a safeguard module to import stdlib objects from

[nicoddemus]

The rationale here being that users sometimes monkeypatch functions and objects from the standard library which can break pytest itself.

@RonnyPfannschmidt's idea from https://github.com/pytest-dev/pytest/issues/3288#issuecomment-371188406.

Refs: #3288, #2180, #3352

[nicoddemus]

To elaborate, the idea is that in normal pytest modules we never import from stdlib or external modules directly, but from a safeimp.py instead which imports all required symbols when pytest starts.

The issue is that sometimes people will monkey patch some stdlib module or builtin function, and this breaks pytest internals.

For example some pytest module (say python.py) might have this import:

import os

    # down the module...
    os.environ['PYTEST_CURRENT_TEST'] = node.id

Now if a user during a test does something like this:

import os
def test(monkeypatch):
    monkeypatch.setattr(os, 'environ', 1)

It will break pytest internally because os.environ will be 1 instead of a dict, which will break the code in python.py.

That is the problem, the solution @RonnyPfannschmidt suggests is to import all symbols we require early in a safeimp.py module:

# safeimp.py
from os import environ

So the code in python.py is changed to:

from .safeimp import environ

    # down the module...
    environ['PYTEST_CURRENT_TEST'] = node.id

So we get the original os.environ even if the user messes with it during the tests.

You might start with looking at all imports from the stdlib (like os.environ above) and move them to this new safeimp.py file. And don't worry if we miss some, it is then a matter of updating it with new symbols in the future, the important thing is to have this mechanism in-place.

[brianmaissy]

Wouldn't this damage the readability of the code, because we would always have to refer to stdlib objects without their module?

[nicoddemus]

@brianmaissy Definitely it would, and it would be a hassle to enforce that without a linter. 😬

[brianmaissy]

Then I wonder if it's worth the bother. How common is the use case of someone monkeypatching the standard library so badly that it doesn't behave like the standard library anymore? I sort feel like you deserve what you get if you make os.environ something that doesn't behave reasonably like a mapping.

[feuillemorte]

@nicoddemus

Seems, it doesn't help or I doing smth wrong :(

test_1.py:

import functools

from unittest.mock import Mock


def test_partial(monkeypatch):
    monkeypatch.setattr(functools, "partial", Mock())
    assert functools.partial.call_count == 1

safeguarded_function.py:

import functools

compat.py:

from _pytest import safeguarded_function as sf
......
if isinstance(obj, sf.functools.partial):
....

output:

platform linux -- Python 3.6.4, pytest-3.5.1.dev26+gad0b4330.d20180404, py-1.5.3, pluggy-0.6.0
rootdir: /home/feuillemorte/github/pytest-dev/pytest/pytest_3290, inifile: tox.ini
collected 1 item                                                                                                                                                                       

tests/test_1.py 
INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/main.py", line 107, in wrap_session
INTERNALERROR>     session.exitstatus = doit(config, session) or 0
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/main.py", line 145, in _main
INTERNALERROR>     config.hook.pytest_runtestloop(session=session)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/__init__.py", line 617, in __call__
INTERNALERROR>     return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/__init__.py", line 222, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/__init__.py", line 216, in <lambda>
INTERNALERROR>     firstresult=hook.spec_opts.get('firstresult'),
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/callers.py", line 201, in _multicall
INTERNALERROR>     return outcome.get_result()
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/callers.py", line 76, in get_result
INTERNALERROR>     raise ex[1].with_traceback(ex[2])
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/callers.py", line 180, in _multicall
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/main.py", line 168, in pytest_runtestloop
INTERNALERROR>     item.config.hook.pytest_runtest_protocol(item=item, nextitem=nextitem)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/__init__.py", line 617, in __call__
INTERNALERROR>     return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/__init__.py", line 222, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/__init__.py", line 216, in <lambda>
INTERNALERROR>     firstresult=hook.spec_opts.get('firstresult'),
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/callers.py", line 201, in _multicall
INTERNALERROR>     return outcome.get_result()
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/callers.py", line 76, in get_result
INTERNALERROR>     raise ex[1].with_traceback(ex[2])
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/callers.py", line 180, in _multicall
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/runner.py", line 62, in pytest_runtest_protocol
INTERNALERROR>     runtestprotocol(item, nextitem=nextitem)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/runner.py", line 79, in runtestprotocol
INTERNALERROR>     reports.append(call_and_report(item, "call", log))
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/runner.py", line 160, in call_and_report
INTERNALERROR>     report = hook.pytest_runtest_makereport(item=item, call=call)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/__init__.py", line 617, in __call__
INTERNALERROR>     return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/__init__.py", line 222, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/__init__.py", line 216, in <lambda>
INTERNALERROR>     firstresult=hook.spec_opts.get('firstresult'),
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/callers.py", line 196, in _multicall
INTERNALERROR>     gen.send(outcome)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/skipping.py", line 117, in pytest_runtest_makereport
INTERNALERROR>     rep = outcome.get_result()
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/callers.py", line 76, in get_result
INTERNALERROR>     raise ex[1].with_traceback(ex[2])
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pluggy-0.6.0-py3.6.egg/pluggy/callers.py", line 180, in _multicall
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/runner.py", line 312, in pytest_runtest_makereport
INTERNALERROR>     longrepr = item.repr_failure(excinfo)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/python.py", line 598, in repr_failure
INTERNALERROR>     return self._repr_failure_py(excinfo, style=style)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/python.py", line 591, in _repr_failure_py
INTERNALERROR>     style=style)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/nodes.py", line 229, in _repr_failure_py
INTERNALERROR>     self._prunetraceback(excinfo)
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/python.py", line 567, in _prunetraceback
INTERNALERROR>     code = _pytest._code.Code(get_real_func(self.obj))
INTERNALERROR>   File "/home/feuillemorte/github/pytest-dev/pytest/pytest_3290/env/lib/python3.6/site-packages/pytest-3.5.1.dev26+gad0b4330.d20180404-py3.6.egg/_pytest/compat.py", line 214, in get_real_func
INTERNALERROR>     if isinstance(obj, sf.functools.partial):
INTERNALERROR> TypeError: isinstance() arg 2 must be a type or tuple of types

[brianmaissy]

@feuillemorte You need to import partial itself in safeguarded_function, not justfunctools. You are monkeypatching the same module which you have already imported.

[feuillemorte]

@brianmaissy I see, thank you!

[thisch]

Note that this safeimpl approach fails when tests mock methods/attributes of classes from the stdlibrary!

[nicoddemus]

@thisch that's true, thanks for pointing it out. If anybody has any other suggestions they are welcome though, the outline above is what I got from @RonnyPfannschmidt's suggestion.

[RonnyPfannschmidt]

we can definitively pick a service level for patching the stdlib tho

• replace module level objects and you will be fine, go for the internals aand you are on your own

[nicoddemus]

@brianmaissy

Then I wonder if it's worth the bother. How common is the use case of someone monkeypatching the standard library so badly that it doesn't behave like the standard library anymore? I sort feel like you deserve what you get if you make os.environ something that doesn't behave reasonably like a mapping.

You got a point and I kinda agree with you. The problem is that people sometimes do it (for legitimate testing reasons), and they waste a lot of time trying to figure out what happened. If we can save those users time by implementing some safe-guard, I think it is worth doing it. My 2 cents.

[feuillemorte]

What if I import it like

from functools import partial as functools_partial
from inspect import isclass as inspect_isclass, CO_VARARGS as inspect_CO_VARARGS, \
    CO_VARKEYWORDS as inspect_CO_VARKEYWORDS
from sys import exc_info as sys_exc_info

then this will easier to understand what is it:

from _pytest.safeguarded_function import functools_partial
.....
    if isinstance(obj, functools_partial):
        obj = obj.func
    return obj

Because it's really hard to understand what I import:

from _pytest.safeguarded_function import isclass, CO_VARARGS, CO_VARKEYWORDS, exc_info, ref, ib, Factory, s

Also, file safeguarded_function.py maybe huge:

from functools import \
    partial as functools_partial
from inspect import \
    isclass as inspect_isclass,\
    CO_VARARGS as inspect_CO_VARARGS,\
    CO_VARKEYWORDS as inspect_CO_VARKEYWORDS
from sys import \
    exc_info as sys_exc_info
from attr import \
    ib as attr_ib, \
    s as attr_s, \
    Factory as attr_Factory
from weakref import \
    ref as weakref_ref
from re import \
    search as re_search

[nicoddemus]

Looking at the implementation so far, I'm starting to have second thoughts about this myself because it seems we will be adding a lot of overhead to our development.

Let's backtrack a little, to recap:

3288: monkeypatch.setattr(functools, "partial", Mock())

2180: with patch('builtins.open', mock_open(read_data='')):

3352: monkeypatch.delattr(os, 'environ')

Except for #2180, the others could be fixed by using with mock.patch instead, because the mock would be undone after the with block.

If the above is true, we could instead improve monkeypatch to support some form of with statement, which would be an alternative for #3288 and #3352. 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.

Something like:

with monkeypatch.context() as m:
    m.setattr(functools, "partial", Mock())

(context is not a good name I think, just using it to illustrate the point).

[RonnyPfannschmidt]

i agree - lets document that monkeypatching the stdlib without care can and will break pytest

[nicoddemus]

Sounds good, and what do you think about the suggestions I made above?

[RonnyPfannschmidt]

the "i agree" was targetted at those, its a good starting point and i believe with a few iterations we will arrive at something fabulous

[feuillemorte]

class MonkeyPatch(object):
    def __enter__(self):
        print('HEY OPEN!')

    def __exit__(self, exc_type, exc_val, exc_tb):
        print('HEY CLOSE!')

def test_partial(monkeypatch):
    with monkeypatch as m:
        m.setattr(functools, "partial", Mock())
        assert functools.partial.call_count == 1



md5-0d5f9f1888d6b22c7a3737256c531a40



--------------------------------------------------------------------------------- Captured stdout call ---------------------------------------------------------------------------------
HEY OPEN!
HEY CLOSE!

But how to create another namespace for with statement? I don't understand how i can save functools in this case

[RonnyPfannschmidt]

the new namespace is the return value - the monkeypatcher nesting should be done as a separate feature tool and should be used for the capture plugin as well

[feuillemorte]

Merged. So, I'm closing the task