Mark expose nontransfered marks via pytestmark property #2517

RonnyPfannschmidt · 2017-06-22T08:02:38Z

will fix #2516

coveralls · 2017-06-22T08:41:17Z

Coverage increased (+0.0009%) to 92.133% when pulling bdec2c8 on RonnyPfannschmidt:mark-expose-nontransfered into 9bd8907 on pytest-dev:features.

fixes pytest-dev#2516

RonnyPfannschmidt · 2017-06-22T08:52:53Z

@petr-balogh please take a look if this addition can help your use-case

coveralls · 2017-06-22T13:49:03Z

Coverage increased (+0.01%) to 92.142% when pulling 1d92601 on RonnyPfannschmidt:mark-expose-nontransfered into 9bd8907 on pytest-dev:features.

petr-balogh · 2017-06-22T15:22:48Z

@RonnyPfannschmidt Cool, seems that now I can access everything I need and affect collecting tests exactly how I need. When do you thing this change could be merged and released?

Content of: conftest.py

import subprocess                                                                                                    
                                                                                                                     
COLUMNS = int(subprocess.check_output(['stty', 'size']).split()[1])                                                  
                                                                                                                     
                                                                                                                     
def pytest_collection_modifyitems(session, config, items):                                                           
    for item in items:                                                                                               
        print str(item).center(COLUMNS, "#")                                                                         
        g = item.get_marker("glob")                                                                                  
                                                                                                                     
        pytestmark = getattr(item.function, 'pytestmark', None)                                                      
        print ("Marker got from item.get_marker('glob') is %s" % g)                                                  
        if pytestmark:                                                                                               
            print (                                                                                                  
                "pytestmark from item.pytestmark of item: %s is: %s" %                                               
                (item, pytestmark)                                                                                   
            )                                                                                                        
        if g is not None:                                                                                            
            for info in g:                                                                                           
                print ("Glob args: %s, kwargs: %s" % (info.args, info.kwargs))

Content of: test_file.py

import pytest

glob = pytest.mark.glob
tier1 = pytest.mark.tier1
tier2 = pytest.mark.tier2
tier3 = pytest.mark.tier3

@tier3
@glob("class", tier=3)
class ParentClass(object):
    __test__ = False

@tier2
@glob("class", tier=2)
class TestClass(ParentClass):
    __test__ = True
    @tier1
    @glob("method", tier=1)
    def test_demo1(self):
        print "Info test 1"

    def test_demo2(self):                                                                                            
        print "Info test 2"

Output of pytest: py.test --capture=no ./test_file.py

================================================ test session starts ================================================
platform linux2 -- Python 2.7.11, pytest-3.1.3.dev29+g54876a9, py-1.4.34, pluggy-0.4.0
rootdir: ~/sandbox/pytest/pytest_issue, inifile:
plugins: timeout-1.2.0
collected 2 items 
###############################################<Function 'test_demo1'>###############################################
Marker got from item.get_marker('glob') is <MarkInfo Mark(name='glob', args=('method', 'class', 'class'), kwargs={'tier': 2})>
pytestmark from item.pytestmark of item: <Function 'test_demo1'> is: [Mark(name='glob', args=('method',), kwargs={'tier': 1}), Mark(name='tier1', args=(), kwargs={})]
Glob args: ('method',), kwargs: {'tier': 1}
Glob args: ('class',), kwargs: {'tier': 3}
Glob args: ('class',), kwargs: {'tier': 2}
###############################################<Function 'test_demo2'>###############################################
Marker got from item.get_marker('glob') is <MarkInfo Mark(name='glob', args=('class', 'class'), kwargs={'tier': 2})>
Glob args: ('class',), kwargs: {'tier': 3}
Glob args: ('class',), kwargs: {'tier': 2}

test_file.py Info test 1
.Info test 2

Thank you very much

petr-balogh · 2017-06-22T15:28:49Z

_pytest/python.py

@@ -23,7 +23,7 @@
    safe_str, getlocation, enum,
 )
 from _pytest.runner import fail
-
+from _pytest.mark import transfer_markers


maybe just empty line after imports?

nicoddemus

Thanks a ton for your never ending quest for improving our marks code! 👍 Of course you know the other devs and I really appreciate it!

