-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Factor out own_markers
into a property
#7052
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
from functools import lru_cache | ||
from typing import Any | ||
from typing import Dict | ||
from typing import Iterator | ||
from typing import List | ||
from typing import Optional | ||
from typing import Set | ||
|
@@ -128,8 +129,8 @@ def __init__( | |
#: keywords/markers collected from all scopes | ||
self.keywords = NodeKeywords(self) | ||
|
||
#: the marker objects belonging to this node | ||
self.own_markers = [] # type: List[Mark] | ||
#: The (manually added) marks belonging to this node (start, end). | ||
self._own_markers = ([], []) # type: Tuple[List[Mark], List[Mark]] | ||
bluetech marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#: allow adding of extra keywords to use for matching | ||
self.extra_keyword_matches = set() # type: Set[str] | ||
|
@@ -174,6 +175,11 @@ def ihook(self): | |
""" fspath sensitive hook proxy used to call pytest hooks""" | ||
return self.session.gethookproxy(self.fspath) | ||
|
||
@property | ||
def own_markers(self) -> List[Mark]: | ||
"""The marker objects belonging to this node.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize the property name can't be changed but why "markers" rather than "marks"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. another critical detail i just realized, changing own_markers from a real object to a transient temporal property is breaking api change that breaks users that change own_markers directly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would extend the comment here. Suggestion (but please verify what I write!):
Would also add a note about modifying it, depending on the answer to the next question. BTW, is there actually any significance to the the order or marks?
Do you think changing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is significance to the order of marks in the sense, that they apply in the order they are declared/applied this is important for details like selection of correct messages/failures in retrospect i believe exposing own_markers was a mistake, however right now its public and mutable |
||
return self._own_markers[0] + self._own_markers[1] | ||
|
||
def __repr__(self): | ||
return "<{} {}>".format(self.__class__.__name__, getattr(self, "name", None)) | ||
|
||
|
@@ -255,11 +261,11 @@ def add_marker( | |
raise ValueError("is not a string or pytest.mark.* Marker") | ||
self.keywords[marker_.name] = marker | ||
if append: | ||
self.own_markers.append(marker_.mark) | ||
self._own_markers[1].append(marker_.mark) | ||
else: | ||
self.own_markers.insert(0, marker_.mark) | ||
self._own_markers[0].insert(0, marker_.mark) | ||
|
||
def iter_markers(self, name=None): | ||
def iter_markers(self, name: Optional[str] = None) -> Iterator[Mark]: | ||
""" | ||
:param name: if given, filter the results by the name attribute | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,7 @@ | |
from _pytest.compat import safe_getattr | ||
from _pytest.compat import safe_isclass | ||
from _pytest.compat import STRING_TYPES | ||
from _pytest.compat import TYPE_CHECKING | ||
from _pytest.config import Config | ||
from _pytest.config import hookimpl | ||
from _pytest.deprecated import FUNCARGNAMES | ||
|
@@ -271,28 +272,35 @@ class PyobjMixin: | |
module = pyobj_property("Module") | ||
cls = pyobj_property("Class") | ||
instance = pyobj_property("Instance") | ||
_ALLOW_MARKERS = True | ||
_obj_markers = None # type: Optional[List[Mark]] | ||
|
||
# Function and attributes that the mixin needs (for type-checking only). | ||
if TYPE_CHECKING: | ||
_own_markers = ([], []) # type: Tuple[List[Mark], List[Mark]] | ||
|
||
@property | ||
def obj(self): | ||
"""Underlying Python object.""" | ||
obj = getattr(self, "_obj", None) | ||
if obj is None: | ||
self._obj = obj = self._getobj() | ||
# XXX evil hack | ||
# used to avoid Instance collector marker duplication | ||
if self._ALLOW_MARKERS: | ||
self.own_markers.extend(get_unpacked_marks(self.obj)) | ||
return obj | ||
|
||
@obj.setter | ||
def obj(self, value): | ||
def obj(self, value) -> None: | ||
self._obj = value | ||
self._obj_markers = None | ||
|
||
def _getobj(self): | ||
"""Gets the underlying Python object. May be overwritten by subclasses.""" | ||
return getattr(self.parent.obj, self.name) | ||
|
||
@property | ||
def own_markers(self) -> List[Mark]: | ||
if self._obj_markers is None: | ||
self._obj_markers = get_unpacked_marks(self.obj) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be too expensive to do this eagerly (when |
||
return self._own_markers[0] + self._obj_markers + self._own_markers[1] | ||
|
||
def getmodpath(self, stopatmodule=True, includemodule=False): | ||
""" return python path relative to the containing module. """ | ||
chain = self.listchain() | ||
|
@@ -753,14 +761,14 @@ def xunit_setup_method_fixture(self, request): | |
|
||
|
||
class Instance(PyCollector): | ||
_ALLOW_MARKERS = False # hack, destroy later | ||
# instances share the object with their parents in a way | ||
# that duplicates markers instances if not taken out | ||
# can be removed at node structure reorganization time | ||
|
||
def _getobj(self): | ||
return self.parent.obj() | ||
|
||
@property | ||
def own_markers(self) -> List[Mark]: | ||
# Do not include markers from obj, coming from Class already. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess storing a mark directly on an instance ( |
||
return self._own_markers[0] + self._own_markers[1] | ||
|
||
def collect(self): | ||
self.session._fixturemanager.parsefactories(self) | ||
return super().collect() | ||
|
@@ -783,13 +791,13 @@ def hasnew(obj): | |
|
||
|
||
class CallSpec2: | ||
def __init__(self, metafunc): | ||
def __init__(self, metafunc: "Metafunc") -> None: | ||
self.metafunc = metafunc | ||
self.funcargs = {} | ||
self._idlist = [] | ||
self.params = {} | ||
self._arg2scopenum = {} # used for sorting parametrized resources | ||
self.marks = [] | ||
self.marks = [] # type: List[Mark] | ||
self.indices = {} | ||
|
||
def copy(self): | ||
|
@@ -1432,9 +1440,6 @@ class Function(PyobjMixin, nodes.Item): | |
Python test function. | ||
""" | ||
|
||
# disable since functions handle it themselves | ||
_ALLOW_MARKERS = False | ||
|
||
def __init__( | ||
self, | ||
name, | ||
|
@@ -1443,7 +1448,7 @@ def __init__( | |
config=None, | ||
callspec: Optional[CallSpec2] = None, | ||
callobj=NOTSET, | ||
keywords=None, | ||
keywords: Optional[Iterable[str]] = None, | ||
session=None, | ||
fixtureinfo: Optional[FuncFixtureInfo] = None, | ||
originalname=None, | ||
|
@@ -1454,7 +1459,6 @@ def __init__( | |
self.obj = callobj | ||
|
||
self.keywords.update(self.obj.__dict__) | ||
self.own_markers.extend(get_unpacked_marks(self.obj)) | ||
if callspec: | ||
self.callspec = callspec | ||
# this is total hostile and a mess | ||
|
@@ -1464,7 +1468,6 @@ def __init__( | |
# feel free to cry, this was broken for years before | ||
# and keywords cant fix it per design | ||
self.keywords[mark.name] = mark | ||
self.own_markers.extend(normalize_mark_list(callspec.marks)) | ||
if keywords: | ||
self.keywords.update(keywords) | ||
|
||
|
@@ -1509,6 +1512,14 @@ def function(self): | |
"underlying python 'function' object" | ||
return getimfunc(self.obj) | ||
|
||
@property | ||
def own_markers(self) -> List[Mark]: | ||
if self._obj_markers is None: | ||
self._obj_markers = get_unpacked_marks(self.obj) | ||
if hasattr(self, "callspec"): | ||
self._obj_markers += normalize_mark_list(self.callspec.marks) | ||
return self._own_markers[0] + self._obj_markers + self._own_markers[1] | ||
|
||
def _getobj(self): | ||
name = self.name | ||
i = name.find("[") # parametrization | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,11 +3,14 @@ | |
from unittest import mock | ||
|
||
import pytest | ||
from _pytest.config import Config | ||
from _pytest.config import ExitCode | ||
from _pytest.mark import EMPTY_PARAMETERSET_OPTION | ||
from _pytest.mark import MarkGenerator as Mark | ||
from _pytest.mark.structures import NodeKeywords | ||
from _pytest.nodes import Collector | ||
from _pytest.nodes import Node | ||
from _pytest.python import Function | ||
|
||
|
||
class TestMark: | ||
|
@@ -961,7 +964,7 @@ class TestBarClass(BaseTests): | |
# assert skipped_k == failed_k == 0 | ||
|
||
|
||
def test_addmarker_order(): | ||
def test_addmarker_order(pytestconfig: Config, monkeypatch) -> None: | ||
session = mock.Mock() | ||
session.own_markers = [] | ||
session.parent = None | ||
|
@@ -973,6 +976,53 @@ def test_addmarker_order(): | |
extracted = [x.name for x in node.iter_markers()] | ||
assert extracted == ["baz", "foo", "bar"] | ||
|
||
# Check marks/keywords with Function. | ||
session.name = "session" | ||
session.keywords = NodeKeywords(session) | ||
|
||
# Register markers for `--strict-markers`. | ||
added_markers = pytestconfig._inicache["markers"] + [ | ||
"funcmark", | ||
"prepended", | ||
"funcmark2", | ||
] | ||
monkeypatch.setitem(pytestconfig._inicache, "markers", added_markers) | ||
|
||
@pytest.mark.funcmark | ||
def f1(): | ||
assert False, "don't call me" | ||
|
||
func = Function.from_parent(node, name="func", callobj=f1) | ||
expected_marks = ["funcmark", "baz", "foo", "bar"] | ||
assert [x.name for x in func.iter_markers()] == expected_marks | ||
func.add_marker("prepended", append=False) | ||
assert [x.name for x in func.iter_markers()] == ["prepended"] + expected_marks | ||
assert set(func.keywords) == { | ||
"Test", | ||
"bar", | ||
"baz", | ||
"foo", | ||
"func", | ||
"funcmark", | ||
"prepended", | ||
"pytestmark", | ||
"session", | ||
} | ||
|
||
# Changing the "obj" updates marks and keywords (lazily). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "keywords" being updated is not done here yet (but in blueyed#359 already). Could test the current behavior here for now. |
||
@pytest.mark.funcmark2 | ||
def f2(): | ||
assert False, "don't call me" | ||
|
||
func.obj = f2 | ||
assert [x.name for x in func.iter_markers()] == [ | ||
"prepended", | ||
"funcmark2", | ||
"baz", | ||
"foo", | ||
"bar", | ||
] | ||
|
||
|
||
@pytest.mark.filterwarnings("ignore") | ||
def test_markers_from_parametrize(testdir): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "start", "end" mean? (=> would be good to explain in the comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bluetech
It keeps two list for prepended/appended marks - with the ones from the object inbetween. This might not be needed really, but is needed for the current behavior (fixing it for changed
obj
s etc though).Having two separate lists might be good, but also not really needed maybe (given that the single list can be typed "enough" already).
Any suggestions for a better/clearer comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, looking a bit more I can see either way.
First I think the name
_own_markers
is a little misleading now: the established meaning ofown_markers
(which can't be changed due to backward compat) includes the manually-added marks (add_marker()
) but also the marks directly applied to the object (@pytest.mark.foo
). But after this change,_own_markers
only contains theadd_marker()
ones.So maybe
_manually_added_markers
?As regards to the comment, maybe something like this?