Node.get_marker needs a proper replacement

[The-Compiler]

Continuing the discussion from #3317 (especially #3317 (comment)) here - cc @nicoddemus @RonnyPfannschmidt @flub

Current situation

I'm trying the 3.6.0 release (#3445), here's what I see with -Wall:

pytest-rerunfailures breaks

INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   [...]
INTERNALERROR>   File ".../lib/python3.6/site-packages/pytest_rerunfailures.py", line 123, in pytest_runtest_protocol
INTERNALERROR>     reruns = get_reruns_count(item)
INTERNALERROR>   File ".../lib/python3.6/site-packages/pytest_rerunfailures.py", line 80, in get_reruns_count
INTERNALERROR>     if "reruns" in rerun_marker.kwargs:
INTERNALERROR>   File ".../lib/python3.6/site-packages/_pytest/mark/structures.py", line 20, in warned
INTERNALERROR>     warnings.warn(warning, stacklevel=2)
INTERNALERROR> _pytest.deprecated.RemovedInPytest4Warning: MarkInfo objects are deprecated as they contain the merged marks.
INTERNALERROR> Please use node.iter_markers to iterate over markers correctly

pytest-qt breaks

    def pytest_runtest_setup(self, item):
        if item.get_marker('no_qt_log'):
            return
        m = item.get_marker('qt_log_ignore')
        if m:
>           if not set(m.kwargs).issubset(set(['extend'])):

local things in my testsuite break

xfail = request.node.get_marker('xfail')
if xfail and (not xfail.args or xfail.args[0]):
    ...

    @pytest.fixture(autouse=True)
    def apply_fake_os(monkeypatch, request):
        fake_os = request.node.get_marker('fake_os')
        if not fake_os:
            return
    
>       name = fake_os.args[0]
E       _pytest.deprecated.RemovedInPytest4Warning: MarkInfo objects are deprecated as they contain the merged marks.
E       Please use node.iter_markers to iterate over markers correctly

Problems

Insufficient documentation

So, the deprecation warning tells me to use node.iter_markers. Looking at the docs, all I can see is "iterate over all markers of the node" and for get_marker(name) a "warning: deprecated" (with wrong ReST-syntax, too).

If I'm not aware of the problem of marker smearing (and I wasn't really, I've never done strange subclassing stuff in tests), I'm quite lost there. If it wasn't for @nicoddemus' example I would be pretty lost here, and write something based on guesswork.

Bad abstraction

The suggested alternative to

fake_os = request.node.get_marker('fake_os')
if not fake_os:
    return
    
name = fake_os.args[0]
...

Is supposedly:

for fake_os in request.node.iter_markers():
    if fake_os.name == 'fake_os':
        break
else:
    return

name = fake_os.args[0]
...

With that kind of ugly construct, I can guarantee you: People will make mistakes (or plugins will handle it differently) and you'll end up with more issues regarding marker handling than before. Suddenly, only some markers will smear, others will have other funny issues (what if I forget the else: for the for), and so on.

We want the default case to be that only the closest marker applies, right? So get an equivalent API which does that, and document that's the replacement for the "default case" where you don't need to worry about those issues at all.

Conclusion

I feel like this should block a 3.6.0 release. If we release something with deprecation warnings, no replacement, and bad documentation, then plugins and testsuites which want to preserve backwards compatibility will end up with three ways of getting a marker, each probably different in another subtle way.

[RonnyPfannschmidt]

well consider yourself lucky,

pytest-retunfailures is basically broken based on the code you posted, so its lucky we can fix it now,
same goes for pytest-qt

welcome to #568 - markers stain on things

in addition the example you posted is likely incorrect as well, as its absolutely unclear which mark from which level should take priority

in the code you posted a mark on the class/module level would always be prioritized over a function scope one (and users most likely want it the other way around)

[RonnyPfannschmidt]

the main reason there is no marks_by name is, that it would also be incomplete and half-wrong - the api was iterate on in the marks pr and thrown out for lack of clarity in details

[The-Compiler]

