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.

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

Mark expose nontransfered marks via pytestmark property #2517

Merged
Merged
8 changes: 8 additions & 0 deletions _pytest/deprecated.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@
"""
from __future__ import absolute_import, division, print_function


class RemovedInPytest4_0Warning(DeprecationWarning):
Copy link
Member

Choose a reason for hiding this comment

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

I think just RemovedPytest4Warning is enough, no need to add the _0 in my opinion.

"warning class for features removed in pytest 4.0"
Copy link
Member

Choose a reason for hiding this comment

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

Minor nitpick, please use triple strings for docstrings. 😁



MAIN_STR_ARGS = 'passing a string to pytest.main() is deprecated, ' \
'pass a list of arguments instead.'

Expand All @@ -22,3 +27,6 @@
GETFUNCARGVALUE = "use of getfuncargvalue is deprecated, use getfixturevalue"

RESULT_LOG = '--result-log is deprecated and scheduled for removal in pytest 4.0'

MARK_INFO_ATTRIBUTE = RemovedInPytest4_0Warning(
"Markinfo attributes are deprecated, please iterate the mark Collection")
Copy link
Member

@nicoddemus nicoddemus Jun 23, 2017

Choose a reason for hiding this comment

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

This should be documented on the docs, I think. Users who see this probably won't know what this means.

I also suggest to add a (see https://docs.pytest.org/...) pointing to the docs so users can easily go to it to know how to update their code.

Copy link
Member Author

Choose a reason for hiding this comment

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

there is no new code for this yet ^^

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean? Actually, now that I read it again, I'm not sure what you mean by "please iterate the mark Collection"... oh do you mean the marks in the collected item?

Copy link
Member Author

Choose a reason for hiding this comment

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

markinfo objects are iterables, atm the iteration yields markinfo objects as wel

@hpk42 i wonder if we can make it return the new mark type instead

88 changes: 67 additions & 21 deletions _pytest/mark.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,20 @@
from __future__ import absolute_import, division, print_function

import inspect
import warnings
from collections import namedtuple
from operator import attrgetter
from .compat import imap
from .deprecated import MARK_INFO_ATTRIBUTE

def alias(name, warning=None):
getter = attrgetter(name)

def alias(name):
return property(attrgetter(name), doc='alias for ' + name)
def warned(self):
warnings.warn(warning, stacklevel=2)
return getter(self)

return property(getter if warning is None else warned, doc='alias for ' + name)


class ParameterSet(namedtuple('ParameterSet', 'values, marks, id')):
Expand Down Expand Up @@ -329,30 +336,40 @@ def __call__(self, *args, **kwargs):
is_class = inspect.isclass(func)
if len(args) == 1 and (istestfunc(func) or is_class):
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion to improve readability here:

  1. move istestfunc(func) to a is_test_func local variable
  2. add a assert is_test_func as the first line of the else statement;

I find a good practice to always assert for a known condition in a else statement when the condition is not really obvious. This is more apparent when checking multiple possible paths:

if condition1:
   ...
elif condition2:
   ...
else:
   assert condition3
   ...

if is_class:
if hasattr(func, 'pytestmark'):
mark_list = func.pytestmark
if not isinstance(mark_list, list):
mark_list = [mark_list]
# always work on a copy to avoid updating pytestmark
# from a superclass by accident
mark_list = mark_list + [self]
func.pytestmark = mark_list
else:
func.pytestmark = [self]
apply_mark(func, self.mark)
else:
holder = getattr(func, self.name, None)
if holder is None:
holder = MarkInfo(self.mark)
setattr(func, self.name, holder)
else:
holder.add_mark(self.mark)
apply_legacy_mark(func, self.mark)
apply_mark(func, self.mark)
return func

mark = Mark(self.name, args, kwargs)
return self.__class__(self.mark.combined_with(mark))


def apply_mark(obj, mark):
assert isinstance(mark, Mark), mark
"""applies a marker to an object,
Copy link
Member

Choose a reason for hiding this comment

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

The docstring should be the first statement here (before the assert)

Copy link
Member

Choose a reason for hiding this comment

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

I had to review the PR, the code, the tests and use the debugger to finally get a picture of what's going on. 😅

OK IIUC apply_mark only sets or updates the pytestmark attribute of the given obj, while apply_legacy_mark actually adds/updates the MarkInfo instance with the mark name in obj.

