Stricter arg/kwarg handling in pytest.mark.skip; skip vs. skipif

[The-Compiler]

Apologies if there already is an issue about this, I tried to search, but it's a bit tricky to find a proper term for this.

It seems like a somewhat common issue for people to accidentally use @pytest.mark.skip when they actually wanted skipif.

Strangely enough, pytest will happily accept something like:

import pytest

@pytest.mark.skip(False, reason="bla")
def test_skipped():
    pass

and skip the test.

This is due to how arguments are handled in the marker:

pytest/src/_pytest/skipping.py

Lines 186 to 193 in cd783eb

	for mark in item.iter_markers(name="skip"):
	if "reason" in mark.kwargs:
	reason = mark.kwargs["reason"]
	elif mark.args:
	reason = mark.args[0]
	else:
	reason = "unconditional skip"
	return Skip(reason)

Yet our reference documentation actually claims:

pytest.mark.skip(*, reason=None)

which isn't true.

I see a couple of ways how to fix this. By decreasing impact of the respective changes:

• Make the skip marker actually take a condition as first argument, so that it replaces skipif entirely, and deprecate the latter. The difference between skip / skipif and xfail (but no xfailif) has always bothered me to be honest.
• Deprecate and warn about using pytest.mark.skip with a positional argument (i.e. make the signature consistent with what the docs claim).
• Continue to accept the reason as positional argument, but at least show an error if both are given (i.e. like Python would if the signature was def skip(reason):).

[Zac-HD]

I think it would be better if skipif had never existed, but at this point it's very widely used in the wild. I would be strongly against removal unless we also provided HypothesisWorks/hypothesis#2705 upgrade tooling, and weakly against it even so. (adding xfailif could be nice, though!)

So consider this a vote for enforcing the documented signature, after an appropriate deprecation period.

[nicoddemus]

I definitely like the idea of enforcing the signature!

I also agree with @Zac-HD though that we shouldn't deprecate skipif because I believe it is much more used than skipif currently (skip is relatively recent).

Currently we have these documented signatures:

pytest.mark.skip(*, reason=None)
pytest.mark.skipif(condition, *, reason=None)

Make the skip marker actually take a condition as first argument, so that it replaces skipif entirely, and deprecate the latter. The difference between skip / skipif and xfail (but no xfailif) has always bothered me to be honest.

I like that, except for the "deprecate the latter" part.

IIUC then, the steps to accomplish that would be:

1. Update pytest.mark.skip signature to:

   pytest.mark.skip(condition: Union[str, bool]=True, *, reason: Optional[str]=None)

   This makes it a drop-in replacement for skipif.

2. Update all skipif examples in the docs to use skip.

3. Mention skipif in the docs just as an alias to skip, commenting about the historical reasons that led both to exist, however also mention that we don't plan to remove it anytime soon, or ever.

[The-Compiler]

@nicoddemus Note that those two solutions are pretty much mutually exclusive.

If we enforce the documented signature:

• Existing usage like pytest.mark.skip(not utils.is_linux, reason="Only on Linux") will show an error (instead of silently skipping) and needs to be migrated to skipif.
• Existing usage like pytest.mark.skip("Work on progress") will show an error (instead of working properly, albeit undocumented) and will need to be migrated to reason="Work in progress".

If we make skip an alias to skipif:

• Existing usage like pytest.mark.skip(not utils.is_linux, reason="Only on Linux") will now start running on macOS and Windows where it was previously skipped (thus potentially leading to "why is my testsuite failing after the upgrade?" questions, which are hard to answer if you don't know about what's going on)
• Existing usage like pytest.mark.skip("Work on progress") will now interpret the former reason string as condition instead. I suspect this will fail for most (all) of those strings, though the failure might not be obvious either.

I wonder if we should first introduce an error (or at least warning) for the "wrong" usage, and then only later reintroduce skip as an alias to skipif - kind of similar to what we did with --strict?

[nicoddemus]

@The-Compiler yeah you are right. A quick search shows that there are many usages without the keyword argument (@pytest.mark.skip(reason='Unimplemented Test')) so those would break if we make the skip and skipif signatures equal; I thought those would be few but that doesn't seem to be the case.

With that in mind, I'm leaning towards just enforcing the signature to this:

def skip(reason=None):

(a single argument, either positional or reason keyword)

As you point out, making skipif an alias to skipif will force thousands of test suites to change, which I'm not sure it worth doing at this point.

[Zac-HD]

Closed by #8392 🎉