[MRG] Adds XFAIL/XPASS to common tests

[thomasjpfan]

Reference Issues/PRs

Alternative to #16306

What does this implement/fix? Explain your changes.

Places the xfail into the test themselves.

Any other comments?

check_methods_subset_invariance feels like it should be broken down into multiple tests - one for each method.

CC @rth

[rth]

I'm biased but I think it's more natural to skip or mark a test a xfail in the place where the calculation is done, rather than keeping a global dict somewhere else. Estimators checks are already complex for new contributors, and this will make it more complex because the behavior of a common check is no longer self contained.

Here we mark it based on estimator name and check name, but there could potentially be more logic determining when to call xfail or skip (say something based on parameters of the estimator used, etc).

If we could use pytest in common tests, we would just call pytest.xfail there as needed. As we can't do that we need to make a small wrapper, to call it conditionally. Do you think that the other PR is too complicated?

[thomasjpfan]

As we can't do that we need to make a small wrapper, to call it conditionally. Do you think that the other PR is too complicated?

I do not think it is too complicated for a seasoned pytest user.

I am thinking of this from a third party user of parametrize_with_checks, and how would they use xfail. They would not be able to add custom logic into the check and would have to use an approach like this PR to xfail a specific check.

[rth]

I am thinking of this from a third party user of parametrize_with_checks, and how would they use xfail. They would not be able to add custom logic into the check and would have to use an approach like this PR to xfail a specific check.

Absolutely, contrib users would have to do this that way, and if there is a way to making it smoother for for them with parametrize_with_checks that would be nice.

We are not a contrib user though, we can change code inside check estimaor, and same as we don't have a global list of all tests that are skipped we shouldn't have one for xfail I think.

[thomasjpfan]

We are not a contrib user though, we can change code inside check estimaor, and same as we don't have a global list of all tests that are skipped we shouldn't have one for xfail I think.

I think having special cases inside check_estimator leads to issues like #6715

For example, lets say someone creates a estimator with the name "BernoulliRBM" that passes check_methods_subset_invariance (without skipping), with #16306 it will xpass and fail. On master, it will skip the test.

For the test skips, we are mostly using conditions that do not need a global list:

1. Estimator cant be instantiate
2. Pandas not installed
3. Tags say to skip

The only skip depending on name is check_estimators_data_not_an_array, which depends on CROSS_DECOMPOSITION. Maybe this should be xfailed?

In my mental testing framework, lets say we have a check that fails. I would mark the test_* as xfail and not go into the check's code and use a condition to determine if it should xfail.

Anyways, I am happy with either solution, fundamentally I agree it would be nice to move forward on the PRs mentioned in #16306 (comment)

[thomasjpfan]

I would want to use check_estimator as a contrib developer so we can see what it is like to be a contrib developer. With this PR, we can showcase how a contrib developer can mark tests as xfail for their custom estimator and work their way up to getting the tests to pass.

In the end, I want to see an ecosystem of estimators that pass check_estimator and thus conform to the API.

[rth]

For example, lets say someone creates a estimator with the name "BernoulliRBM" that passes check_methods_subset_invariance (without skipping), with #16306 it will xpass and fail. On master, it will skip the test.

For the identically named estimators, it is indeed a valid use case, but also one has to agree that naming estimators identically to scikit-learn and use scikit-learn tooling on them is not the best idea.

I would want to use check_estimator as a contrib developer so we can see what it is like to be a contrib developer. With this PR, we can showcase how a contrib developer can mark tests as xfail for their custom estimator and work their way up to getting the tests to pass.

https://github.com/scikit-learn-contrib/scikit-learn-extra/ is looking for maintainers in case you are looking for additional experiences as a contrib developper :)

I share your vision that we should make life easier for contrib developpers, I'm just saying that I would rather it did not involve making things more annoying for scikit-learn contributors. Personally, when I work on one of the above linked PRs, I have one tab for the common check, one tab for the estimator code. I don't think that having to open tests/tests_common.py and remember to change things there in addition is helping.

The idea that common checks should not contain any scikit-learn specific logic is laudable, but I don't think very realistic in the near future. That's why we wanted to add skip=True to disable scikit-learn specific checks. And if scikit-learn specific logic remains in checks, I don't see why we should put the scikit-learn specific xfail in a separate file than where they happen.

I would mark the test_* as xfail and not go into the check's code and use a condition to determine if it should xfail.

On the opposite, I would go to the checks code to see what failed exactly, and while I'm there I can as well mark it as xfail.

For contrib users, in my experience one would as much skip common tests than mark them as xfail, which would need proper documentation in any case.

We have skip tests for docstrings in conftest.py only because there is no other way to do it, but I don't think it's a pleasant contribution flow either and new contributors certainly have a hard time finding it unless they are told where to look.

In any case, we need to convince some other reviewer to approve this or the other PR, it's not too important in the end and both would solve the immediate problem :)