refactor mark evaluators #2773

RonnyPfannschmidt · 2017-09-13T10:19:43Z

No description provided.

The-Compiler · 2017-09-13T10:24:45Z

Wrong branch?

RonnyPfannschmidt · 2017-09-13T10:39:20Z

yup, thanks for the note

RonnyPfannschmidt · 2017-09-13T12:57:20Z

this pr will contain breakage to insane mark behaviour,
there will still be unclear behaviour afterward

mark evaluator operating on merged marks is a total and utterly incorrect mess, dissolving it breaks stuff
and leaves questions open

this one is entirely @hpk42 's fault

nicoddemus · 2017-09-13T23:01:43Z

mark evaluator operating on merged marks is a total and utterly incorrect mess, dissolving it breaks stuff
and leaves questions open

You mean that we have the "merged marks" where keywords contain all marks, but put together in a dict?

nicoddemus · 2017-09-13T22:43:12Z

changelog/2767.removal

@@ -0,0 +1,2 @@
+* remove the internal optional multi-typed attribute _evalskip


typo: replace

Also please use correct grammar and formatting so we show users a well formatted changelog. 😉

Also don't need to add a *, towncrier does it for us automatically.

Remove the internal multi-typed attribute ``Node._evalskip`` and replace it with the boolean ``Node._skipped_by_mark``.

@RonnyPfannschmidt I was thinking this whole mark refactoring deserves a blog post on pytest's blog, to explain the pain points of the current design, our attempts to fix it and how it might affect users. People will be much more understanding if they know the reasons of why their test suite broke.

The more I understand the underlying issues the more I think a blog post or article in the docs is needed. 😬

The changelog is still the same, what do you think about my suggestion above @RonnyPfannschmidt?

nicoddemus · 2017-09-13T22:46:33Z

_pytest/skipping.py

@@ -155,17 +155,17 @@ def getexplanation(self):
 @hookimpl(tryfirst=True)
 def pytest_runtest_setup(item):
    # Check if skip or skipif are specified as pytest marks
-
+    item._skipped_by_mark = False


Should we make this part of the public API? Often users want to know if a test has been skipped in some hook, and the only way to do it currently is using the awkward:

was_skipped = hasattr(item, "_evalskip") and item._evalskip.istrue()

We might as well make it item.skipped_by_mark.

While on that, it might also be useful to have the reason attached tn it. If so, perhaps we should use a frozen attrs instance which also contains the reason? This makes it possible to add more information to it in the future:

item.skip_outcome.skipped == True item.skip_outcome.reason == "skipped for some reason"

im not going to introduce a new api in a refactoring/fixup attempt,
thats for a follow-up - please open a issue for better communicating outcomes

