Warn on pytest.mark.skip("sys.version_info < (3, 6)") (should use skipif)

[Zac-HD]

See #12172 (comment) for some cases where this bit us; at time of writing there are at least three cases in Pytest's own tests where this is biting us. I think it's pretty unlikely that we're the only people ever to make this mistake, so let's add a warning that would have caught it.

I propose that the warning should trigger if:

• the mark is named e.g. skip and a mark named e.g. skipif also exists - for any name, not just skip, and
• the reason or first-positional-argument string is (1) a valid expression for ast.parse, and (2) it's not just a trivial name expression (i.e. contains a comparison and/or a call)

Based on some quick searches, this would not have any false alarms (and so I think there would be very few after shipping), and it would catch things like pytest.mark.skip("sys.platform.startswith('win')").

[ferdnyc]

@Zac-HD

I think it's pretty unlikely that we're the only people ever to make this mistake,

Word.

It almost makes me think the problem is the API. Having pytest.mark.skip and pytest.mark.skipif "right next to each other" is begging for trouble, really. It's like Qt's QTableWidget having both itemAt() and item() methods.¹ Recipe for disaster.

It's too bad they're both under the mark module, which is really more a developer-organizational thing than something for users. If pytest.mark.skip were just pytest.skip, and pytest.mark.skipif were pytest.conditional.skipif (or even pytest.conditional.skip), there'd probably be a lot less confusion. (Except among people who do things like from pytest import skip; from pytest.conditional import skipif, but there's no helping those people.)

Going back to your warning proposal, I'm a bit confused... how would a warning condition containing " " in reason.strip() trigger for pytest.mark.skip("sys.platform.startswith('win')"), which has no spaces in the reason argument?

Also, wouldn't condition (1) require running every reason for every pytest.mark.skip() call through the Python parser? When most of them will presumably be just free-form text strings? That sounds... potentially messy, even if it's limited only to test environments where pytest.mark.skipif is also in use.

Notes

1. QTableWidget::itemAt(x, y) returns the item under the given local display coordinates, while QTableWidget::item(row, col) returns the cell at the given row & column of the table. And since both methods take two integer arguments, developers have been regularly confusing them ever since they were introduced.

[Zac-HD]

Oops, warning proposal was indeed nonsensical, edited.

I don't want to reconsider the whole API design here, just a small improvement on it. But yeah, the confusability certainly doesn't help!

[RonnyPfannschmidt]

Perhaps a ruff rule might help aswell

[ferdnyc]

@RonnyPfannschmidt

A ruff rule might help pytest, but the implicit concern here is about the potential hundreds of other projects whose test files may contain the same silent mistake. A ruff rule in the pytest repo won't help them. 😀

[RonnyPfannschmidt]

@ferdnyc I was suggesting a upstream ruff rule that's the default in future

[ferdnyc]

Oh! Oh, that's a neat idea, yeah.

(Edit: GitHub really needs to add 💡 to its paltry set of comment reactions.)

[pygeek]

Is there a reason not to deprecate skipIf string evaluation, and just constrain the condition parameter to being type bool?

Furthermore, would it make sense to deprecate skipIf all together, and bake the conditionals into the skip API?:

def skip(reason: str = None, conditional: bool = None)

This would be backwards compatible (ie: skip("a reason")) and be very difficult to fat finger.

PS:

Technically, additional logic, we could maintain the order of, conditional,..., *, reason while maintaining backwards compatibility—though this behavior may not be intuitive.

[The-Compiler]

Personally I'd like to deprecate string conditions, but then again, they're in quite wide use and that'd cause quite an amount of code churn.

-1 to reason, condition arguments. We already have some inconsistencies between how those arguments with (also with xfail), and that adds yet another layer of confusion by doing it the opposite way it's been done so far.

[RonnyPfannschmidt]

I'd recommend a soft deprecation,

If we support lambda expressions instead, it's probably doable to supply a ruff auto-fix thats Safe

[Zac-HD]

I don't think a broader API change is worth the churn at the moment; we can ship this small tweak and move on to the many other open issues 🙂

[pygeek]

Yes to the soft deprecation. I should have clarified in regards to the deprecation—I meant adding a deprecation warning—at least until next major release.

As for the lambda, It's still not clear to me why this would be necessary, aside from lazy evaluation. It should be noted that wrapping these conditionals in lambda would impact performance (though negligible in most use cases).

I was considering passing conditionals into skip as tuple[bool]. This would bring the skip API in line with other pytest module APIs (ie: parameterize. albeit, differing from the more closely related xfail API), and likely reduce complexity and improve performance (we wouldn't have to check each parameter to check for condition / reason). I'd like to keep the scope tight here, however, so I'm a bit on the fence.

---

As a side note, we could pass all skipif args strings through the conditional parser:

• If it throws an exception parse it as a normal skip

• If it is parsed successfully as type bool, raise a similar error to what that is raised currently when skip is passed a conditional (src/_pytest/skipping.py#L190).

Though this feature would only be useful as an augmentation to skipif. skip wouldn't support parsing conditionals as strings to begin with. And of course, this would impact performance.

I'm not advocating for this particular approach, and personally don't think it's worth the added complexity, but I do think it's worth discussion (ie: perhaps most appropriate as a 3rd party pytest plugin.).

[RonnyPfannschmidt]

String conditionals are able to use extra values like the pytest configuration and a few more things

[pygeek]

I was wondering if there was a use case for lazy evaluation—seems like there is.

[pygeek]

I don't think a broader API change is worth the churn at the moment; we can ship this small tweak and move on to the many other open issues 🙂

Sounds good. Is this something you're working on? Otherwise, I can create PR.