Allso all the assert_equal, assert_true etc (e.g. here) can be removed now that pytest is used for tests. There must be an issue on it somewhere but I'm not able to find it.

Or was it decided that e.g. running nose2pytest on the whole repo was not desirable as it would create conflicts for existing PRs?

Merge conflicts with parametrize-based fixes to yield tests will be hard to resolve...

Thanks for the additional details @jnothman @lesteve !

We should take on the yield test changes now, file by file, if possible.

Maybe module by module would be easier to follow (apart for the estimator_checks)?

Here is a list of all test files (~29) that use yield (this can have some false positives, feel free to edit this comment).

$ find sklearn/ -iname "*test_*py" -exec grep -l '^[[:space:]]\+yield ' {} +

sklearn/ensemble/tests/test_gradient_boosting.py
sklearn/ensemble/tests/test_forest.py
sklearn/linear_model/tests/test_ridge.py
sklearn/manifold/tests/test_t_sne.py
sklearn/metrics/tests/test_common.py
sklearn/metrics/tests/test_ranking.py
sklearn/metrics/tests/test_pairwise.py
sklearn/metrics/tests/test_score_objects.py
sklearn/preprocessing/tests/test_label.py
sklearn/svm/tests/test_bounds.py
sklearn/utils/tests/test_stats.py
sklearn/utils/tests/test_extmath.py
sklearn/gaussian_process/tests/test_kernels.py
sklearn/model_selection/tests/test_validation.py
sklearn/mixture/tests/test_gmm.py
sklearn/neighbors/tests/test_neighbors.py
sklearn/neighbors/tests/test_kd_tree.py
sklearn/neighbors/tests/test_dist_metrics.py
sklearn/neighbors/tests/test_quad_tree.py
sklearn/neighbors/tests/test_ball_tree.py
sklearn/neighbors/tests/test_kde.py
sklearn/decomposition/tests/test_pca.py
sklearn/decomposition/tests/test_online_lda.py
sklearn/tree/tests/test_tree.py
sklearn/datasets/tests/test_svmlight_format.py
sklearn/tests/test_naive_bayes.py
sklearn/tests/test_common.py
sklearn/tests/test_cross_validation.py
sklearn/tests/test_random_projection.py

IMO the most painful place to do it is in estimator_checks because estimator_checks can be used outside of a pytest run e.g. by downstream packages (look at e.g. imblearn for an example) or just by calling check_estimator. Something I realised while looking at #10663 is the fact that estimator_checks can be used as a module outside of a pytest run makes it a bit akward to use pytest tricks (in my case I wished to use the tmpdir fixture but that was just not easily doable).

One promising approach I can remember was #7319 (comment) although I have not looked at it in too many details.

• reviving the part of [WIP] Pytest test suite completely independent of nose #8321 that was removing yield tests would be a good first PR.
• I would be in favour of looking at estimator_checks in a separate PR
• there are other things in [WIP] Pytest test suite completely independent of nose #8321 that seems like it could be solved by just using the tmpdir or tmpdir_factory fixture. That could go in a separate PR too please.
• About removal of assert_true, assert_equal, I find it hard to really care about this I have to say, but if someone want to run nose2pytest (or something else) to automate the bulk of it, why not ... in a separate PR too though.

I'm +1 for first removing yield and I'm +1 for @jnothman's way of doing this : simply execute the function instead of yielding it (like numpy/numpy#10294). yield has been deprecated and I don't think removing it in this way will introduce too many conflicts in existing PRs.

Just to be explicit, the downside with replacing yield check, args by check(args) is that you have one single test function instead of multiple test functions. This means that the first assert failing with stop the test whereas it is sometimes nice to have all the failures in one go.

Having said that outside of estimator_checks it/test_common is probably fine to do the simplest thing i.e. yield check, args -> check(args).

The verbosity of the current approach with yield is nice but IMO there is a larger problem in the fact that a significant portion of tests is in common tests (and it would make sense to increase this portion even more) but there is no way currently to run a particular common test for all estimators.

For instance, in the last sprint cc @glemaitre , #11000 should have been straightforward (at least on the purely coding side, ignoring 64/32bit complications for now) : add a common test that checks 32/64 bit preservation then make it pass without breaking the exiting ones. In reality, it was somewhat painful because,

