Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node: do not add "::()" to nodeid #4358

Merged
merged 3 commits into from Nov 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions changelog/4358.removal.rst
@@ -0,0 +1,18 @@
Remove the ``::()`` notation to denote a test class instance in node ids.

Previously, node ids that contain test instances would use ``::()`` to denote the instance like this::

test_foo.py::Test::()::test_bar

The extra ``::()`` was puzzling to most users and has been removed, so that the test id becomes now::

test_foo.py::Test::test_bar

This change could not accompany a deprecation period as is usual when user-facing functionality changes because
it was not really possible to detect when the functionality was being used explicitly.

The extra ``::()`` might have been removed in some places internally already,
which then led to confusion in places where it was expected, e.g. with
``--deselect`` (`#4127 <https://github.com/pytest-dev/pytest/issues/4127>`_).

Test class instances are also not listed with ``--collect-only`` anymore.
12 changes: 7 additions & 5 deletions src/_pytest/nodes.py
Expand Up @@ -27,7 +27,7 @@ def _splitnode(nodeid):
''
'testing/code'
'testing/code/test_excinfo.py'
'testing/code/test_excinfo.py::TestFormattedExcinfo::()'
'testing/code/test_excinfo.py::TestFormattedExcinfo'

Return values are lists e.g.
[]
Expand All @@ -39,15 +39,15 @@ def _splitnode(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(SEP)
# Replace single last element 'test_foo.py::Bar::()' with multiple elements 'test_foo.py', 'Bar', '()'
# 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'
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)
Expand Down Expand Up @@ -107,10 +107,12 @@ def __init__(
self._name2pseudofixturedef = {}

if nodeid is not None:
assert "::()" not in nodeid
self._nodeid = nodeid
else:
assert parent is not None
self._nodeid = self.parent.nodeid + "::" + self.name
self._nodeid = self.parent.nodeid
if self.name != "()":
self._nodeid += "::" + self.name

@property
def ihook(self):
Expand Down
3 changes: 1 addition & 2 deletions src/_pytest/runner.py
Expand Up @@ -60,8 +60,7 @@ def pytest_terminal_summary(terminalreporter):
tr.write_line("")
tr.write_line("(0.00 durations hidden. Use -vv to show these durations.)")
break
nodeid = rep.nodeid.replace("::()::", "::")
tr.write_line("%02.2fs %-8s %s" % (rep.duration, rep.when, nodeid))
tr.write_line("%02.2fs %-8s %s" % (rep.duration, rep.when, rep.nodeid))


def pytest_sessionstart(session):
Expand Down
10 changes: 4 additions & 6 deletions src/_pytest/terminal.py
Expand Up @@ -605,9 +605,7 @@ def _printcollecteditems(self, items):
self._tw.line("%s: %d" % (name, count))
else:
for item in items:
nodeid = item.nodeid
nodeid = nodeid.replace("::()::", "::")
self._tw.line(nodeid)
self._tw.line(item.nodeid)
return
stack = []
indent = ""
Expand All @@ -619,8 +617,8 @@ def _printcollecteditems(self, items):
stack.pop()
for col in needed_collectors[len(stack) :]:
stack.append(col)
# if col.name == "()":
# continue
if col.name == "()": # Skip Instances.
continue
indent = (len(stack) - 1) * " "
self._tw.line("%s%s" % (indent, col))

Expand Down Expand Up @@ -687,7 +685,7 @@ def mkrel(nodeid):
# collect_fspath comes from testid which has a "/"-normalized path

if fspath:
res = mkrel(nodeid).replace("::()", "") # parens-normalization
res = mkrel(nodeid)
if self.verbosity >= 2 and nodeid.split("::")[0] != fspath.replace(
"\\", nodes.SEP
):
Expand Down
4 changes: 1 addition & 3 deletions testing/python/collect.py
Expand Up @@ -1408,9 +1408,7 @@ def test_hello(self):
"""
)
result = testdir.runpytest("--collect-only")
result.stdout.fnmatch_lines(
["*MyClass*", "*MyInstance*", "*MyFunction*test_hello*"]
)
result.stdout.fnmatch_lines(["*MyClass*", "*MyFunction*test_hello*"])


def test_unorderable_types(testdir):
Expand Down
11 changes: 3 additions & 8 deletions testing/test_collection.py
Expand Up @@ -510,13 +510,8 @@ def test_method(self):
pass
"""
)
normid = p.basename + "::TestClass::()::test_method"
for id in [
p.basename,
p.basename + "::TestClass",
p.basename + "::TestClass::()",
normid,
]:
normid = p.basename + "::TestClass::test_method"
for id in [p.basename, p.basename + "::TestClass", normid]:
Copy link
Contributor Author

@blueyed blueyed Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw: I would really prefer if black would not make a) the diff unnecessarily bigger here by merging this into a single line, and b) keep it for readability. psf/black#601

items, hookrec = testdir.inline_genitems(id)
assert len(items) == 1
assert items[0].name == "test_method"
Expand Down Expand Up @@ -625,7 +620,7 @@ def test_method(self):
items, hookrec = testdir.inline_genitems(arg)
assert len(items) == 1
item, = items
assert item.nodeid.endswith("TestClass::()::test_method")
assert item.nodeid.endswith("TestClass::test_method")
# ensure we are reporting the collection of the single test item (#2464)
assert [x.name for x in self.get_reported_items(hookrec)] == ["test_method"]

Expand Down
8 changes: 4 additions & 4 deletions testing/test_nodes.py
Expand Up @@ -8,11 +8,11 @@
("", "", True),
("", "foo", True),
("", "foo/bar", True),
("", "foo/bar::TestBaz::()", 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),
("foo/bar::TestBaz", "foo/bar", False),
("foo/bar::TestBaz", "foo/bar::TestBop", False),
("foo/bar", "foo/bar::TestBop", True),
),
)
def test_ischildnode(baseid, nodeid, expected):
Expand Down
14 changes: 12 additions & 2 deletions testing/test_session.py
Expand Up @@ -274,16 +274,26 @@ def test_deselect(testdir):
testdir.makepyfile(
test_a="""
import pytest

def test_a1(): pass

@pytest.mark.parametrize('b', range(3))
def test_a2(b): pass

class TestClass:
def test_c1(self): pass

def test_c2(self): pass
"""
)
result = testdir.runpytest(
"-v", "--deselect=test_a.py::test_a2[1]", "--deselect=test_a.py::test_a2[2]"
"-v",
"--deselect=test_a.py::test_a2[1]",
"--deselect=test_a.py::test_a2[2]",
"--deselect=test_a.py::TestClass::test_c1",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should still support the old syntax, but figured it would not be worth it really.
People might be affected though if they use this in scripts already etc..?!
Anyway, it currently uses .startswith(), and therefore a Node.matches method should be added/used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it is not worth supporting the old syntax. 👍

)
assert result.ret == 0
result.stdout.fnmatch_lines(["*2 passed, 2 deselected*"])
result.stdout.fnmatch_lines(["*3 passed, 3 deselected*"])
for line in result.stdout.lines:
assert not line.startswith(("test_a.py::test_a2[1]", "test_a.py::test_a2[2]"))

Expand Down