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.

[flub]

On Mon 07 May 2018 at 11:50 +0000, Bruno Oliveira wrote: 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? ```python 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.

What would happen if we just deprecate having multiple markers of the same name which end up on one node at all? Because everything that's being discussed here so far does not address the part of the feeback that no two plugins will ever handle this situation in the same way. It rightfully is the task of the API pytest provides to do this. If we do that then maybe even keeping .get_marker() is a sane option? How different are the new MarkInfo objects?

[nicoddemus]

What would happen if we just deprecate having multiple markers of the
same name which end up on one node at all?

IIUC that's what was done, Node.get_marker returns the "merged" markers and is now deprecated.

If we do that then maybe even keeping .get_marker() is a sane option?
How different are the new MarkInfo objects?

I think @RonnyPfannschmidt knows that from the top of his head and can answer more easily. 😁

[nicoddemus]

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

Oh definitely. Sorry if it came across that way, I just meant to point out that this is a release blocker so we can't postpone it indefinitely. But you are right to emphasize that we should strive for a good solution, and not just hush for something to get the release out. 👍

[RonnyPfannschmidt]

@flub we cant deprecate having multiple marks with the same name, it would break skip, skip_if and xfail_usages that happen in practice

i consider the perception that one "typically" wants the closes mark a false one - for example at a work project - i believe actually all custom marks (about a dozen ones for different purposes) have requirements for correctly working with individual markings, considering them in order

[nicoddemus]

i consider the perception that one "typically" wants the closes mark a false one

I see. Just to share, I reviewed all custom marks we use at work and they all actually are meant to work as the "closest" one. I think the same can be said about all the markers @The-Compiler brought up in his original post, so I guess the mileage varies according to the codebase.

I propose then:

1. Adding the get_closest_mark(name) function: its semantics are obvious and well defined I believe.
2. Update the deprecation message to mention both get_closest_mark(name) and find_marks() as possible alternatives to get_marker.
3. Add more examples to the documentation about how to replace get_marker().

What do you guys think?

[flub]

So maybe it helps enumerating the strategies for handling marks. I'm doing this partially to expose my naivety about this again, but also because if we're finding this so hard then maybe we should spell these various things out very clearly in the documentation complete with sensible examples of how these marking strategies work. Anyway, here my attempt:

1. Marks compose additive. E.g. skipif(condiion) marks means you just want to evaluate all of them, order doesn't even matter. You probably want to think of your marks as a set here.

2. Marks overwrite each other. This is the get_closest_mark case, so order matters but you only want to think of your mark as a single item. E.g. log_level('info') at a module level can be overwritten to log_level('debug') for a specific test.

3. Marks compose, but order matters. I'm struggling to think of an example here. Any ideas?

4. Marks do not compose and should never appear on more then one level. Not sure there's a sane example here either.

So I think this really should be added to the documentation of marks and then can be referred too in the news for the release as well as justification.

Also this maybe also suggests what the APIs should be like: get_closes_mark() which returns a single mark and a get_all_marks() which returns a set.

What do you all think? Am I completely off the mark or is this useful?

[nicoddemus]

@flub thanks, enumerating all cases is very helpful.

In my mind there were only two use cases, 1) and 2). I also can't think of examples for 3) and 4).

[RonnyPfannschmidt]

4. is indeed usefull, i have a use-case for 3 in a project at work (where more close marks refine outer marks)

[The-Compiler]

What do you all think? Am I completely off the mark or is this useful?

Please tell me that pun was intentional. 😆

I agree there are probably those cases. Some thoughts about each:

Case 1 and 3 are basically the same, just ordered vs. unordered, right? I think the current iter_markers can easily cover both, as even with 1. you probably want to iterate over them to somehow process them anyways. So even if we can't think of an exact example for 3., I do think they exist (similar to what Ronny said), and it's trivial to support when we support 1. If someone wants a set, assuming that marks are hashable, you can always do set(node.iter_markers()), right?

Case 2 is what I think is the most common case - I'm not just speaking for qutebrowser here, I've seen many pytest testsuites and plugins by now. We should support it in some straightforward way, and I agree a Node.get_closest_marker(name) would be that.

Case 4 might be useful, but I'm not sure - @RonnyPfannschmidt do you have a more concrete example? Maybe it's useful when I just never want any marker ambiguity in my testsuite (because this can be hard to reason about)? I'm not sure how the API would look, though. I first thought about a node.get_closest_marker(name, strict=True) or whatever, but that'd mean e.g. plugins would have to decide. Not sure if it's worth the trouble really - and if it is, I think that's one we could postpone until after the release.

[RonnyPfannschmidt]

whops, the one about 4 was a typo - i meant 3, sorry for the confusion

[RonnyPfannschmidt]

@The-Compiler what would strict mean

i propose 2 steps

a) updating iter_markers to def iter_markers(name=None) - where if given, a filter for the name attribute is added
b) adding def get_closest_marker(name, default=None) which is short for next(iter_markers(name), default)

[nicoddemus]

@RonnyPfannschmidt that sounds great to me. 👍

[RonnyPfannschmidt]

my initial implementation has some bugs, will reiterate

[RonnyPfannschmidt]

almost complete

[nicoddemus]

I think this can now be closed right?