From 011f88f7e77b8b2e6de01d2c5588dda47c77093b Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sun, 22 Jul 2018 11:10:44 -0300 Subject: [PATCH] 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 000000000..baa30fccf --- /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 3a29c688e..a1cd3bd4e 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 b3c40c2ae..d32b675ae 100644 --- a/src/_pytest/deprecated.py +++ b/src/_pytest/deprecated.py @@ -22,6 +22,12 @@ FUNCARG_PREFIX = ( "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 207bf27e4..93557fa04 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -6,6 +6,8 @@ import os import sys import warnings from collections import OrderedDict, deque, defaultdict + +import six from more_itertools import flatten import attr @@ -29,6 +31,7 @@ from _pytest.compat import ( 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 @@ class FixtureFunctionMarker(object): "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 5b42b81ee..e75a09db5 100644 --- a/src/_pytest/pytester.py +++ b/src/_pytest/pytester.py @@ -669,6 +669,11 @@ class Testdir(object): 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 b82dbcf7d..2a9b4be91 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_pytest_plugins_in_non_top_level_conftest_deprecated_no_false_positives( 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 000000000..f4ad07462 --- /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 3e34f384a..70d79ab71 100644 --- a/testing/python/fixture.py +++ b/testing/python/fixture.py @@ -1456,6 +1456,7 @@ class TestFixtureManagerParseFactories(object): testdir.makepyfile( """ import pytest + import six @pytest.fixture def hello(request): @@ -1468,9 +1469,11 @@ class TestFixtureManagerParseFactories(object): 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 aff644ee7..23763f078 100644 --- a/testing/test_assertion.py +++ b/testing/test_assertion.py @@ -12,7 +12,6 @@ from _pytest.assertion import truncate PY3 = sys.version_info >= (3, 0) -@pytest.fixture def mock_config(): class Config(object): verbose = False @@ -768,15 +767,15 @@ def test_rewritten(testdir): 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 4b80f1f56..449ef5281 100644 --- a/testing/test_conftest.py +++ b/testing/test_conftest.py @@ -10,9 +10,7 @@ from _pytest.main import EXIT_NOTESTSCOLLECTED, EXIT_USAGEERROR @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 db6e68674..b351066a3 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -3,35 +3,10 @@ import sys 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):