Factor out `own_markers` into a property #7052

blueyed · 2020-04-09T07:12:20Z

Follow-up might be to do this for keywords also: blueyed#359

This makes it a bit more easier to follow, especially with the removed hack of setting it only based on `_ALLOW_MARKERS` property. This was triggered based on wondering why setting `_obj` directly would not set the marks.

bluetech

Haven't tried to understand this yet, but some initial comments.

bluetech · 2020-04-09T07:40:14Z

src/_pytest/nodes.py


-        #: the marker objects belonging to this node
-        self.own_markers = []  # type: List[Mark]
+        #: The (manually added) marks belonging to this node (start, end).


What does "start", "end" mean? (=> would be good to explain in the comment)

@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 objs 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?

Having two separate lists might be good, but also not really needed maybe

Yes, looking a bit more I can see either way.

Any suggestions for a better/clearer comment?

First I think the name _own_markers is a little misleading now: the established meaning of own_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 the add_marker() ones.

So maybe _manually_added_markers?

As regards to the comment, maybe something like this?

Marks that are manually added to this node with `add_marker()`. These marks are included in the node's `own_markers`. Marks in the first item are prepended, and marks in the second item are appended.

src/_pytest/nodes.py

bluetech · 2020-04-09T07:41:51Z

src/_pytest/nodes.py


+    @property
+    def own_markers(self) -> List[Mark]:
+        """The marker objects belonging to this node."""


I realize the property name can't be changed but why "markers" rather than "marks"?

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

@blueyed

I would extend the comment here. Suggestion (but please verify what I write!):

The marks directly belonging to this node. This includes marks manually added with `add_marker()`, and marks directly applied to the object (using `@pytest.mark.foo` decorations for example). It does not include marks inherited from parent nodes (for example, a mark applied to a class node is not included in a method's `own_markers`).

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?

@RonnyPfannschmidt

changing own_markers from a real object to a transient temporal property is breaking api change

Do you think changing own_markers directly is something that should be supported, or deprecated?

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

bluetech · 2020-04-09T07:44:34Z

src/_pytest/python.py



-class PyobjMixin:
+class PyobjMixin(nodes.Node):


I think this is not a good idea in general -- it makes it look like PyobjMixin is an actual Node.

TBH I'd prefer to get rid of PyobjMixin entirely (and this looks like a positive step towards that), but in my typing branch I solved it like this for now:

bluetech@a34e7ff#diff-88cb3736bed2f144d42f07ccbeb97eb0R280-R291

I think this is not a good idea in general -- it makes it look like PyobjMixin is an actual Node.

Agreed!

TBH I'd prefer to get rid of PyobjMixin entirely (and this looks like a positive step towards that), but in my typing branch I solved it like this for now:

bluetech@a34e7ff#diff-88cb3736bed2f144d42f07ccbeb97eb0R280-R291

Thanks - took that approach here also.

Note however that it should be parent: PyobjMixin with PyobjMixin really (since it assumes that parent has obj), which does not work without subclassing Node (Definition of "parent" in base class "PyobjMixin" is incompatible with definition in base class "Node"). And using parent: Optional[Union["Node", "PyobjMixin"]] = None, with Node results in Item "PyobjMixin" of "Union[Node, PyobjMixin]" has no attribute "config" etc.
So I think that subclassing Node here is the best approach for now.
Despite its name it is a base/intermediate class for all "python" nodes/collectors after all.

Note however that it should be parent: PyobjMixin with PyobjMixin really

I guess such constraints on parent can be added once the from_parent work is completed? For now however I'm not sure they're valid. It's definitely an issue that typing expose.

Despite its name it is a base/intermediate class for all "python" nodes/collectors after all.

Maybe changing it to a common base for all "python" nodes makes sense, I am not sure. One problem with that is that it breaks the clear Item/Collector split, since both types will derive from it. And might confuse MRO/from_parent a lot too...

Anyway, as long as it is a mixin and written as a mixin, IMO it is better not to derive from Node. And we should strive to eliminate it as such :)

RonnyPfannschmidt

replacing a bad hack planned for drop without coordination with a different thing that looks worse

for stuff like that please coordinate first

The test is not including keywords for now (in the end, #359)

blueyed · 2020-04-09T14:30:51Z

testing/test_mark.py

+        "session",
+    }
+
+    # Changing the "obj" updates marks and keywords (lazily).


