From 29fa9d5bff93c90526286117ddc788799ff18673 Mon Sep 17 00:00:00 2001 From: Tom Dalton Date: Mon, 23 Oct 2017 12:28:54 +0100 Subject: [PATCH 01/12] Add failing test --- testing/test_collection.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/testing/test_collection.py b/testing/test_collection.py index 5d1654410..f15e4925c 100644 --- a/testing/test_collection.py +++ b/testing/test_collection.py @@ -2,6 +2,7 @@ from __future__ import absolute_import, division, print_function import pytest import py +import _pytest._code from _pytest.main import Session, EXIT_NOTESTSCOLLECTED, _in_venv @@ -830,3 +831,28 @@ def test_continue_on_collection_errors_maxfail(testdir): "*Interrupted: stopping after 3 failures*", "*1 failed, 2 error*", ]) + + +def test_fixture_scope_sibling_conftests(testdir): + """Regression test case for https://github.com/pytest-dev/pytest/issues/2836""" + foo_path = testdir.mkpydir("foo") + foo_path.join("conftest.py").write(_pytest._code.Source(""" + import pytest + @pytest.fixture + def fix(): + return 1 + """)) + foo_path.join("test_foo.py").write("def test_foo(fix): assert fix == 1") + + # Tests in `food/` should not see the conftest fixture from `foo/` + food_path = testdir.mkpydir("food") + food_path.join("test_food.py").write("def test_food(fix): assert fix == 1") + + res = testdir.runpytest() + assert res.ret == 1 + + # TODO Work out what this will really look like. Currently the retcode assertion above fails (as expected). + res.stdout.fnmatch_lines([ + "collected 2 items / 1 errors", + "*1 passed, 1 error*", + ]) From c03612f729a81f60002b540cc1b1310f4fa94752 Mon Sep 17 00:00:00 2001 From: Tom Dalton Date: Mon, 23 Oct 2017 12:40:32 +0100 Subject: [PATCH 02/12] Test now looks for real expected output --- testing/test_collection.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testing/test_collection.py b/testing/test_collection.py index f15e4925c..49a290060 100644 --- a/testing/test_collection.py +++ b/testing/test_collection.py @@ -851,8 +851,8 @@ def test_fixture_scope_sibling_conftests(testdir): res = testdir.runpytest() assert res.ret == 1 - # TODO Work out what this will really look like. Currently the retcode assertion above fails (as expected). res.stdout.fnmatch_lines([ - "collected 2 items / 1 errors", + "*ERROR at setup of test_food*", + "E*fixture 'fix' not found", "*1 passed, 1 error*", ]) From 1e6dc6f8e5efd63c3626785982e34353a5d799f2 Mon Sep 17 00:00:00 2001 From: Tom Dalton Date: Mon, 23 Oct 2017 13:26:42 +0100 Subject: [PATCH 03/12] Working (I think) fix for #2836 --- _pytest/fixtures.py | 36 +++++++++++++++++++++++++++++++++++- testing/test_fixtures.py | 18 ++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 testing/test_fixtures.py diff --git a/_pytest/fixtures.py b/_pytest/fixtures.py index e1c2d05f4..51ccaed67 100644 --- a/_pytest/fixtures.py +++ b/_pytest/fixtures.py @@ -1130,7 +1130,41 @@ class FixtureManager: else: return tuple(self._matchfactories(fixturedefs, nodeid)) + @classmethod + def _splitnode(cls, nodeid): + """Split a nodeid into constituent 'parts'. + + Node IDs are strings, and can be things like: + '', + 'testing/code', + 'testing/code/test_excinfo.py' + 'testing/code/test_excinfo.py::TestFormattedExcinfo::()' + + """ + if nodeid == '': + return [] + sep = py.path.local.sep + parts = nodeid.split(sep) + if parts: + last_part = parts[-1] + if '::' in last_part: + namespace_parts = last_part.split("::") + parts[-1:] = namespace_parts + return parts + + @classmethod + def _ischildnode(cls, baseid, nodeid): + """Return True if the nodeid is a child node of the baseid. + + E.g. 'foo/bar::Baz::()' is a child of 'foo', 'foo/bar' and 'foo/bar::Baz', but not of 'foo/blorp' + """ + base_parts = cls._splitnode(baseid) + node_parts = cls._splitnode(nodeid) + if len(node_parts) < len(base_parts): + return False + return node_parts[:len(base_parts)] == base_parts + def _matchfactories(self, fixturedefs, nodeid): for fixturedef in fixturedefs: - if nodeid.startswith(fixturedef.baseid): + if self._ischildnode(fixturedef.baseid, nodeid): yield fixturedef diff --git a/testing/test_fixtures.py b/testing/test_fixtures.py new file mode 100644 index 000000000..8d595fe52 --- /dev/null +++ b/testing/test_fixtures.py @@ -0,0 +1,18 @@ +import pytest + +from _pytest import fixtures + + +@pytest.mark.parametrize("baseid, nodeid, expected", ( + ('', '', True), + ('', 'foo', True), + ('', 'foo/bar', True), + ('', 'foo/bar::TestBaz::()', True), + ('foo', 'food', False), + ('foo/bar::TestBaz::()', 'foo/bar', False), + ('foo/bar::TestBaz::()', 'foo/bar::TestBop::()', False), + ('foo/bar', 'foo/bar::TestBop::()', True), +)) +def test_fixturemanager_ischildnode(baseid, nodeid, expected): + result = fixtures.FixtureManager._ischildnode(baseid, nodeid) + assert result is expected From f003914d4b066e4825da2afa1180914620eecfbc Mon Sep 17 00:00:00 2001 From: Tom Dalton Date: Mon, 23 Oct 2017 15:37:02 +0100 Subject: [PATCH 04/12] Add changelog entry for #2836 --- changelog/2836.bug | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/2836.bug diff --git a/changelog/2836.bug b/changelog/2836.bug new file mode 100644 index 000000000..a6e36a34b --- /dev/null +++ b/changelog/2836.bug @@ -0,0 +1,3 @@ +Attempted fix for https://github.com/pytest-dev/pytest/issues/2836 + +Improves the handling of "nodeid"s to be more aware of path and class separators. From de9d116a49eff117f124b70f89e0bd0e5cd8ffc0 Mon Sep 17 00:00:00 2001 From: Tom Dalton Date: Mon, 23 Oct 2017 15:37:15 +0100 Subject: [PATCH 05/12] Added Tom Dalton to AUTHORS --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index 6e341d64e..cbebd9e30 100644 --- a/AUTHORS +++ b/AUTHORS @@ -164,6 +164,7 @@ Stephan Obermann Tareq Alayan Ted Xiao Thomas Grainger +Tom Dalton Tom Viner Trevor Bekolay Tyler Goodlet From ee7e1c94d2436e82ad4e455f0cc436ddacae44e3 Mon Sep 17 00:00:00 2001 From: Tom Dalton Date: Mon, 23 Oct 2017 16:12:07 +0100 Subject: [PATCH 06/12] Remove redundant if, tidy if-body --- _pytest/fixtures.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/_pytest/fixtures.py b/_pytest/fixtures.py index 51ccaed67..f69326f1d 100644 --- a/_pytest/fixtures.py +++ b/_pytest/fixtures.py @@ -1145,11 +1145,10 @@ class FixtureManager: return [] sep = py.path.local.sep parts = nodeid.split(sep) - if parts: - last_part = parts[-1] - if '::' in last_part: - namespace_parts = last_part.split("::") - parts[-1:] = namespace_parts + last_part = parts[-1] + if '::' in last_part: + # Replace single last element 'test_foo.py::Bar::()' with multiple elements 'test_foo.py', 'Bar', '()' + parts[-1:] = last_part.split("::") return parts @classmethod From d714c196a523a121960573fa75771572c4db0b9b Mon Sep 17 00:00:00 2001 From: Tom Dalton Date: Mon, 23 Oct 2017 16:55:35 +0100 Subject: [PATCH 07/12] Shorter code, longer docstring --- _pytest/fixtures.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/_pytest/fixtures.py b/_pytest/fixtures.py index f69326f1d..c62e9185a 100644 --- a/_pytest/fixtures.py +++ b/_pytest/fixtures.py @@ -1135,20 +1135,20 @@ class FixtureManager: """Split a nodeid into constituent 'parts'. Node IDs are strings, and can be things like: - '', - 'testing/code', + '' + 'testing/code' 'testing/code/test_excinfo.py' 'testing/code/test_excinfo.py::TestFormattedExcinfo::()' + Return values are lists e.g. + [''] + ['testing', 'code'] + ['testing', 'code', test_excinfo.py'] + ['testing', 'code', 'test_excinfo.py', 'TestFormattedExcinfo', '()'] """ - if nodeid == '': - return [] - sep = py.path.local.sep - parts = nodeid.split(sep) - last_part = parts[-1] - if '::' in last_part: - # Replace single last element 'test_foo.py::Bar::()' with multiple elements 'test_foo.py', 'Bar', '()' - parts[-1:] = last_part.split("::") + parts = nodeid.split(py.path.local.sep) + # Replace single last element 'test_foo.py::Bar::()' with multiple elements 'test_foo.py', 'Bar', '()' + parts[-1:] = parts[-1].split("::") return parts @classmethod From a7199fa8ab0d90dccd2496f122d5f438af961448 Mon Sep 17 00:00:00 2001 From: Tom Dalton Date: Mon, 23 Oct 2017 16:59:56 +0100 Subject: [PATCH 08/12] Docstring typo --- _pytest/fixtures.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/_pytest/fixtures.py b/_pytest/fixtures.py index c62e9185a..9b2abc94f 100644 --- a/_pytest/fixtures.py +++ b/_pytest/fixtures.py @@ -1143,7 +1143,7 @@ class FixtureManager: Return values are lists e.g. [''] ['testing', 'code'] - ['testing', 'code', test_excinfo.py'] + ['testing', 'code', 'test_excinfo.py'] ['testing', 'code', 'test_excinfo.py', 'TestFormattedExcinfo', '()'] """ parts = nodeid.split(py.path.local.sep) From 655ab0bf8b8af9962e40360b40c7e5b55c2092f6 Mon Sep 17 00:00:00 2001 From: Tom Dalton Date: Mon, 23 Oct 2017 17:49:49 +0100 Subject: [PATCH 09/12] Address more review comments, fix massive bug I reintroduced in the node-splitting code :-/ --- _pytest/fixtures.py | 51 ++++----------------- _pytest/nodes.py | 37 +++++++++++++++ changelog/2836.bug | 4 +- testing/{test_fixtures.py => test_nodes.py} | 6 +-- 4 files changed, 51 insertions(+), 47 deletions(-) create mode 100644 _pytest/nodes.py rename testing/{test_fixtures.py => test_nodes.py} (71%) diff --git a/_pytest/fixtures.py b/_pytest/fixtures.py index 9b2abc94f..3a1321664 100644 --- a/_pytest/fixtures.py +++ b/_pytest/fixtures.py @@ -1,12 +1,12 @@ from __future__ import absolute_import, division, print_function -import sys - -from py._code.code import FormattedExcinfo - -import py -import warnings import inspect +import sys +import warnings + +import py +from py._code.code import FormattedExcinfo + import _pytest from _pytest._code.code import TerminalRepr from _pytest.compat import ( @@ -15,9 +15,11 @@ from _pytest.compat import ( is_generator, isclass, getimfunc, getlocation, getfuncargnames, safe_getattr, + FuncargnamesCompatAttr, ) +from _pytest.nodes import ischildnode from _pytest.outcomes import fail, TEST_OUTCOME -from _pytest.compat import FuncargnamesCompatAttr + if sys.version_info[:2] == (2, 6): from ordereddict import OrderedDict @@ -1130,40 +1132,7 @@ class FixtureManager: else: return tuple(self._matchfactories(fixturedefs, nodeid)) - @classmethod - def _splitnode(cls, nodeid): - """Split a nodeid into constituent 'parts'. - - Node IDs are strings, and can be things like: - '' - 'testing/code' - 'testing/code/test_excinfo.py' - 'testing/code/test_excinfo.py::TestFormattedExcinfo::()' - - Return values are lists e.g. - [''] - ['testing', 'code'] - ['testing', 'code', 'test_excinfo.py'] - ['testing', 'code', 'test_excinfo.py', 'TestFormattedExcinfo', '()'] - """ - parts = nodeid.split(py.path.local.sep) - # Replace single last element 'test_foo.py::Bar::()' with multiple elements 'test_foo.py', 'Bar', '()' - parts[-1:] = parts[-1].split("::") - return parts - - @classmethod - def _ischildnode(cls, baseid, nodeid): - """Return True if the nodeid is a child node of the baseid. - - E.g. 'foo/bar::Baz::()' is a child of 'foo', 'foo/bar' and 'foo/bar::Baz', but not of 'foo/blorp' - """ - base_parts = cls._splitnode(baseid) - node_parts = cls._splitnode(nodeid) - if len(node_parts) < len(base_parts): - return False - return node_parts[:len(base_parts)] == base_parts - def _matchfactories(self, fixturedefs, nodeid): for fixturedef in fixturedefs: - if self._ischildnode(fixturedef.baseid, nodeid): + if ischildnode(fixturedef.baseid, nodeid): yield fixturedef diff --git a/_pytest/nodes.py b/_pytest/nodes.py new file mode 100644 index 000000000..c28c63180 --- /dev/null +++ b/_pytest/nodes.py @@ -0,0 +1,37 @@ +import py + + +def _splitnode(nodeid): + """Split a nodeid into constituent 'parts'. + + Node IDs are strings, and can be things like: + '' + 'testing/code' + 'testing/code/test_excinfo.py' + 'testing/code/test_excinfo.py::TestFormattedExcinfo::()' + + Return values are lists e.g. + [] + ['testing', 'code'] + ['testing', 'code', 'test_excinfo.py'] + ['testing', 'code', 'test_excinfo.py', 'TestFormattedExcinfo', '()'] + """ + if nodeid == '': + # If there is no root node at all, return an empty list so the caller's logic can remain sane + return [] + parts = nodeid.split(py.path.local.sep) + # Replace single last element 'test_foo.py::Bar::()' with multiple elements 'test_foo.py', 'Bar', '()' + parts[-1:] = parts[-1].split("::") + return parts + + +def ischildnode(baseid, nodeid): + """Return True if the nodeid is a child node of the baseid. + + E.g. 'foo/bar::Baz::()' is a child of 'foo', 'foo/bar' and 'foo/bar::Baz', but not of 'foo/blorp' + """ + base_parts = _splitnode(baseid) + node_parts = _splitnode(nodeid) + if len(node_parts) < len(base_parts): + return False + return node_parts[:len(base_parts)] == base_parts diff --git a/changelog/2836.bug b/changelog/2836.bug index a6e36a34b..afa1961d7 100644 --- a/changelog/2836.bug +++ b/changelog/2836.bug @@ -1,3 +1 @@ -Attempted fix for https://github.com/pytest-dev/pytest/issues/2836 - -Improves the handling of "nodeid"s to be more aware of path and class separators. +Match fixture paths against actual path segments in order to avoid matching folders which share a prefix. diff --git a/testing/test_fixtures.py b/testing/test_nodes.py similarity index 71% rename from testing/test_fixtures.py rename to testing/test_nodes.py index 8d595fe52..6f4540f99 100644 --- a/testing/test_fixtures.py +++ b/testing/test_nodes.py @@ -1,6 +1,6 @@ import pytest -from _pytest import fixtures +from _pytest import nodes @pytest.mark.parametrize("baseid, nodeid, expected", ( @@ -13,6 +13,6 @@ from _pytest import fixtures ('foo/bar::TestBaz::()', 'foo/bar::TestBop::()', False), ('foo/bar', 'foo/bar::TestBop::()', True), )) -def test_fixturemanager_ischildnode(baseid, nodeid, expected): - result = fixtures.FixtureManager._ischildnode(baseid, nodeid) +def test_ischildnode(baseid, nodeid, expected): + result = nodes.ischildnode(baseid, nodeid) assert result is expected From a3ec3df0c883d0655beadb44e3873c25bc38d06c Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 23 Oct 2017 18:19:15 -0200 Subject: [PATCH 10/12] Add E722 and E741 flake errors to the ignore list Also fixed 'E704 multiple statements on one line (def)' in python_api --- _pytest/python_api.py | 3 ++- tox.ini | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/_pytest/python_api.py b/_pytest/python_api.py index b73e0457c..a3cf1c8cb 100644 --- a/_pytest/python_api.py +++ b/_pytest/python_api.py @@ -217,7 +217,8 @@ class ApproxScalar(ApproxBase): absolute tolerance or a relative tolerance, depending on what the user specified or which would be larger. """ - def set_default(x, default): return x if x is not None else default + def set_default(x, default): + return x if x is not None else default # Figure out what the absolute tolerance should be. ``self.abs`` is # either None or a value specified by the user. diff --git a/tox.ini b/tox.ini index 9245ff418..33e5fa02c 100644 --- a/tox.ini +++ b/tox.ini @@ -213,3 +213,8 @@ filterwarnings = [flake8] max-line-length = 120 exclude = _pytest/vendored_packages/pluggy.py +ignore= + # do not use bare except' + E722 + # ambiguous variable name 'l' + E741 From 14e3a5fcb927adac8d5a07679f6655a46ef6c0cd Mon Sep 17 00:00:00 2001 From: Tom Dalton Date: Tue, 24 Oct 2017 10:42:16 +0100 Subject: [PATCH 11/12] Move the generic separator to a constant --- _pytest/fixtures.py | 8 ++++---- _pytest/junitxml.py | 3 ++- _pytest/main.py | 9 +++++---- _pytest/nodes.py | 5 ++++- _pytest/terminal.py | 3 ++- 5 files changed, 17 insertions(+), 11 deletions(-) diff --git a/_pytest/fixtures.py b/_pytest/fixtures.py index 3a1321664..f71f35768 100644 --- a/_pytest/fixtures.py +++ b/_pytest/fixtures.py @@ -8,6 +8,7 @@ import py from py._code.code import FormattedExcinfo import _pytest +from _pytest import nodes from _pytest._code.code import TerminalRepr from _pytest.compat import ( NOTSET, exc_clear, _format_args, @@ -17,7 +18,6 @@ from _pytest.compat import ( safe_getattr, FuncargnamesCompatAttr, ) -from _pytest.nodes import ischildnode from _pytest.outcomes import fail, TEST_OUTCOME @@ -983,8 +983,8 @@ class FixtureManager: # by their test id) if p.basename.startswith("conftest.py"): nodeid = p.dirpath().relto(self.config.rootdir) - if p.sep != "/": - nodeid = nodeid.replace(p.sep, "/") + if p.sep != nodes.SEP: + nodeid = nodeid.replace(p.sep, nodes.SEP) self.parsefactories(plugin, nodeid) def _getautousenames(self, nodeid): @@ -1134,5 +1134,5 @@ class FixtureManager: def _matchfactories(self, fixturedefs, nodeid): for fixturedef in fixturedefs: - if ischildnode(fixturedef.baseid, nodeid): + if nodes.ischildnode(fixturedef.baseid, nodeid): yield fixturedef diff --git a/_pytest/junitxml.py b/_pytest/junitxml.py index ed3ba2e9a..7fb40dc35 100644 --- a/_pytest/junitxml.py +++ b/_pytest/junitxml.py @@ -17,6 +17,7 @@ import re import sys import time import pytest +from _pytest import nodes from _pytest.config import filename_arg # Python 2.X and 3.X compatibility @@ -252,7 +253,7 @@ def mangle_test_address(address): except ValueError: pass # convert file path to dotted path - names[0] = names[0].replace("/", '.') + names[0] = names[0].replace(nodes.SEP, '.') names[0] = _py_ext_re.sub("", names[0]) # put any params back names[-1] += possible_open_bracket + params diff --git a/_pytest/main.py b/_pytest/main.py index 4bddf1e2d..91083c85f 100644 --- a/_pytest/main.py +++ b/_pytest/main.py @@ -6,6 +6,7 @@ import os import sys import _pytest +from _pytest import nodes import _pytest._code import py try: @@ -14,8 +15,8 @@ except ImportError: from UserDict import DictMixin as MappingMixin from _pytest.config import directory_arg, UsageError, hookimpl -from _pytest.runner import collect_one_node from _pytest.outcomes import exit +from _pytest.runner import collect_one_node tracebackcutdir = py.path.local(_pytest.__file__).dirpath() @@ -516,14 +517,14 @@ class FSCollector(Collector): rel = fspath.relto(parent.fspath) if rel: name = rel - name = name.replace(os.sep, "/") + name = name.replace(os.sep, nodes.SEP) super(FSCollector, self).__init__(name, parent, config, session) self.fspath = fspath def _makeid(self): relpath = self.fspath.relto(self.config.rootdir) - if os.sep != "/": - relpath = relpath.replace(os.sep, "/") + if os.sep != nodes.SEP: + relpath = relpath.replace(os.sep, nodes.SEP) return relpath diff --git a/_pytest/nodes.py b/_pytest/nodes.py index c28c63180..7b6b695b5 100644 --- a/_pytest/nodes.py +++ b/_pytest/nodes.py @@ -1,6 +1,9 @@ import py +SEP = "/" + + def _splitnode(nodeid): """Split a nodeid into constituent 'parts'. @@ -19,7 +22,7 @@ def _splitnode(nodeid): if nodeid == '': # If there is no root node at all, return an empty list so the caller's logic can remain sane return [] - parts = nodeid.split(py.path.local.sep) + parts = nodeid.split(SEP) # Replace single last element 'test_foo.py::Bar::()' with multiple elements 'test_foo.py', 'Bar', '()' parts[-1:] = parts[-1].split("::") return parts diff --git a/_pytest/terminal.py b/_pytest/terminal.py index bb114391d..f56b966f3 100644 --- a/_pytest/terminal.py +++ b/_pytest/terminal.py @@ -13,6 +13,7 @@ import sys import time import platform +from _pytest import nodes import _pytest._pluggy as pluggy @@ -452,7 +453,7 @@ class TerminalReporter: if fspath: res = mkrel(nodeid).replace("::()", "") # parens-normalization - if nodeid.split("::")[0] != fspath.replace("\\", "/"): + if nodeid.split("::")[0] != fspath.replace("\\", nodes.SEP): res += " <- " + self.startdir.bestrelpath(fspath) else: res = "[location]" From f5e72d2f5f9817a504a8e01e0de4dc89880a7621 Mon Sep 17 00:00:00 2001 From: Tom Dalton Date: Tue, 24 Oct 2017 11:25:42 +0100 Subject: [PATCH 12/12] Unused import / lint --- _pytest/nodes.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/_pytest/nodes.py b/_pytest/nodes.py index 7b6b695b5..ad3af2ce6 100644 --- a/_pytest/nodes.py +++ b/_pytest/nodes.py @@ -1,6 +1,3 @@ -import py - - SEP = "/"