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

Override marks on items? #3501

Closed
nicoddemus opened this issue May 24, 2018 · 11 comments
Closed

Override marks on items? #3501

nicoddemus opened this issue May 24, 2018 · 11 comments
Labels
topic: marks related to marks, either the general marks or builtin type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog type: bug problem that needs to be addressed

Comments

@nicoddemus
Copy link
Member

nicoddemus commented May 24, 2018

We have a lot of C++ code which runs with our Python tests. Running tests in debug mode is significantly slower than running in release, so we have a pytest_collection_modifyitems hook that goes over all items and increases their timeout (we use pytest-timeout) if in debug mode.

The hook basically gets the current timeout marker (if any), multiplies its timeout value by 3 if in debug mode, and sets a new marker in the node with the new timeout value.

Here's a test which in essence does same as the hook:

import pytest

@pytest.mark.timeout(1)
def test_replace_mark(request):
    m = request.node.get_closest_marker('timeout')
    value = m.args[0]
    request.node.add_marker(pytest.mark.timeout(value * 3))  # 3 is the "debug factor"
    assert request.node.get_closest_marker('timeout').args[0] == 3

This fails because args[0] == 1, which is the value of the @pytest.mark.timeout(1) applied directly to the test. I suppose get_closest_marker looks at the first marker on the own_markers list, which makes sense.

If I explicitly insert the marker to the front of own_markers then the test works:

request.node.own_markers.insert(0, pytest.mark.timeout(value * 3))

But we need to realize that pytest-timeout still uses the old Node.get_marker API. So if we change the assert to:

assert request.node.get_marker('timeout').args == 3

This fails with:

__________________________________________________ test_replace_mark __________________________________________________

request = <FixtureRequest for <Function 'test_replace_mark'>>

    @pytest.mark.timeout(1)
    def test_replace_mark(request):
        m = request.node.get_closest_marker('timeout')
        value = m.args[0]
        request.node.own_markers.insert(0, pytest.mark.timeout(value * 3))
>       assert request.node.get_marker('timeout').args == 3

python\ben10\_tests\test_bar.py:8: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
..\..\..\envs\souring20-py27\lib\site-packages\_pytest\nodes.py:227: in get_marker
    return MarkInfo(markers)
<attrs generated init ef7829adcefadba2ccaf594dbf1e1b0360ba130a>:6: in __init__
    self.combined = __attr_factory_combined(self)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = MarkInfo(_marks=[MarkDecorator(mark=Mark(name='timeout', args=(3,), kwargs={})), Mark(name='timeout', args=(1,), kwargs={})])

>   default=attr.Factory(lambda self: reduce(Mark.combined_with, self._marks),
                         takes_self=True))
E                        TypeError: unbound method combined_with() must be called with Mark instance as first argument (got MarkDecorator instance instead)

..\..\..\envs\souring20-py27\lib\site-packages\_pytest\mark\structures.py:287: TypeError
============================================== 1 failed in 0.30 seconds ===============================================

I can fix that by inserting a Mark object explicitly:

request.node.own_markers.insert(0, Mark(name='timeout', args=(value * 3,), kwargs={}))

That fixes the test, but requires to import _pytest.mark.structures.Mark.

All in all this exposes how tricky the old implementation/API were, but I'm bringing this up for discussion because it is not very clear how overwrite or add a new marker with precedence over the others.

@pytestbot pytestbot added the topic: marks related to marks, either the general marks or builtin label May 24, 2018
@pytestbot
Copy link
Contributor

GitMate.io thinks possibly related issues are #2872 (mark fixtures ), #222 (mark expressions break for items that dont have a obj property), #3462 (A question on how marks work), #50 (Select tests according to their mark), and #814 (Mark for flaky tests).

@RonnyPfannschmidt
Copy link
Member

your usage adds markdecorators into places where marks are, add_marker correctly appends a actual mark object

insertion a point zero is technically always wrong

the marker object is available as mark property on the markdecorator

@RonnyPfannschmidt
Copy link
Member

now however you exposed that get_closest_marker is not entirely correct, it should get the last marker first

so your added one should be closer than the original one

@RonnyPfannschmidt
Copy link
Member

since we didn't introduce get_closest_marker as experiment we also cant fix it without doing an api breaking change ^^

aint this a nice bill

@pytestbot pytestbot added the type: bug problem that needs to be addressed label May 24, 2018
@RonnyPfannschmidt RonnyPfannschmidt added the type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog label May 24, 2018
@RonnyPfannschmidt
Copy link
Member

@nicoddemus i just noted i wasnt clear enough - you can add the mark object via request.node.add_marker(pytest.mark.timeout(value * 3).mark)

@nicoddemus
Copy link
Member Author

Hmm I see, thanks!

Worth noting that this is an API breakage, because request.node.add_marker(pytest.mark.timeout(value * 3)) worked previously. Can we make the previous behavior work again?

@RonnyPfannschmidt
Copy link
Member

@nicoddemus yes, i#ll take a look in the afternoon

@nicoddemus
Copy link
Member Author

nicoddemus commented Jun 12, 2018

Hmm wait, I mis-remembered this issue: request.node.add_marker(pytest.mark.timeout(value * 3)) works.

Weird, I was trying the example again and it was not generating an error. Then I realized I was in Python 3. Switching to Python 2 then I get the error.

To recapitulate:

import pytest

@pytest.mark.timeout(1)
def test_replace_mark(request):
    m = request.node.get_closest_marker('timeout')
    value = m.args[0]
    request.node.own_markers.insert(0, pytest.mark.timeout(value * 3))
    assert request.node.get_marker('timeout').args[0] == 3

Python 2

______________________________ test_replace_mark ______________________________

request = <FixtureRequest for <Function 'test_replace_mark'>>

    @pytest.mark.timeout(1)
    def test_replace_mark(request):
        m = request.node.get_closest_marker('timeout')
        value = m.args[0]
        request.node.own_markers.insert(0, pytest.mark.timeout(value * 3))
>       assert request.node.get_marker('timeout').args[0] == 3

test_m.py:8:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
pytest\_pytest\nodes.py:227: in get_marker
    return MarkInfo(markers)
<attrs generated init 212d4904f0b7f289f41d7ec8806a17bf5398ea82>:6: in __init__
    self.combined = __attr_factory_combined(self)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = MarkInfo(_marks=[MarkDecorator(mark=Mark(name='timeout', args=(3,), kwargs={})), Mark(name='timeout', args=(1,), kwargs={})])

>   default=attr.Factory(lambda self: reduce(Mark.combined_with, self._marks),
                         takes_self=True))
E                        TypeError: unbound method combined_with() must be called with Mark instance as first argument (got MarkDecorator instance instead)

pytest\_pytest\mark\structures.py:287: TypeError
========================== 1 failed in 0.07 seconds ===========================

Python 3

test_m.py .                                                              [100%]

========================== 1 passed in 0.02 seconds ===========================

@RonnyPfannschmidt
Copy link
Member

@nicoddemus the error doesnt happen on python3 due to disappearance of bound methods - if marks get required to be marks for markinfo, it will fail earlier

@RonnyPfannschmidt
Copy link
Member

@nicoddemus i got a fix - preparing a pr

@RonnyPfannschmidt
Copy link
Member

closed by #3576

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: marks related to marks, either the general marks or builtin type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog type: bug problem that needs to be addressed
Projects
None yet
3 participants