"keywords" being updated is not done here yet (but in blueyed#359 already). Could test the current behavior here for now.

RonnyPfannschmidt · 2020-04-09T16:07:08Z

The refactoring as is, is not acceptable, please pick comprehensible internal names and explain the rationale or drop it

Before: Diff Coverage Diff: eb00182...HEAD, staged and unstaged changes ------------- src/_pytest/mark/structures.py (0.0%): Missing lines 274 src/_pytest/nodes.py (100%) src/_pytest/python.py (72.2%): Missing lines 290,301,1518-1520 testing/test_mark.py (61.5%): Missing lines 967,981,984,989,992-993,995,997-1000,1014-1015,1017-1018 ------------- Total: 63 lines Missing: 21 lines Coverage: 66% After: Diff Coverage Diff: eb00182...HEAD, staged and unstaged changes ------------- src/_pytest/mark/structures.py (100%) src/_pytest/nodes.py (100%) src/_pytest/python.py (95.7%): Missing lines 1519 testing/test_mark.py (64.1%): Missing lines 967,984,989,992-993,995,997-1000,1014-1015,1017-1018 ------------- Total: 72 lines Missing: 15 lines Coverage: 79%

bluetech · 2020-04-10T09:18:50Z

src/_pytest/nodes.py


-        #: the marker objects belonging to this node
-        self.own_markers = []  # type: List[Mark]
+        #: The (manually added) marks belonging to this node (start, end).


Having two separate lists might be good, but also not really needed maybe

Yes, looking a bit more I can see either way.

Any suggestions for a better/clearer comment?

First I think the name _own_markers is a little misleading now: the established meaning of own_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 the add_marker() ones.

So maybe _manually_added_markers?

As regards to the comment, maybe something like this?

Marks that are manually added to this node with `add_marker()`. These marks are included in the node's `own_markers`. Marks in the first item are prepended, and marks in the second item are appended.

bluetech · 2020-04-10T09:27:00Z

src/_pytest/nodes.py


+    @property
+    def own_markers(self) -> List[Mark]:
+        """The marker objects belonging to this node."""


@blueyed

I would extend the comment here. Suggestion (but please verify what I write!):

The marks directly belonging to this node. This includes marks manually added with `add_marker()`, and marks directly applied to the object (using `@pytest.mark.foo` decorations for example). It does not include marks inherited from parent nodes (for example, a mark applied to a class node is not included in a method's `own_markers`).

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?

@RonnyPfannschmidt

changing own_markers from a real object to a transient temporal property is breaking api change

Do you think changing own_markers directly is something that should be supported, or deprecated?

bluetech · 2020-04-10T09:30:40Z

src/_pytest/python.py

+    @property
+    def own_markers(self) -> List[Mark]:
+        if self._obj_markers is None:
+            self._obj_markers = get_unpacked_marks(self.obj)


Would it be too expensive to do this eagerly (when obj is set/changed) as opposed to lazily as in here?

bluetech · 2020-04-10T09:37:38Z

src/_pytest/python.py


+    @property
+    def own_markers(self) -> List[Mark]:
+        # Do not include markers from obj, coming from Class already.


I guess storing a mark directly on an instance (instance.pytestmark = [...]) is not supported/will be ignored?

RonnyPfannschmidt · 2020-05-06T13:18:41Z

closing as unresolved

Turn own_markers into a property

5b699df

This makes it a bit more easier to follow, especially with the removed hack of setting it only based on `_ALLOW_MARKERS` property. This was triggered based on wondering why setting `_obj` directly would not set the marks.

blueyed mentioned this pull request Apr 9, 2020

Factor out own_markers and keywords into properties blueyed/pytest#359

Open

blueyed changed the title ~~Factor out own_markers and keywords into properties~~ Factor out own_markers into a property Apr 9, 2020

bluetech reviewed Apr 9, 2020

View reviewed changes

RonnyPfannschmidt requested changes Apr 9, 2020

View reviewed changes

blueyed added 2 commits April 9, 2020 16:19

fixup! Turn own_markers into a property

b96b13c

test, reset in obj

2cadcfd

The test is not including keywords for now (in the end, #359)

blueyed commented Apr 9, 2020

View reviewed changes

blueyed force-pushed the own_markers-upstream branch from 6fe2599 to 4a89494 Compare April 9, 2020 19:12

blueyed mentioned this pull request Apr 9, 2020

fix support for obj parameter of Class.from_parent #7063

Open

back out typing for obj

19ddf9a

bluetech reviewed Apr 10, 2020

View reviewed changes

RonnyPfannschmidt closed this May 6, 2020

Uh oh!

Factor out own_markers into a property #7052

Factor out own_markers into a property #7052

Uh oh!

Conversation

blueyed commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluetech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

blueyed Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RonnyPfannschmidt commented Apr 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RonnyPfannschmidt commented May 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Factor out `own_markers` into a property #7052

Factor out `own_markers` into a property #7052

blueyed commented Apr 9, 2020 •

edited

Loading

blueyed Apr 9, 2020 •

edited

Loading