Overall the change is good but I left some comments that I would us to discuss before moving this forward.

nicoddemus · 2017-06-22T23:22:11Z

_pytest/mark.py

                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,


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

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?

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

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. 👍

(sorry for all the internal rambling)

nicoddemus · 2017-06-22T23:27:31Z

_pytest/mark.py

@@ -329,30 +336,40 @@ def __call__(self, *args, **kwargs):
            is_class = inspect.isclass(func)
            if len(args) == 1 and (istestfunc(func) or is_class):


Suggestion to improve readability here:

move istestfunc(func) to a is_test_func local variable

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

nicoddemus · 2017-06-22T23:29:08Z

_pytest/mark.py

+def apply_mark(obj, mark):
+    assert isinstance(mark, Mark), mark
+    """applies a marker to an object,
+    makrer transfers only update legacy markinfo objects


typo: marker

nicoddemus · 2017-06-22T23:30:41Z

_pytest/mark.py

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


Typo: marminfo -> MarkInfo

nicoddemus · 2017-06-22T23:31:20Z

_pytest/mark.py

+def transfer_markers(funcobj, cls, mod):
+    """
+    transfer legacy markers to the function level marminfo objects
+    this one is a major fsckup for mark breakages


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

nicoddemus · 2017-06-23T00:23:05Z

changelog/2516.feature

@@ -0,0 +1 @@
+store unmeshed marks on functions pytestmark attribute


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.

nicoddemus · 2017-06-23T00:25:58Z

_pytest/deprecated.py

@@ -7,6 +7,11 @@
 """
 from __future__ import absolute_import, division, print_function

+
+class RemovedInPytest4_0Warning(DeprecationWarning):


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

nicoddemus · 2017-06-23T00:26:22Z

_pytest/deprecated.py

@@ -7,6 +7,11 @@
 """
 from __future__ import absolute_import, division, print_function

+
+class RemovedInPytest4_0Warning(DeprecationWarning):
+    "warning class for features removed in pytest 4.0"


Minor nitpick, please use triple strings for docstrings. 😁

nicoddemus · 2017-06-23T00:27:44Z

_pytest/deprecated.py

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


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.

there is no new code for this yet ^^

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?

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

* enhance api for fetching marks off an object * rename functions for storing marks * enhance deprecation message for MarkInfo

coveralls · 2017-06-23T09:44:39Z

Coverage increased (+0.008%) to 92.14% when pulling b0b6c35 on RonnyPfannschmidt:mark-expose-nontransfered into 9bd8907 on pytest-dev:features.

coveralls · 2017-06-23T10:13:23Z

Coverage increased (+0.008%) to 92.14% when pulling b0b6c35 on RonnyPfannschmidt:mark-expose-nontransfered into 9bd8907 on pytest-dev:features.

coveralls · 2017-06-23T10:44:13Z

Coverage increased (+0.008%) to 92.14% when pulling 8d5f287 on RonnyPfannschmidt:mark-expose-nontransfered into 9bd8907 on pytest-dev:features.

petr-balogh

+1

petr-balogh · 2017-06-23T13:36:54Z

May I ask you if I can expect this change in 3.1.3 release? Thanks

nicoddemus · 2017-06-23T13:39:16Z

@petr-balogh that will have to go into 3.2.0 since it is a new feature: test functions now have a pytestmark attribute.

RonnyPfannschmidt · 2017-06-23T14:48:12Z

@petr-balogh this will be part of the 3.2 release

move marker transfer to _pytest.mark

bdec2c8

RonnyPfannschmidt changed the base branch from master to features June 22, 2017 08:02

RonnyPfannschmidt added 3 commits June 22, 2017 10:41

extract application of marks and legacy markinfos

64ae6ae

store pristine marks on function.pytestmark

19b12b2

fixes pytest-dev#2516

changelog addition

c791895

RonnyPfannschmidt changed the title ~~[WIP] Mark expose nontransfered marks via pytestmark property~~ Mark expose nontransfered marks via pytestmark property Jun 22, 2017

RonnyPfannschmidt requested a review from nicoddemus June 22, 2017 10:50

pytest-dev deleted a comment from coveralls Jun 22, 2017

