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.

Sign up for GitHub

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

Jump to bottom

Function: use originalname in _getobj and make it default to name #7035

Merged
merged 4 commits into from
May 18, 2020
Merged

Function: use originalname in _getobj and make it default to name #7035

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d4b5990
Function: use `originalname` in `_getobj` and make it default to `name`
blueyed Apr 7, 2020
c145437
Update src/_pytest/python.py
blueyed Apr 7, 2020
3e44a10
Apply bluetech suggestions on PR
nicoddemus May 15, 2020
faa50ca
Apply doc updates to Function as suggested in review
nicoddemus May 18, 2020
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: 7 additions & 11 deletions src/_pytest/python.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1453,6 +1453,12 @@ def __init__(
if callobj is not NOTSET:
self.obj = callobj

#: original function name, without any decorations (for example
blueyed marked this conversation as resolved.
Show resolved Hide resolved
#: parametrization adds a ``"[...]"`` suffix to function names).
#:
#: .. versionadded:: 3.0
self.originalname = originalname if originalname is not None else name
Copy link
Member

Choose a reason for hiding this comment

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

Given a field called "originalname", I'd expect it to only contain the original name, but this adds a fallback to name, which is presumably not the original name? If so, then this seems unadvisable. Maybe do this if in _getobj instead?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally this would be a alias for functiondefinition.name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bluetech the point is that it should be usable as-is, i.e. also for non-parametrized functions (where name is the original name AFAICT).
Check 1a79137: it only passes it in for parametrized functions.

Copy link
Member

Choose a reason for hiding this comment

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

(Replying only to the diff, not Ronny's comment which I didn't look into)

Check 1a79137: it only passes it in for parametrized functions.

Would it be possible to fix that? I.e. make originalname be passed always, even when it's the same as name? That would address my concern and makes some sense anyway I think...

Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to fix that?

Done it. 👍

I tried @RonnyPfannschmidt's suggestion, which I think is more elegant, but the problem is that _getobj uses originalname to obtain the function, and originalname requires the function, so we get a recursion.


self.keywords.update(self.obj.__dict__)
self.own_markers.extend(get_unpacked_marks(self.obj))
if callspec:
Expand Down Expand Up @@ -1487,12 +1493,6 @@ def __init__(
self.fixturenames = fixtureinfo.names_closure
self._initrequest()

#: original function name, without any decorations (for example
#: parametrization adds a ``"[...]"`` suffix to function names).
#:
#: .. versionadded:: 3.0
self.originalname = originalname

@classmethod
def from_parent(cls, parent, **kw): # todo: determine sound type limitations
"""
Expand All @@ -1510,11 +1510,7 @@ def function(self):
return getimfunc(self.obj)

def _getobj(self):
name = self.name
i = name.find("[") # parametrization
if i != -1:
name = name[:i]
return getattr(self.parent.obj, name)
return getattr(self.parent.obj, self.originalname)

@property
def _pyfuncitem(self):
Expand Down
28 changes: 26 additions & 2 deletions testing/python/collect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import pytest
from _pytest.config import ExitCode
from _pytest.nodes import Collector
from _pytest.pytester import Testdir


class TestModule:
Expand Down Expand Up @@ -659,16 +660,39 @@ def test_passed(x):
result = testdir.runpytest()
result.stdout.fnmatch_lines(["* 3 passed in *"])

def test_function_original_name(self, testdir):
def test_function_originalname(self, testdir: Testdir) -> None:
items = testdir.getitems(
"""
import pytest

@pytest.mark.parametrize('arg', [1,2])
def test_func(arg):
pass

def test_no_param():
pass
"""
)
assert [x.originalname for x in items] == ["test_func", "test_func"]
assert [x.originalname for x in items] == [
"test_func",
"test_func",
"test_no_param",
]

def test_function_with_square_brackets(self, testdir: Testdir) -> None:
"""Check that Function._getobj uses originalname."""
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: I think this description is too low-level, I would go for something more like "Check that functions with square brackets don't cause trouble".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's in the test name already.
I've added the extra doc to explicitly state what it is testing internally.

Copy link
Member

Choose a reason for hiding this comment

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

I would leave it out then -- it might just become outdated if the mechanism changes, but the test itself will stay relevant always because it tests high-level behavior.

Feel free to disregard though if you prefer to keep it :)

p1 = testdir.makepyfile(
"""
locals()["test_foo[name]"] = lambda: None
"""
)
result = testdir.runpytest("-v", str(p1))
result.stdout.fnmatch_lines(
[
"test_function_with_square_brackets.py::test_foo[[]name[]] PASSED *",
bluetech marked this conversation as resolved.
Show resolved Hide resolved
"*= 1 passed in *",
]
)


class TestSorting:
Expand Down