Perhaps the docstring of both methods should describe exactly what's going on? Suggestions:

def apply_mark(obj, mark):
    """
    Sets or updates the ``pytestmark`` attribute on ``obj`` with the given ``mark`` object.

    In the future (see issue X), this is the only effect ``@pytest.mark`` will have, and the
    mark transfering from classes and modules to test functions will be done on 
    ``pytest.Item`` objects instead.
    """


def update_legacy_mark_info(obj, mark):
    """
    Updates or creates a ``MarkInfo`` as an attribute of the given object.

    This is how markers are transfered from classes and modules to test functions in the legacy way,
    which is planned to be phased out.
    """

Also, we are now introducing a public way to get "pristine" (non-inherited) markers on functions by using a machanism which is already used elsewhere, the pytestmark attribute. The pytestmark attribute in classes and modules has a different semantic in that they will transfer their marks to child test items.

I suggest we take this opportunity to introduce a proper public API which hides this detail, like pytest.get_pristine_marks:

def get_pristine_marks(obj):
    """
    Returns pytest marks applied directly to the given object, without any inheritance from
    parent classes decorated with ``@pytest.mark`` or parent modules with the ``pytestmark`` 
    attribute.
    """

Then we don't have to "promise" that we will be supporting a pytestmark attribute in function objects directly in the future. If this suggesiton sounds good, I also believe we should name the new pytestmark in functions to _pytest_marks to avoid users using it by accident.

On the other hand now that I think about it, pytestmark in modules have to be implemented like this given that you can't really decorate a module, so perhaps in the end pytestmark as the holder for pristine marks is the way to go anyway.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

pytestmark will stay, no need to change it

im against a get_pristine_marks function, - the marks api people use to get/interact with marks sould be an new object on item that we will provide

after all marks in and from parameterization are to be considered as well and those are a pain to get

Copy link
Member

Choose a reason for hiding this comment

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

im against a get_pristine_marks function, - the marks api people use to get/interact with marks sould be an new object on item that we will provide

I was actually suggesting using get_pristine_marks instead of pytestmark. Certainly accessing the merged marks will done through Item's API. But after I concluded later it probably won't make much sense given that for modules at least the pytestmark mechanism to declare marks will have to stay, so we might as well use the same for classes and functions as well. So let's move this forward as it is. 😁

Side topic: usually when things decorate user code, it might be a better to use a public API to manipulate what's being decorated instead of making the decorated attribute public. For example the mock package experienced user experience problems because you can add arbitrary attributes to Mock objects and use the same objects to use functionality, for example Mock().assert_called_once(). The problem with that is that it was common for users to mistype some name and it would silently do nothing (it would return a new Mock instance) as opposed to a failing error. This could have been better addressed I think by providing that functionality through an API instead through the object directly, for example mock.assert_called_once(m) (with mock here being the module and m the Mock object), which would prevent clashes and mistakes.

That was the general feeling that I had when proposing to use an API to access the list of unmerged marks: it would allow us to use an internal ugly name, possibly change this in the future to store that information somewhere without breaking user expectations, and even do some more work instead of just returning an internal attribute of the object.

But as I said, let's move forward with pytestmark then. 👍

Copy link
Member

Choose a reason for hiding this comment

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

(sorry for all the internal rambling)

makrer transfers only update legacy markinfo objects
Copy link
Member

Choose a reason for hiding this comment

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

typo: marker

"""
mark_list = getattr(obj, 'pytestmark', [])

if not isinstance(mark_list, list):
mark_list = [mark_list]
# always work on a copy to avoid updating pytestmark
# from a superclass by accident
mark_list = mark_list + [mark]
obj.pytestmark = mark_list


def apply_legacy_mark(func, mark):
if not isinstance(mark, Mark):
raise TypeError("got {mark!r} instead of a Mark".format(mark=mark))
holder = getattr(func, mark.name, None)
if holder is None:
holder = MarkInfo(mark)
setattr(func, mark.name, holder)
else:
holder.add_mark(mark)


class Mark(namedtuple('Mark', 'name, args, kwargs')):
Expand All @@ -371,9 +388,9 @@ def __init__(self, mark):
self.combined = mark
self._marks = [mark]

name = alias('combined.name')
args = alias('combined.args')
kwargs = alias('combined.kwargs')
name = alias('combined.name', warning=MARK_INFO_ATTRIBUTE)
args = alias('combined.args', warning=MARK_INFO_ATTRIBUTE)
kwargs = alias('combined.kwargs', warning=MARK_INFO_ATTRIBUTE)

def __repr__(self):
return "<MarkInfo {0!r}>".format(self.combined)
Expand All @@ -389,3 +406,32 @@ def __iter__(self):


MARK_GEN = MarkGenerator()


def _marked(func, mark):
""" Returns True if :func: is already marked with :mark:, False otherwise.
This can happen if marker is applied to class and the test file is
invoked more than once.
"""
try:
func_mark = getattr(func, mark.name)
except AttributeError:
return False
return mark.args == func_mark.args and mark.kwargs == func_mark.kwargs


def transfer_markers(funcobj, cls, mod):
"""
transfer legacy markers to the function level marminfo objects
Copy link
Member

