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

refactor mark evaluators #2773

Merged
merged 7 commits into from
Oct 9, 2017
Merged

refactor mark evaluators #2773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
667e70f
switch out the placeholder MarkEvaluator in unittest plugin
RonnyPfannschmidt Sep 13, 2017
a336509
remove unused import
RonnyPfannschmidt Sep 13, 2017
9ad2b75
skipping: replace _evalskip with a more consistent _skipped_by_mark
RonnyPfannschmidt Sep 13, 2017
8480075
resuffle markevaluator internal structure
RonnyPfannschmidt Sep 14, 2017
e3b7368
flake8 fix
RonnyPfannschmidt Sep 15, 2017
459cc40
skipping: cleanup
RonnyPfannschmidt Sep 16, 2017
8a6bdb2
fix changelog entry
RonnyPfannschmidt Oct 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 57 additions & 44 deletions _pytest/skipping.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,22 +60,31 @@ def nop(*args, **kwargs):
)


class MarkEvaluator:
class MarkEvaluator(object):
def __init__(self, item, name):
self.item = item
self.name = name

@property
def holder(self):
return self.item.keywords.get(self.name)
self._marks = None
self._mark = None
self._mark_name = name

def __bool__(self):
return bool(self.holder)
self._marks = self._get_marks()
return bool(self._marks)
__nonzero__ = __bool__

def wasvalid(self):
return not hasattr(self, 'exc')

def _get_marks(self):

keyword = self.item.keywords.get(self._mark_name)
if isinstance(keyword, MarkDecorator):
return [keyword.mark]
elif isinstance(keyword, MarkInfo):
return [x.combined for x in keyword]
else:
return []

def invalidraise(self, exc):
raises = self.get('raises')
if not raises:
Expand All @@ -95,7 +104,7 @@ def istrue(self):
fail("Error evaluating %r expression\n"
" %s\n"
"%s"
% (self.name, self.expr, "\n".join(msg)),
% (self._mark_name, self.expr, "\n".join(msg)),
pytrace=False)

def _getglobals(self):
Expand All @@ -107,40 +116,45 @@ def _getglobals(self):
def _istrue(self):
if hasattr(self, 'result'):
return self.result
if self.holder:
if self.holder.args or 'condition' in self.holder.kwargs:
self.result = False
# "holder" might be a MarkInfo or a MarkDecorator; only
# MarkInfo keeps track of all parameters it received in an
# _arglist attribute
marks = getattr(self.holder, '_marks', None) \
or [self.holder.mark]
for _, args, kwargs in marks:
if 'condition' in kwargs:
args = (kwargs['condition'],)
for expr in args:
self._marks = self._get_marks()

if self._marks:
self.result = False
for mark in self._marks:
self._mark = mark
if 'condition' in mark.kwargs:
args = (mark.kwargs['condition'],)
else:
args = mark.args

for expr in args:
self.expr = expr
if isinstance(expr, six.string_types):
d = self._getglobals()
result = cached_eval(self.item.config, expr, d)
else:
if "reason" not in mark.kwargs:
# XXX better be checked at collection time
msg = "you need to specify reason=STRING " \
"when using booleans as conditions."
fail(msg)
result = bool(expr)
if result:
self.result = True
self.reason = mark.kwargs.get('reason', None)
self.expr = expr
if isinstance(expr, six.string_types):
d = self._getglobals()
result = cached_eval(self.item.config, expr, d)
else:
if "reason" not in kwargs:
# XXX better be checked at collection time
msg = "you need to specify reason=STRING " \
"when using booleans as conditions."
fail(msg)
result = bool(expr)
if result:
self.result = True
self.reason = kwargs.get('reason', None)
self.expr = expr
return self.result
else:
self.result = True
return getattr(self, 'result', False)
return self.result

if not args:
self.result = True
self.reason = mark.kwargs.get('reason', None)
return self.result
return False

def get(self, attr, default=None):
return self.holder.kwargs.get(attr, default)
if self._mark is None:
Copy link
Member

Choose a reason for hiding this comment

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

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").

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

And about this suggestion @RonnyPfannschmidt?

Copy link
Member Author

Choose a reason for hiding this comment

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

@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

return default
return self._mark.kwargs.get(attr, default)

def getexplanation(self):
expl = getattr(self, 'reason', None) or self.get('reason', None)
Expand All @@ -155,17 +169,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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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"

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

OK then, I respect your decision

skipif_info = item.keywords.get('skipif')
if isinstance(skipif_info, (MarkInfo, MarkDecorator)):
eval_skipif = MarkEvaluator(item, 'skipif')
if eval_skipif.istrue():
item._evalskip = eval_skipif
item._skipped_by_mark = True
skip(eval_skipif.getexplanation())

skip_info = item.keywords.get('skip')
if isinstance(skip_info, (MarkInfo, MarkDecorator)):
item._evalskip = True
item._skipped_by_mark = True
if 'reason' in skip_info.kwargs:
skip(skip_info.kwargs['reason'])
elif skip_info.args:
Expand Down Expand Up @@ -212,7 +226,6 @@ def pytest_runtest_makereport(item, call):
outcome = yield
rep = outcome.get_result()
evalxfail = getattr(item, '_evalxfail', None)
evalskip = getattr(item, '_evalskip', None)
# unitttest special case, see setting of _unexpectedsuccess
if hasattr(item, '_unexpectedsuccess') and rep.when == "call":
from _pytest.compat import _is_unittest_unexpected_success_a_failure
Expand Down Expand Up @@ -248,7 +261,7 @@ def pytest_runtest_makereport(item, call):
else:
rep.outcome = "passed"
rep.wasxfail = explanation
elif evalskip is not None and rep.skipped and type(rep.longrepr) is tuple:
elif item._skipped_by_mark and rep.skipped and type(rep.longrepr) is tuple:
# skipped by mark.skipif; change the location of the failure
# to point to the item definition, otherwise it will display
# the location of where the skip exception was raised within pytest
Expand Down
4 changes: 1 addition & 3 deletions _pytest/unittest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
from _pytest.config import hookimpl
from _pytest.outcomes import fail, skip, xfail
from _pytest.python import transfer_markers, Class, Module, Function
from _pytest.skipping import MarkEvaluator


def pytest_pycollect_makeitem(collector, name, obj):
Expand Down Expand Up @@ -134,8 +133,7 @@ def addSkip(self, testcase, reason):
try:
skip(reason)
except skip.Exception:
self._evalskip = MarkEvaluator(self, 'SkipTest')
self._evalskip.result = True
self._skipped_by_mark = True
self._addexcinfo(sys.exc_info())

def addExpectedFailure(self, testcase, rawexcinfo, reason=""):
Expand Down
1 change: 1 addition & 0 deletions changelog/2767.removal
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove the internal multi-typed attribute ``Node._evalskip`` and replace it with the boolean ``Node._skipped_by_mark``.
1 change: 1 addition & 0 deletions changelog/2767.trivial
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* remove unnecessary mark evaluator in unittest plugin