TST Checks can now skip test based on estimator tag _xfail_test #16510
Conversation
sklearn/utils/estimator_checks.py
Outdated
| @wraps(check) | ||
| def wrapper(name, estimator_orig): | ||
| xfail_checks = _safe_tags(estimator_orig, '_xfail_test') | ||
| check_name = _set_check_estimator_ids(check).split("(", maxsplit=1)[0] |
There was a problem hiding this comment.
I don't think we need the .split, it's intentionally there to differentiate checks with same names but different arguments see e.g. #16507
sklearn/utils/estimator_checks.py
Outdated
| msg = xfail_checks[check_name] | ||
| try: | ||
| import pytest | ||
| pytest.xfail(msg) |
There was a problem hiding this comment.
I agree we need something like it, and indeed raising SkipTest on xfail was missing in the original PR, I added it differently here #16507 (comment).
However, with pytest installed wouldn't this stop exectution with an xfail exception, and so we will never get xpass. Maybe there is some way around it with pytest.param(..., marks=pytest.mark.xfail()) not sure.
There was a problem hiding this comment.
Updated this PR with another approach. The parametrize is now generated in such a way that the xfails are marked.
There was a problem hiding this comment.
Thanks have you checked that this indeed runs the tests (I know it should) i.e. that marking a passing test with xfail results in an XPASS?
sklearn/utils/estimator_checks.py
Outdated
| return pytest.param( | ||
| estimator, check, marks=pytest.mark.xfail(reason=msg)) | ||
|
|
||
| except KeyError: |
There was a problem hiding this comment.
I'm not very keen on this. KeyError can happen for a variety of reasons say in pytest.params in which case it will silently pass that error.
I would rather we did,
msg = xfail_checks.get(check_name, None)
if msg is not None:
or something similar.
Generally the case of a KeyError is the default case here (since most tests are not xfailed) and triggering/catching an exception there being expensive is suboptimal https://en.wikipedia.org/wiki/Python_syntax_and_semantics#Exceptions
There was a problem hiding this comment.
Makes sense in this case. I have made the change.
sklearn/utils/estimator_checks.py
Outdated
| msg = xfail_checks[check_name] | ||
| try: | ||
| import pytest | ||
| pytest.xfail(msg) |
There was a problem hiding this comment.
Thanks have you checked that this indeed runs the tests (I know it should) i.e. that marking a passing test with xfail results in an XPASS?
sklearn/utils/estimator_checks.py
Outdated
| return pytest.param( | ||
| estimator, check, marks=pytest.mark.xfail(reason=msg)) | ||
|
|
||
| except KeyError: |
There was a problem hiding this comment.
Makes sense in this case. I have made the change.
There was a problem hiding this comment.
Thanks @thomasjpfan ! Merging as it shouldn't be too controversial, and I also double checked that it works as expected.
Reference Issues/PRs
Continues #16502
What does this implement/fix? Explain your changes.
The checks can now skip tests based on the
_xfail_test.