Better handling of mark signatures

[nicoddemus]

A known problem when dealing with marks is to properly support the intended signature, as this can be tricky and error prone.

For example, the official signature of skipif is:

skipif(condition, *, reason=None)

But the skipping plugin needs to manually handle this signature using args and kwargs, which is error prone and has been a source of errors in the past (letting an unknown keyword argument slip through silently for example).

Someone mentioned about using actual Python functions to declare mark signatures in our pytest sprint back in 2016 (I think it was @RonnyPfannschmidt or @hpk42). I decided to play a bit with it and got excited about how simple it was to get this working (thanks to @RonnyPfannschmidt's large mark refactoring):

import pytest
from _pytest.mark import MarkDecorator, Mark


def skipif(condition, *, reason=None):
    return MarkDecorator(Mark('skipif', (condition,), dict(reason=reason)))


def parametrize(argnames, argvalues, *, indirect=False, ids=None, scope=None):
    return MarkDecorator(
        Mark('parametrize', (argnames, argvalues), dict(indirect=indirect, ids=ids, scope=scope)))


pytest.mark.skipif = skipif
pytest.mark.parametrize = parametrize


@pytest.mark.skipif(False, reason='some reason')
def test(request):
    print(request.node.get_closest_marker('skipif'))


@pytest.mark.parametrize('x', range(10), ids=None)
def test_foo(x):
    print(x)

If we pass the wrong arguments, we get the expected error:

@pytest.mark.skipif(foo=3)
def test_bar(request):
    pass

    @pytest.mark.skipif(foo=3)
E   TypeError: skipif() got an unexpected keyword argument 'foo'

And even better, at collection time instead of at runtime.

---

While we should have a large discussion on the design of an API to expose this functionality to users for custom marks, I propose we adopt this strategy in 5.0 for the builtin marks. This will help users catch errors now, while not exposing any implementation detail. Even if we decide on a completely different approach in the future, this is transparent to users.

[nicoddemus]

API for 5.1 (or later)

There are two aspects here: how to provide the mark signature and when.

How

The user provides a Python function with the desired signature:

def skipif(condition, *, reason=None):
    ...

The return value must be a pair of args and kwargs. We enforce and check the return type as Tuple[Tuple, Dict]:

def skipif(condition, *, reason=None):
    return (condition,), dict(reason=reason)

def parametrize(argnames, argvalues, *, indirect=False, ids=None, scope=None):
    return (argnames, argvalues), dict(indirect=indirect, ids=ids, scope=scope)

The idea of returning a custom object which would then be returned by Node.get_closest_marker can also be supported in the future, in which case this function would return a single object. This is of course a detail to be discussed earlier.

When

Right now makers are registered during pytest_configure:

def pytest_configure(config):
    config.addinivalue_line(
        "markers", "env(name): mark test to run only on named environment"
    )

We can follow the same idea and provide a function in the config object that registers marks. It would be an alternative to the method above:

def pytest_configure(config):

    def env_marker(name):
        """mark test to run only on named environment"""
        return (name,), {}

    config.register_marker("env", env_marker)

Perhaps we can swap the order of the parameters and use the name of the function as default for the name of the marker:

def pytest_configure(config):

    def env(name):
        """mark test to run only on named environment"""
        return (name,), {}

    config.register_marker(env, name=None)  # name is optional

One small problem with the above approach (including the existing addinivalue_line) is that it might be called at any time a config object is available, which is in a lot of hooks.

If we want to restrict users to only call register_marker at a certain time, we could pass an additional parameter to pytest_configure which is used to register markers. This would force users to only register markers during pytest_configure.

def pytest_configure(marker_registry):

    def env(name):
        """mark test to run only on named environment"""
        return (name,), {}

    marker_registry.register_marker(env, name=None)  # name is optional

Thoughts?

[RonnyPfannschmidt]

-10 on it,

we should aim for type based markers in general and registering marker constructors by name

this whole mess randomly combined data has to go

[nicoddemus]

-10 on it,

That's harsh. 😳

I assume you mean the proposal for the API, not the first proposal about using function signatures to validate the builtin markers?

we should aim for type based markers in general and registering marker constructors by name

You mean supporting different types of marks? My proposal for the API allows for that, you can use register_mark to register a new marker type.

[RonnyPfannschmidt]

i dont mean "type" as in kinds, i mean Type as in instead of Mark(name, args, kwargs) i want to be able to use any python class like

@attr.s
class ConditionalSkip;
  condition = attr.ib()
  reason = attr.ib(default=None, kw_only=True)```

and registration should have a primary metadata entrypoint

aka

[pytest.marker]
skipif = _pytest.skipping.ConditionalSkip

this would allow the marker registry to set up pretty early in the configuration process

on top of that there should e a mechanism to add back the legacy markers, and for backward compatibility for quite a while there should be name based legacy markers, and type based modern markers

additionally a marker "constructor" should be able to return none to skip out, and it should be possible t have more than one type of marker per marker name (so the runtime can choose which implementations of a marker apply)

[nicoddemus]

i want to be able to use any python class like

I see, that's what I had in mind:

register_marker("skipif", ConditionalSkip)

this would allow the marker registry to set up pretty early in the configuration process

Point taken about being able to register this as meta-data, either in pytest.ini or in an entry-point (for plugins). For plugins would it look like this?

setup(
    entry_points={
        "pytest11": ["timeout = pytest_timeout"],
        "pytest-markers": ["pytest_timeout.TimeoutMark"],
    },
    ...
)

But does this mean you don't want any type of runtime registration?

on top of that there should e a mechanism to add back the legacy markers, and for backward compatibility for quite a while there should be name based legacy markers, and type based modern markers

How about:

[pytest.markers]
slow = "pytest.Marker"  # name-based marker
timeout = "pytest_timeout.TimeoutMark"

additionally a marker "constructor" should be able to return none to skip out, and it should be possible t have more than one type of marker per marker name (so the runtime can choose which implementations of a marker apply)

Not sure I follow, could you elaborate?

[RonnyPfannschmidt]

i already had the static registration example in my initial writeup
dynamic registration should be possible

the problem is, that if we go from Mark("skipif", ...) to ConfitionalSkip(....)

on order to keep api consumers working we need a mechanism so that both exist for a while, slowly opting out of the legacy api/marker declaration mechanism

[nicoddemus]

i already had the static registration example in my initial writeup
dynamic registration should be possible

OK. Does the example of using the entry points in setup.py match what you had in mind about registering marks using meta-data?

slowly opting out of the legacy api/marker declaration mechanism

Just declaring named markers is very useful and probably the most common use case out there, for example pytest.mark.slow, or pytest.mark.trio. We shouldn't force users to create a class if they are not interested on it having any parameters.

[RonnyPfannschmidt]

that reminds we that there should be a more correct mechanism for symbolic markers

[nicoddemus]

Closing in favor of #5424