From 011f88f7e77b8b2e6de01d2c5588dda47c77093b Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sun, 22 Jul 2018 11:10:44 -0300 Subject: [PATCH 1/3] Deprecate calling fixture functions directly This will now issue a RemovedInPytest4Warning when the user calls a fixture function directly, instead of requesting it from test functions as is expected Fix #3661 --- changelog/3661.removal.rst | 3 ++ src/_pytest/compat.py | 1 + src/_pytest/deprecated.py | 6 +++ src/_pytest/fixtures.py | 52 ++++++++++++++++++- src/_pytest/pytester.py | 5 ++ testing/deprecated_test.py | 14 +++++ .../example_scripts/tmpdir/tmpdir_fixture.py | 7 +++ testing/python/fixture.py | 9 ++-- testing/test_assertion.py | 13 +++-- testing/test_conftest.py | 4 +- testing/test_tmpdir.py | 31 ++--------- 11 files changed, 103 insertions(+), 42 deletions(-) create mode 100644 changelog/3661.removal.rst create mode 100644 testing/example_scripts/tmpdir/tmpdir_fixture.py diff --git a/changelog/3661.removal.rst b/changelog/3661.removal.rst new file mode 100644 index 00000000000..baa30fccf6b --- /dev/null +++ b/changelog/3661.removal.rst @@ -0,0 +1,3 @@ +Calling a fixture function directly, as opposed to request them in a test function, now issues a ``RemovedInPytest4Warning``. It will be changed into an error in pytest ``4.0``. + +This is a great source of confusion to new users, which will often call the fixture functions and request them from test functions interchangeably, which breaks the fixture resolution model. diff --git a/src/_pytest/compat.py b/src/_pytest/compat.py index 3a29c688ebb..a1cd3bd4ed4 100644 --- a/src/_pytest/compat.py +++ b/src/_pytest/compat.py @@ -84,6 +84,7 @@ def iscoroutinefunction(func): def getlocation(function, curdir): + function = get_real_func(function) fn = py.path.local(inspect.getfile(function)) lineno = function.__code__.co_firstlineno if fn.relto(curdir): diff --git a/src/_pytest/deprecated.py b/src/_pytest/deprecated.py index b3c40c2ae45..d32b675ae20 100644 --- a/src/_pytest/deprecated.py +++ b/src/_pytest/deprecated.py @@ -22,6 +22,12 @@ class RemovedInPytest4Warning(DeprecationWarning): "Please remove the prefix and use the @pytest.fixture decorator instead." ) +FIXTURE_FUNCTION_CALL = ( + "Fixture {name} called directly. Fixtures are not meant to be called directly, " + "are created automatically when test functions request them as parameters. " + "See https://docs.pytest.org/en/latest/fixture.html for more information." +) + CFG_PYTEST_SECTION = ( "[pytest] section in {filename} files is deprecated, use [tool:pytest] instead." ) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 207bf27e432..93557fa04eb 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -6,6 +6,8 @@ import sys import warnings from collections import OrderedDict, deque, defaultdict + +import six from more_itertools import flatten import attr @@ -29,6 +31,7 @@ safe_getattr, FuncargnamesCompatAttr, ) +from _pytest.deprecated import FIXTURE_FUNCTION_CALL, RemovedInPytest4Warning from _pytest.outcomes import fail, TEST_OUTCOME FIXTURE_MSG = 'fixtures cannot have "pytest_funcarg__" prefix and be decorated with @pytest.fixture:\n{}' @@ -798,7 +801,7 @@ def call_fixture_func(fixturefunc, request, kwargs): def _teardown_yield_fixture(fixturefunc, it): """Executes the teardown of a fixture function by advancing the iterator after the - yield and ensure the iteration ends (if not it means there is more than one yield in the function""" + yield and ensure the iteration ends (if not it means there is more than one yield in the function)""" try: next(it) except StopIteration: @@ -928,6 +931,13 @@ def pytest_fixture_setup(fixturedef, request): request._check_scope(argname, request.scope, fixdef.scope) kwargs[argname] = result + # if function has been defined with @pytest.fixture, we want to + # pass the special __being_called_by_pytest parameter so we don't raise a warning + # this is an ugly hack, see #3720 for an opportunity to improve this + defined_using_fixture_decorator = hasattr(fixturedef.func, "_pytestfixturefunction") + if defined_using_fixture_decorator: + kwargs["__being_called_by_pytest"] = True + fixturefunc = resolve_fixture_function(fixturedef, request) my_cache_key = request.param_index try: @@ -947,6 +957,44 @@ def _ensure_immutable_ids(ids): return tuple(ids) +def wrap_function_to_warning_if_called_directly(function, fixture_marker): + """Wrap the given fixture function so we can issue warnings about it being called directly, instead of + used as an argument in a test function. + + The warning is emitted only in Python 3, because I didn't find a reliable way to make the wrapper function + keep the original signature, and we probably will drop Python 2 in Pytest 4 anyway. + """ + is_yield_function = is_generator(function) + msg = FIXTURE_FUNCTION_CALL.format(name=fixture_marker.name or function.__name__) + warning = RemovedInPytest4Warning(msg) + + if is_yield_function: + + @functools.wraps(function) + def result(*args, **kwargs): + __tracebackhide__ = True + __being_called_by_pytest = kwargs.pop("__being_called_by_pytest", False) + if not __being_called_by_pytest: + warnings.warn(warning, stacklevel=3) + for x in function(*args, **kwargs): + yield x + + else: + + @functools.wraps(function) + def result(*args, **kwargs): + __tracebackhide__ = True + __being_called_by_pytest = kwargs.pop("__being_called_by_pytest", False) + if not __being_called_by_pytest: + warnings.warn(warning, stacklevel=3) + return function(*args, **kwargs) + + if six.PY2: + result.__wrapped__ = function + + return result + + @attr.s(frozen=True) class FixtureFunctionMarker(object): scope = attr.ib() @@ -964,6 +1012,8 @@ def __call__(self, function): "fixture is being applied more than once to the same function" ) + function = wrap_function_to_warning_if_called_directly(function, self) + function._pytestfixturefunction = self return function diff --git a/src/_pytest/pytester.py b/src/_pytest/pytester.py index 5b42b81eedf..e75a09db5d5 100644 --- a/src/_pytest/pytester.py +++ b/src/_pytest/pytester.py @@ -669,6 +669,11 @@ def copy_example(self, name=None): else: raise LookupError("example is not found as a file or directory") + def run_example(self, name=None, *pytest_args): + """Runs the given example and returns the results of the run""" + p = self.copy_example(name) + return self.runpytest(p, *pytest_args) + Session = Session def getnode(self, config, arg): diff --git a/testing/deprecated_test.py b/testing/deprecated_test.py index b82dbcf7d85..2a9b4be9135 100644 --- a/testing/deprecated_test.py +++ b/testing/deprecated_test.py @@ -1,4 +1,6 @@ from __future__ import absolute_import, division, print_function + + import pytest @@ -263,3 +265,15 @@ def test_func(): str(PYTEST_PLUGINS_FROM_NON_TOP_LEVEL_CONFTEST).splitlines()[0] not in res.stderr.str() ) + + +# @pytest.mark.skipif(six.PY2, reason="We issue the warning in Python 3 only") +def test_call_fixture_function_deprecated(): + """Check if a warning is raised if a fixture function is called directly (#3661)""" + + @pytest.fixture + def fix(): + return 1 + + with pytest.deprecated_call(): + assert fix() == 1 diff --git a/testing/example_scripts/tmpdir/tmpdir_fixture.py b/testing/example_scripts/tmpdir/tmpdir_fixture.py new file mode 100644 index 00000000000..f4ad07462cb --- /dev/null +++ b/testing/example_scripts/tmpdir/tmpdir_fixture.py @@ -0,0 +1,7 @@ +import pytest + + +@pytest.mark.parametrize("a", [r"qwe/\abc"]) +def test_fixture(tmpdir, a): + tmpdir.check(dir=1) + assert tmpdir.listdir() == [] diff --git a/testing/python/fixture.py b/testing/python/fixture.py index 3e34f384a48..70d79ab7132 100644 --- a/testing/python/fixture.py +++ b/testing/python/fixture.py @@ -1456,6 +1456,7 @@ def test_parsefactories_conftest_and_module_and_class(self, testdir): testdir.makepyfile( """ import pytest + import six @pytest.fixture def hello(request): @@ -1468,9 +1469,11 @@ def test_hello(self, item, fm): faclist = fm.getfixturedefs("hello", item.nodeid) print (faclist) assert len(faclist) == 3 - assert faclist[0].func(item._request) == "conftest" - assert faclist[1].func(item._request) == "module" - assert faclist[2].func(item._request) == "class" + + kwargs = {'__being_called_by_pytest': True} + assert faclist[0].func(item._request, **kwargs) == "conftest" + assert faclist[1].func(item._request, **kwargs) == "module" + assert faclist[2].func(item._request, **kwargs) == "class" """ ) reprec = testdir.inline_run("-s") diff --git a/testing/test_assertion.py b/testing/test_assertion.py index aff644ee7d0..23763f07886 100644 --- a/testing/test_assertion.py +++ b/testing/test_assertion.py @@ -12,7 +12,6 @@ PY3 = sys.version_info >= (3, 0) -@pytest.fixture def mock_config(): class Config(object): verbose = False @@ -768,15 +767,15 @@ def test_rewritten(): assert testdir.runpytest().ret == 0 -def test_reprcompare_notin(mock_config): - detail = plugin.pytest_assertrepr_compare( - mock_config, "not in", "foo", "aaafoobbb" - )[1:] +def test_reprcompare_notin(): + config = mock_config() + detail = plugin.pytest_assertrepr_compare(config, "not in", "foo", "aaafoobbb")[1:] assert detail == ["'foo' is contained here:", " aaafoobbb", "? +++"] -def test_reprcompare_whitespaces(mock_config): - detail = plugin.pytest_assertrepr_compare(mock_config, "==", "\r\n", "\n") +def test_reprcompare_whitespaces(): + config = mock_config() + detail = plugin.pytest_assertrepr_compare(config, "==", "\r\n", "\n") assert detail == [ r"'\r\n' == '\n'", r"Strings contain only whitespace, escaping them using repr()", diff --git a/testing/test_conftest.py b/testing/test_conftest.py index 4b80f1f56d7..449ef528181 100644 --- a/testing/test_conftest.py +++ b/testing/test_conftest.py @@ -10,9 +10,7 @@ @pytest.fixture(scope="module", params=["global", "inpackage"]) def basedir(request, tmpdir_factory): - from _pytest.tmpdir import tmpdir - - tmpdir = tmpdir(request, tmpdir_factory) + tmpdir = tmpdir_factory.mktemp("basedir", numbered=True) tmpdir.ensure("adir/conftest.py").write("a=1 ; Directory = 3") tmpdir.ensure("adir/b/conftest.py").write("b=2 ; a = 1.5") if request.param == "inpackage": diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index db6e6867400..b351066a316 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -3,35 +3,10 @@ import py import pytest -from _pytest.tmpdir import tmpdir - -def test_funcarg(testdir): - testdir.makepyfile( - """ - def pytest_generate_tests(metafunc): - metafunc.addcall(id='a') - metafunc.addcall(id='b') - def test_func(tmpdir): pass - """ - ) - from _pytest.tmpdir import TempdirFactory - - reprec = testdir.inline_run() - calls = reprec.getcalls("pytest_runtest_setup") - item = calls[0].item - config = item.config - tmpdirhandler = TempdirFactory(config) - item._initrequest() - p = tmpdir(item._request, tmpdirhandler) - assert p.check() - bn = p.basename.strip("0123456789") - assert bn.endswith("test_func_a_") - item.name = "qwe/\\abc" - p = tmpdir(item._request, tmpdirhandler) - assert p.check() - bn = p.basename.strip("0123456789") - assert bn == "qwe__abc" +def test_tmpdir_fixture(testdir): + results = testdir.run_example("tmpdir/tmpdir_fixture.py") + results.stdout.fnmatch_lines("*1 passed*") def test_ensuretemp(recwarn): From 6e57d123bb240e4b3668fc4e422a6e0f01d08f17 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sun, 22 Jul 2018 17:23:38 -0300 Subject: [PATCH 2/3] Mark test_idval_hypothesis as flaky on Windows (#3707) --- testing/python/metafunc.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/testing/python/metafunc.py b/testing/python/metafunc.py index 9f425821b67..7ef34678c9c 100644 --- a/testing/python/metafunc.py +++ b/testing/python/metafunc.py @@ -212,6 +212,9 @@ class A(object): @hypothesis.settings( deadline=400.0 ) # very close to std deadline and CI boxes are not reliable in CPU power + @pytest.mark.xfail( + sys.platform.startswith("win32"), reason="flaky #3707", strict=False + ) def test_idval_hypothesis(self, value): from _pytest.python import _idval From 57b0c60cb4a083592cce04e8b383b55bf14a59dc Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 26 Jul 2018 20:10:40 -0300 Subject: [PATCH 3/3] Remove Testdir.run_example as recommended --- src/_pytest/pytester.py | 5 ----- testing/test_tmpdir.py | 3 ++- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/_pytest/pytester.py b/src/_pytest/pytester.py index e75a09db5d5..5b42b81eedf 100644 --- a/src/_pytest/pytester.py +++ b/src/_pytest/pytester.py @@ -669,11 +669,6 @@ def copy_example(self, name=None): else: raise LookupError("example is not found as a file or directory") - def run_example(self, name=None, *pytest_args): - """Runs the given example and returns the results of the run""" - p = self.copy_example(name) - return self.runpytest(p, *pytest_args) - Session = Session def getnode(self, config, arg): diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index b351066a316..487f9b21e54 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -5,7 +5,8 @@ def test_tmpdir_fixture(testdir): - results = testdir.run_example("tmpdir/tmpdir_fixture.py") + p = testdir.copy_example("tmpdir/tmpdir_fixture.py") + results = testdir.runpytest(p) results.stdout.fnmatch_lines("*1 passed*")