Open Zac-HD opened 2 weeks ago
@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.1 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.
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.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!
Perhaps a ruff rule might help aswell
@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. :grinning:
@ferdnyc I was suggesting a upstream ruff rule that's the default in future
Oh! Oh, that's a neat idea, yeah.
(Edit: GitHub really needs to add :bulb: to its paltry set of comment reactions.)
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.
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.
I'd recommend a soft deprecation,
If we support lambda expressions instead, it's probably doable to supply a ruff auto-fix thats Safe
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 🙂
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.).
String conditionals are able to use extra values like the pytest configuration and a few more things
I was wondering if there was a use case for lazy evaluation—seems like there is.
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.
See https://github.com/pytest-dev/pytest/pull/12172#discussion_r1667357437 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:
skip
and a mark named e.g.skipif
also exists - for any name, not justskip
, andreason
or first-positional-argument string is (1) a valid expression forast.parse
, and (2) it's not just a trivialname
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')")
.