I understand the sentiment, but by renaming it to _skipped_by_mark you will break code which currently looks at this attribute to see if a test was skipped (for example IDEs, see #1613). So IDE and plugin authors which use this to check if a test was skipped will now have to test for _evalskip and _skipped_by_mark to comply. And then when a new version with a public API comes around, we will have another way to check for that.

Just pointing out we will bring a lot of pain to those people. Perhaps we can brainstorm a little on how the "skip" plugin decorates items with skip information?

its a private attribute and stays a private attribute
so im not going to protect them too much from that use, thats what they should have CI for

OK then, I respect your decision

RonnyPfannschmidt · 2017-09-14T06:02:26Z

the markinfo objects that merge the args ad kwargs of marks generate a lot of bugs, but also create a implied behaviour (its why marks from different levels overide each other in part)

nicoddemus · 2017-09-14T23:17:07Z

_pytest/skipping.py

-        return self.item.keywords.get(self.name)
+        self._marks = None
+        self._mark = None
+        self._repr_name = name


Perhaps this should be self._mark_name? Seems clearer to me. "repr" is too close to Python's own __repr__ which might be confusing.

nicoddemus · 2017-09-14T23:22:50Z

_pytest/skipping.py

+
+        if self._marks:
+            self.result = False
+            # "holder" might be a MarkInfo or a MarkDecorator; only


This comment is no longer relevant it seems.

nicoddemus · 2017-09-14T23:25:01Z

_pytest/skipping.py

+            # MarkInfo keeps track of all parameters it received in an
+            # _arglist attribute
+            for mark in self._marks:
+                self._mark = mark


self._mark will point to the last mark in self._marks, seems a little arbitrary to set this here. Is this by design? Otherwise it should be set in self._get_marks() instead to make it explicit that this is intended and doesn't depend on _istrue being called.

it points to the mark that had the first true expression, this mess comes from merged mark and remembering epxressions, then using attributes from merged mrks

so if you had 2 marks with different expressions, and one had run=False, all would have run=False implied

nicoddemus · 2017-09-14T23:32:31Z

_pytest/skipping.py


    def get(self, attr, default=None):
-        return self.holder.kwargs.get(attr, default)
+        if self._mark is None:


Shouldn't this loop over all marks trying to get the attr (which is really a kwarg actually), returning default if no marks have it?

Can we rename it to find_kwarg instead for clarity btw? To actually understand what this does I had to add an assert 0 and run the tests to figure out where it is called (because you can't actually do a search for "get").

To exemplify:

def find_kwarg(self, kwarg, default=None): for mark in self._get_marks(): if kwarg in mark.kwargs: return mark.kwargs[kwarg] return default

With this we can even get rid of self._mark.

if one mark has run=True, and another one doesnt, we get wrong results, welcome to why marks are so horrific

I see thanks for the explanation.

In that case, I suggest we change this to return all the kwarg values, and then the caller has responsibility to decide what to do with them.

def get_kwarg_values(self, kwarg, default=None): return [mark.kwargs[kwarg] mark in self._get_marks() if kwarg in mark.kwargs]

I don't like how implicit this is. If you don't like this suggestion, at least I would rename self._mark to something clear albeit ugly like self._first_true_mark or something.

well, it started out as something much worse, i#ll try to whip it in shape more

And about this suggestion @RonnyPfannschmidt?

@nicoddemus that breaks current usage inside pytest at least

so in order to go there i'd prefer to keep that until we iterate it again

nicoddemus · 2017-09-14T23:38:14Z

_pytest/skipping.py

@@ -155,17 +155,17 @@ def getexplanation(self):
 @hookimpl(tryfirst=True)
 def pytest_runtest_setup(item):
    # Check if skip or skipif are specified as pytest marks
-
+    item._skipped_by_mark = False


I understand the sentiment, but by renaming it to _skipped_by_mark you will break code which currently looks at this attribute to see if a test was skipped (for example IDEs, see #1613). So IDE and plugin authors which use this to check if a test was skipped will now have to test for _evalskip and _skipped_by_mark to comply. And then when a new version with a public API comes around, we will have another way to check for that.

Just pointing out we will bring a lot of pain to those people. Perhaps we can brainstorm a little on how the "skip" plugin decorates items with skip information?

nicoddemus · 2017-09-14T23:40:34Z

changelog/2767.removal

@@ -0,0 +1,2 @@
+* remove the internal optional multi-typed attribute _evalskip


@RonnyPfannschmidt I was thinking this whole mark refactoring deserves a blog post on pytest's blog, to explain the pain points of the current design, our attempts to fix it and how it might affect users. People will be much more understanding if they know the reasons of why their test suite broke.

RonnyPfannschmidt · 2017-09-15T06:29:20Z

note that when this one is finished it should break test suites, but it will fix a few bugs

nicoddemus · 2017-09-16T21:35:29Z

@RonnyPfannschmidt argh sorry, I spoke with you that I made a few comments, but those were "Pending" here on GH and I realized my mistake just now. Well just published them, please take a look.

remove dead comments fix naming remove dead code

coveralls · 2017-10-06T09:06:51Z

Coverage decreased (-0.001%) to 92.501% when pulling 459cc40 on RonnyPfannschmidt:fix-markeval-2767 into e7a4d3d on pytest-dev:features.

coveralls · 2017-10-09T13:40:36Z

Coverage decreased (-0.001%) to 92.501% when pulling 8a6bdb2 on RonnyPfannschmidt:fix-markeval-2767 into e7a4d3d on pytest-dev:features.

RonnyPfannschmidt changed the base branch from master to features September 13, 2017 10:39

nicoddemus reviewed Sep 13, 2017

View reviewed changes

nicoddemus requested changes Sep 14, 2017

View reviewed changes

RonnyPfannschmidt changed the title ~~[wip] refactor mark evaluators~~ refactor mark evaluators Sep 15, 2017

RonnyPfannschmidt added 6 commits October 6, 2017 10:44

switch out the placeholder MarkEvaluator in unittest plugin

667e70f

remove unused import

a336509

skipping: replace _evalskip with a more consistent _skipped_by_mark

9ad2b75

resuffle markevaluator internal structure

8480075

flake8 fix

e3b7368

skipping: cleanup

459cc40

remove dead comments fix naming remove dead code

RonnyPfannschmidt force-pushed the fix-markeval-2767 branch from f86cd8d to 459cc40 Compare October 6, 2017 08:44

fix changelog entry

8a6bdb2

nicoddemus approved these changes Oct 9, 2017

View reviewed changes

nicoddemus merged commit 059455b into pytest-dev:features Oct 9, 2017

bilderbuchi mentioned this pull request Oct 24, 2017

refactor and unittest mark evaluator #2767

Closed

RonnyPfannschmidt deleted the fix-markeval-2767 branch November 13, 2017 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor mark evaluators #2773

refactor mark evaluators #2773

RonnyPfannschmidt commented Sep 13, 2017

The-Compiler commented Sep 13, 2017

RonnyPfannschmidt commented Sep 13, 2017

RonnyPfannschmidt commented Sep 13, 2017

nicoddemus commented Sep 13, 2017

nicoddemus Sep 13, 2017

nicoddemus Sep 14, 2017

nicoddemus Sep 15, 2017

nicoddemus Oct 6, 2017

nicoddemus Sep 13, 2017

nicoddemus Sep 13, 2017

RonnyPfannschmidt Sep 14, 2017

nicoddemus Sep 14, 2017

RonnyPfannschmidt Sep 15, 2017

nicoddemus Sep 15, 2017

RonnyPfannschmidt commented Sep 14, 2017

nicoddemus Sep 14, 2017

nicoddemus Sep 14, 2017

nicoddemus Sep 14, 2017

RonnyPfannschmidt Sep 15, 2017

nicoddemus Sep 15, 2017

nicoddemus Sep 14, 2017

nicoddemus Sep 14, 2017

RonnyPfannschmidt Sep 15, 2017

nicoddemus Sep 15, 2017

RonnyPfannschmidt Sep 17, 2017

nicoddemus Oct 6, 2017

RonnyPfannschmidt Oct 9, 2017

nicoddemus Sep 14, 2017

nicoddemus Sep 14, 2017

RonnyPfannschmidt commented Sep 15, 2017

nicoddemus commented Sep 16, 2017

coveralls commented Oct 6, 2017

coveralls commented Oct 9, 2017

		@@ -0,0 +1,2 @@
		* remove the internal optional multi-typed attribute _evalskip

refactor mark evaluators #2773

refactor mark evaluators #2773

Conversation

RonnyPfannschmidt commented Sep 13, 2017

The-Compiler commented Sep 13, 2017

RonnyPfannschmidt commented Sep 13, 2017

RonnyPfannschmidt commented Sep 13, 2017

nicoddemus commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonnyPfannschmidt commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonnyPfannschmidt commented Sep 15, 2017

nicoddemus commented Sep 16, 2017

coveralls commented Oct 6, 2017

coveralls commented Oct 9, 2017