Please pursue step 2; step 1 is unnecessary, and we've correspondingly closed #13748

Thanks @jnothman - do you have any pointers on how I could get started on step 2 ? Or any pytest resources that could point me in the right direction to generate the tests for the collector ?

Sorry guys - is there any resource you could point me to to create tests for the pystest collector ?

I think this might be a good entry point:
https://docs.pytest.org/en/latest/usage.html#calling-pytest-from-python-code

Here's how to use hooks:
https://docs.pytest.org/en/latest/example/simple.html#incremental-testing-test-steps

I think you want to work with the collection hooks?
https://docs.pytest.org/en/latest/reference.html#collection-hooks

Thanks @amueller ! I will look into it

I wouldn't think we want to make use of pytest.run here.

I think we already have a usable solution internal to scikit-learn here:

but at the moment it relies on the internal _yield_all_checks. If we made that public, we could provide a pytest hook that would improve the syntax and allow for that test_estimators to be reduced to

@pytest.mark.estimator_checks(_tested_estimators())
def test_estimators(estimator, check):
    # Common tests for estimator instances
    with ignore_warnings(category=(DeprecationWarning, ConvergenceWarning,
                                   UserWarning, FutureWarning)):
        set_checking_parameters(estimator)
        name = estimator.__class__.__name__
        check(name, estimator)

or something even more succinct, such as not needing the pytest.mark syntax.

This assumes that it's fine to construct the estimator and identify which checks it qualifies at collection time (we currently do this in scikit-learn). This may not be efficient. To avoid constructing the estimator at collection time, you'd need to generate all the checks and skip those combinations that are inapplicable at test (not collection) time. Here one option would be to create an estimator_check fixture that is itself parametrized to consider each check a different collected item.

I'm happy with the former approach - a public alternative to our current use of _generate_checks_per_estimator - for now.

Thanks for the feedback @jnothman - I will work on that and update you with progress !

@jnothman I guess I wanted it to be a function, your solution would require the users to write a test, right? Which might be more sensible but not be a direct replacement for check_estimator.

I guess I wanted it to be a function, your solution would require the users to write a test, right? Which might be more sensible but not be a direct replacement for check_estimator.

check_estimator needs to be called in a test for library developers already, so I don't see what's changed. We could add magic like

test_my_estimator = make_check_estimator_test(MyEstimator())

but I don't really see the benefit.

If you really want to follow the approach I suggested above (not sure if it's the right thing to do), you will need to implement something much like https://github.com/pytest-dev/pytest/blob/2c402f4bd9cff2c6faeccb86a97364e1fa122a16/src/_pytest/python.py#L119-L128

but actually that is probably a nuisance since scikit-learn would have to provide a pytest plugin, which seems a bit silly? Maybe better off just having users do:

@pytest.mark.parametrize('check', yield_all_checks)
@pytest.mark.parametrize('estimator', _tested_estimators())
def test_estimators(estimator, check):
    # Common tests for estimator instances
    with ignore_warnings(category=(DeprecationWarning, ConvergenceWarning,
                                   UserWarning, FutureWarning)):
        set_checking_parameters(estimator)
        name = estimator.__class__.__name__
        check(name, estimator)

@pytest.mark.parametrize('check', yield_all_checks)
@pytest.mark.parametrize('estimator', _tested_estimators())

The limitation of this is that checks are estimator dependent, and having lots of skipped tests that will never run is also not ideal.

The solution proposed in #13843 (comment) sounds reasonable. It's fairly close to the one proposed in #11622 (comment)

ALL_ESTIMATORS = [Estimator1, Estimator2, etc]

@pytest.mark.parametrize(
    'check, name, estimator',
    itertools.chain.from_iterable(
         check_estimator(Estimator, evaluate=False)
         for Estimator in ALL_ESTIMATORS
    )
)

(maybe with an additional helper function), where check_estimator(Estimator, evaluate=False) yields checks for that estimator.

Right. Thanks for the reminder that we are trying to not overwhelm it with skips.

Fair, let's go with something like that. Seems pretty straight-forward, right?

I think we should have generate_only=True rather than evaluate=False, but yes, it's a good solution as far as I'm concerned.

@scouvreur I think we're going with #14381 right now. Not sure if there has been some miscommunication anywhere? cc @thomasjpfan

Yes, it seems we are leaning toward #14381. Thank you for working on this issue @scouvreur !

Ah I see @amueller - should I close this PR then ?