Note that that example was from @nicoddemus - which reinforces my point pretty well, this is difficult to get right. This is exactly why pytest should solve this problem and not everyone using it.

[RonnyPfannschmidt]

@The-Compiler the main problem is that we have no clear idea on how to solve this right either

[nicoddemus]

@The-Compiler is absolutely right that we should nail down the documentation before releasing 3.6 (this is one of the points I made during the review).

I'm confused:

in addition the example you posted is likely incorrect as well, as its absolutely unclear which mark from which level should take priority

The "closest" mark is not what we want in this case?

[nicoddemus]

Guys, gentle ping. This is blocking the 3.6 release so we need to reach a consensus soon.

[RonnyPfannschmidt]

i propose a require_name argument to iter_markers, allowing to filter by name

i can implement this evening or tommorow

[nicoddemus]

We can't update Node.get_marker to return the closest marker because currently it returns the "merged" markers for backward compatibility, correct?

i propose a require_name argument to iter_markers, allowing to filter by name

(Little bickshedding: I think calling the parameter just name is sufficient)

If we add this new parameter to iter_markers, how the example below should change to use the new API?

        fake_os = request.node.get_marker('fake_os')
        if not fake_os:
            return
    
        name = fake_os.args[0]

What if instead we add a new get_closest_marker(name) function, which returns the closest marker to the node with the given name, or None if not found? This would provide a more direct replacement of current get_marker(name) usages.

[The-Compiler]

I agree that a get_closest_marker(name) is most likely what most code using marks would want, i.e. the "default" case.

[flub]

On Mon 07 May 2018 at 11:37 +0000, Bruno Oliveira wrote: Guys, gentle ping. This is blocking the 3.6 release so we need to reach a consensus soon.

I'd be somewhat careful not to try and do this too hurriedly. It's bad to break things for users. And really bad if we break things for indirect users (a user of a plugin really doesn't care, they just want their tests to keep running).

[flub]

Thanks for the thorough feedback Florian!

On Thu 03 May 2018 at 22:38 -0700, Florian Bruhin wrote: Continuing the discussion from #3317 (especially #3317 (comment)) here - cc @nicoddemus @RonnyPfannschmidt @flub # Current situation I'm trying the 3.6.0 release (#3445), here's what I see with `-Wall`: ## `pytest-rerunfailures` breaks ```python INTERNALERROR> Traceback (most recent call last): INTERNALERROR> [...] INTERNALERROR> File ".../lib/python3.6/site-packages/pytest_rerunfailures.py", line 123, in pytest_runtest_protocol INTERNALERROR> reruns = get_reruns_count(item) INTERNALERROR> File ".../lib/python3.6/site-packages/pytest_rerunfailures.py", line 80, in get_reruns_count INTERNALERROR> if "reruns" in rerun_marker.kwargs: INTERNALERROR> File ".../lib/python3.6/site-packages/_pytest/mark/structures.py", line 20, in warned INTERNALERROR> warnings.warn(warning, stacklevel=2) INTERNALERROR> _pytest.deprecated.RemovedInPytest4Warning: MarkInfo objects are deprecated as they contain the merged marks. INTERNALERROR> Please use node.iter_markers to iterate over markers correctly ```

We crash plugins because we want to emit a warning? Is this the intention? This seems not great to me.

With that kind of ugly construct, I can guarantee you: People will make mistakes (or plugins will handle it differently) and you'll end up with *more* issues regarding marker handling than before. Suddenly, only *some* markers will smear, others will have other funny issues (what if I forget the `else:` for the `for`), and so on.

You're right. We should aim for a direct replacement for get_marker.

We want the default case to be that only the closest marker applies, right? So get an equivalent API which does that, and document that's the replacement for the "default case" where you don't need to worry about those issues at all.

+1

[The-Compiler]

We crash plugins because we want to emit a warning? Is this the intention? This seems not great to me.

Only for testsuites using -Werror (or --pythonwarnings error) - not -Wall like I said above, sorry. Still, IMHO it's a good idea to turn all warnings into errors in testsuites, so you actually notice deprecation issues. My main point is that we should have a proper replacement in the same release we deprecate the old way.