RonnyPfannschmidt changed the title ~~Mark expose nontransfered marks via pytestmark property~~ [WIP] Mark expose nontransfered marks via pytestmark property Jun 22, 2017

add deprecation warnings for using markinfo attributes

1d92601

RonnyPfannschmidt changed the title ~~[WIP] Mark expose nontransfered marks via pytestmark property~~ Mark expose nontransfered marks via pytestmark property Jun 22, 2017

petr-balogh reviewed Jun 22, 2017

View reviewed changes

nicoddemus requested changes Jun 23, 2017

View reviewed changes

RonnyPfannschmidt added 2 commits June 23, 2017 11:05

address review comments

23d016f

* enhance api for fetching marks off an object * rename functions for storing marks * enhance deprecation message for MarkInfo

fixup changelog, thanks Bruno

b0b6c35

minor code style fix

8d5f287

nicoddemus approved these changes Jun 23, 2017

View reviewed changes

petr-balogh approved these changes Jun 23, 2017

View reviewed changes

nicoddemus merged commit bab18e1 into pytest-dev:features Jun 23, 2017

nicoddemus mentioned this pull request Jun 23, 2017

expose unaltered pristine pytest marks on function.pytestmark #2516

Closed

RonnyPfannschmidt mentioned this pull request Jun 23, 2017

Replace internal warnings system by standard warnings and use them to make deprecations visible and later enforced #2452

Closed

The-Compiler mentioned this pull request Jul 18, 2017

Revert "introduce deprecation warnings for legacy parametersets" #2584

Closed

RonnyPfannschmidt deleted the mark-expose-nontransfered branch November 13, 2017 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark expose nontransfered marks via pytestmark property #2517

Mark expose nontransfered marks via pytestmark property #2517

RonnyPfannschmidt commented Jun 22, 2017

coveralls commented Jun 22, 2017

RonnyPfannschmidt commented Jun 22, 2017

coveralls commented Jun 22, 2017

petr-balogh commented Jun 22, 2017

petr-balogh Jun 22, 2017

nicoddemus left a comment •

edited

Loading

nicoddemus Jun 22, 2017

nicoddemus Jun 23, 2017

RonnyPfannschmidt Jun 23, 2017

nicoddemus Jun 23, 2017

nicoddemus Jun 23, 2017

nicoddemus Jun 22, 2017

nicoddemus Jun 22, 2017

nicoddemus Jun 22, 2017

nicoddemus Jun 22, 2017

nicoddemus Jun 23, 2017

nicoddemus Jun 23, 2017

nicoddemus Jun 23, 2017

nicoddemus Jun 23, 2017 •

edited

Loading

RonnyPfannschmidt Jun 23, 2017

nicoddemus Jun 23, 2017

RonnyPfannschmidt Jun 23, 2017

coveralls commented Jun 23, 2017

coveralls commented Jun 23, 2017

coveralls commented Jun 23, 2017

petr-balogh left a comment

petr-balogh commented Jun 23, 2017

nicoddemus commented Jun 23, 2017

RonnyPfannschmidt commented Jun 23, 2017

		@@ -329,30 +336,40 @@ def __call__(self, args, *kwargs):
		is_class = inspect.isclass(func)
		if len(args) == 1 and (istestfunc(func) or is_class):

		@@ -0,0 +1 @@
		store unmeshed marks on functions pytestmark attribute

Mark expose nontransfered marks via pytestmark property #2517

Mark expose nontransfered marks via pytestmark property #2517

Conversation

RonnyPfannschmidt commented Jun 22, 2017

coveralls commented Jun 22, 2017

RonnyPfannschmidt commented Jun 22, 2017

coveralls commented Jun 22, 2017

petr-balogh commented Jun 22, 2017

Choose a reason for hiding this comment

nicoddemus left a comment • edited Loading

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

nicoddemus Jun 23, 2017 • edited Loading

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

coveralls commented Jun 23, 2017

coveralls commented Jun 23, 2017

coveralls commented Jun 23, 2017

petr-balogh left a comment

Choose a reason for hiding this comment

petr-balogh commented Jun 23, 2017

nicoddemus commented Jun 23, 2017

RonnyPfannschmidt commented Jun 23, 2017

nicoddemus left a comment •

edited

Loading

nicoddemus Jun 23, 2017 •

edited

Loading