Choose a reason for hiding this comment

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

Typo: marminfo -> MarkInfo

this one is a major fsckup for mark breakages
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to expand on this here; the details are scattered through multiple issues, here would be a good place to summarize them IMO

"""
for obj in (cls, mod):
mark_list = getattr(obj, 'pytestmark', [])

if not isinstance(mark_list, list):
mark_list = [mark_list]

for mark in mark_list:
mark = getattr(mark, 'mark', mark) # unpack MarkDecorator
if not _marked(funcobj, mark):
apply_legacy_mark(funcobj, mark)
31 changes: 1 addition & 30 deletions _pytest/python.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
safe_str, getlocation, enum,
)
from _pytest.runner import fail

from _pytest.mark import transfer_markers

Choose a reason for hiding this comment

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

maybe just empty line after imports?

cutdir1 = py.path.local(pluggy.__file__.rstrip("oc"))
cutdir2 = py.path.local(_pytest.__file__).dirpath()
cutdir3 = py.path.local(py.__file__).dirpath()
Expand Down Expand Up @@ -361,35 +361,6 @@ def _genfunctions(self, name, funcobj):
)


def _marked(func, mark):
""" Returns True if :func: is already marked with :mark:, False otherwise.
This can happen if marker is applied to class and the test file is
invoked more than once.
"""
try:
func_mark = getattr(func, mark.name)
except AttributeError:
return False
return mark.args == func_mark.args and mark.kwargs == func_mark.kwargs


def transfer_markers(funcobj, cls, mod):
# XXX this should rather be code in the mark plugin or the mark
# plugin should merge with the python plugin.
for holder in (cls, mod):
try:
pytestmark = holder.pytestmark
except AttributeError:
continue
if isinstance(pytestmark, list):
for mark in pytestmark:
if not _marked(funcobj, mark):
mark(funcobj)
else:
if not _marked(funcobj, pytestmark):
pytestmark(funcobj)


class Module(main.File, PyCollector):
""" Collector for test classes and functions. """

Expand Down
1 change: 1 addition & 0 deletions changelog/2516.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
store unmeshed marks on functions pytestmark attribute
Copy link
Member

Choose a reason for hiding this comment

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

This could be more descriptive to end users:

Now test function objects have a ``pytestmark`` attribute containing a list of marks applied directly to the test function, as opposed to marks inherited from parent classes or modules.

Also we should definitely document this properly somewhere on the docs and make it official part of the API.

24 changes: 23 additions & 1 deletion testing/test_mark.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import sys

import pytest
from _pytest.mark import MarkGenerator as Mark, ParameterSet
from _pytest.mark import MarkGenerator as Mark, ParameterSet, transfer_markers

class TestMark(object):
def test_markinfo_repr(self):
Expand Down Expand Up @@ -772,3 +772,25 @@ def assert_test_is_not_selected(keyword):
def test_parameterset_extractfrom(argval, expected):
extracted = ParameterSet.extract_from(argval)
assert extracted == expected


def test_legacy_transfer():

class FakeModule(object):
pytestmark = []

class FakeClass(object):
pytestmark = pytest.mark.nofun

@pytest.mark.fun
def fake_method(self):
pass


transfer_markers(fake_method, FakeClass, FakeModule)

# legacy marks transfer smeared
assert fake_method.nofun
assert fake_method.fun
# pristine marks dont transfer
assert fake_method.pytestmark == [pytest.mark.fun.mark]