• one had to run a large part of common tests just to get that test. This makes iterations slow, so one can forget about TDD and essentially this wastes contributor time
• or write a separate not committed test to run this check for all applicable estimators but then we are not sure that we are actually running it on the same estimators as in _yield_non_meta_checks because there is some more selection logic there. Also, that's confusing for new contributors.

There are a few other issues where this could be relevant I think.

Current situation

(simplified)

sklearn/tests/test_common.py

def test_non_meta_estimators():
    for name, Estimator in all_estimators():
        estimator = Estimator()
        for check in _yield_all_checks(name, estimator):
            yield check, name, estimator

sklearn/utils/estimator_checks.py

def _yield_non_meta_checks(name, estimator)
    yield check_a
    if name in SOME_LIST:
        yield check_b
    # etc.

def check_a(name, estimator):
   # check something.

def check_b(name, estimator):
   # check something else only applies
   # to a few estimators

def check_estimatator(Estimator):
    # similar code to test_non_meta_estimators
    # but running check(name, estimator) instead of
    # yielding

Desired behavior

Putting aside the fact that yield will be deprecated by pytest, the above allows to

• have a separate entry for test_a in the test suite for each estimator it applies to.
• be able to run all tests for an estimator with check_estimator both just in a python interpreter without requiring pytest and in the test suite of some other sciki-learn-contrib project.

what it doesn't allow to do is to run e.g. check_a for all estimators. This is problematic as outlined above.

Possible approach

The most straightforward thing, I guess, would be to parametrize check_a, check_a. For instance,

def selector_check_a(name):
    return True

def selector_check_b(name):
    return name in SOME_LIST

@pytest.mark.parametrize('name, Estimator',
                         [(name, Estimator) for name, Estimator in all_estimators()
                          if selector_check_a(name)])
def test_non_meta_estimator_check_a(name, estimator):
    check_a(name, estimator)

# same for check_b

def check_estimators(Estimator):
    name = Estimator.__name__
    if selector_check_a(name):
        check_a(name, Estimator)
    ... 
    # essentially same thing as in _yield_non_meta_checks but with
    # factorized selectors

This is very rough prototyping, it would be good to associate a check and it's selector in the same object somehow, and improve other things, but in any case, I think, this should get us the desired behavior from the current setup,

• also one can use pytest selector -k to run all non_meta_estimator checks,

  pytest sklearn -k non_meta_estimator
  

• and the ability to run a single common test for all relevant estimators,

  pytest sklearn -k test_non_meta_estimator_check_a
  

I have not followed in detail the work on estimator tags and it's implication on check_estimator #6715, but I guess the above selectors could be used to filter on some tags as well..

What do you think?

Possible approach

My message above fails to account for several constraints, but an improved approach that should work can be found in #11063

As stated in #11063 (comment):

sklearn/utils/estimator_checks.py is left unchanged, and the parametrization relies on _yield_all_checks to produce a list of valid checks for a given estimator.

yield-tests are still used, so this issue is not really complete?

I'm asking from the view of a downstream project, I'm testing my classifier with check_estimator, and would like to see which of its individual tests failed. Doing so under nose by using for check in _yield_all_checks: yield check, name, my_estimator, but py.test won't support this.

This will be included in the not yet released scikit-learn 0.20+, unless you are calling check_estimator with scikit-learn from master?

For downstream projects check_estimator should still work with both pytest and nose. The fact that check_estimator uses yields internally is fine with both.

The issue was about having patterns of the form ,

def test_something():
   for check in checks:
      yield check

in scikit-learn tests, these have been now replaced with pytest parametric tests but it should have no incidence on downstream projects.

For downstream projects check_estimator should still work with both pytest and nose.

Yes it works, but if a single test from check_estimator fails all the others are not run. I think that's bad for debugging the own estimator (you never know how many/which things will come up after you fixed the current test, or maybe how the failures are related), so I disassembled the function (for check in _yield_all_checks: yield check, name, my_estimator), making nose run all these separately.

So actually yes, I mangled this issue with my own (stemming from using that private function), sorry for that. I should open another issue to re-fit check_estimator for being run "in pieces" as I suggested, shouldn't I?

It a valid concern, if you could open a separate issue about it it would be great! Thank you!