[MRG+2] Pytest parametrize unit tests #11074

rth · 2018-05-07T14:36:26Z

Reference Issues/PRs

Continues #8321

What does this implement/fix? Explain your changes.

This PR,

Replaces tests that use yield with pytest parameterization

Parametrizing some tests that don't use yield but where it would be helpful (e.g. a tests is a for loop over estimators, solvers etc). This applies to,

def test_a():
   for solver in solvers:
       # run some tests

but not

def test_b():
   # computationally expensive setup + some tests
   for solver in solvers:
       # run some tests

In some occasions, where using paramerization is not straightforward, yields were simply removed i.e. yield func, arg replaced by func(arg)

Beyond removing deprecation warnings in pytest, the motivation is somewhat similar to #11063 : better verbosity in tests logs and ability to select tests with the -k argument.

Any other comments?

The changes are fairly mechanical but there is a lot of them. For review, I think it would make sense to split this in smaller pieces,

part 1: sklearn.ensemble PR [MRG+1] Pytest parametrize unit tests (part 1) - ensemble module #11075
part 2: sklearn.{cluster, datasets, decomposition} [MRG+1] Pytest parametrization part2 - cluster, datasets and decomposition modules #11142
part 3: sklearn.{feature_extraction,gaussian_process} [MRG+1] Pytest parametrize part3 - feature_extraction, gaussian_process modules #11143

~~Please add general comments here, but review specific PRs linked above. The diff in this PR will be updated in the future.~~ Once the above PRs are merged, the remaining diff can be directly reviewed here.

Because a lot of these changes consist in taking a loop over some list of parameters, and writing it with @pytest.mark.parametrize the diff contains a lot of lines where only the indentation changes. As a workaround, I find helpful the option to ignore whitespaces in Github diff (by appending the &w=1 string to the URL or using the Refined Github extension which creates a UI button for it).

Please let met me know if can do anything to make the review easier.

…e-non-common-tests

amueller · 2018-05-19T19:15:40Z

still broken?

rth · 2018-05-20T08:27:25Z

still broken?

Was waiting for #11075 to reach consensus before updating this PR with changes there, which should also help fixing CI..

…ommon-tests

…scikit-learn into pytest-parametrize-non-common-tests

rth · 2018-05-27T19:18:39Z

Tests are passing now, but it still might be easier review smaller chunks of this linked in the second part of the description #11074 (comment) (or liked above this comment).

Leaving the WIP status even if this PR is mostly done to indicate that the diff will be updated as its chunks / sub-PRs are merged.

qinhanmin2014 · 2018-06-01T11:43:16Z

Curious about the purpose of this PR? If it contains all the remaining things except from part1-part3 (after part3 begin merged), I think it might be better to review this PR directly (at least I'm willing to do so)

rth · 2018-06-01T11:55:33Z

Initially, I was trying to split it in smaller peaces to make review easier. After part 3 (#11143) is merged, I agree that the remaining diff here should be managable. Marking this as MRG. Thanks @qinhanmin2014 !

lesteve

LGTM, just a small side-comment about the instable test that you found.

lesteve · 2018-06-04T08:45:00Z

sklearn/linear_model/tests/test_ridge.py

@@ -153,6 +156,8 @@ def test_ridge_regression_convergence_fail():

 def test_ridge_sample_weights():
    # TODO: loop over sparse data as well
+    # Note: parametrizing this test with pytest results in failed


Weird, maybe you can create an issue about this, if you haven't already.

Opened #11200

qinhanmin2014 · 2018-06-04T14:28:21Z

@rth I've merged part3 and will give this a review these days. I think it's important to quickly get this in to avoid conflicts.
Couple of things:
(1) Need to merge master in.
(2) Could you please share how you did this? I think it's important for the completeness of the PR.

lesteve · 2018-06-04T14:45:51Z

I have to admit I have not followed closely the subdivision of this work. Ping me if you need another review to get this completed.

rth · 2018-06-04T18:12:22Z

Thanks for the review @lesteve - will open the issue!

@qinhanmin2014 I realized there was a PR duplication issue, but haven't though about it in terms or PR approval transitivity :) Thanks, everything should be in order now, this is the only PR for this parametrization, that remains.

Could you please share how you did this? I think it's important for the completeness of the PR.

You mean how I arrived at the diff in this PR? Well, some was already done in #8321 I then searched for tests using yield with the command from #10728 (comment) . A more detailed list of occurrences (as opposed to files) can be obtained on Linux with,

find sklearn -iname "*test_*py" -type f -exec grep -H --color -i yield {} \;

Then I realized that there were a lot of tests that would benefit from straightforward parametrization, so I manually went through tests - file by file and made the changes.

qinhanmin2014

Couple of questions, LGTM.

3420

qinhanmin2014 · 2018-06-05T01:13:43Z

sklearn/metrics/cluster/tests/test_common.py

@@ -126,7 +123,7 @@ def test_normalized_output(metric_name):
 # that is when 0 and 1 exchanged.
 @pytest.mark.parametrize(
    "metric_name",
-    [name for name in dict(SUPERVISED_METRICS, **UNSUPERVISED_METRICS)]


Someone has pointed out that dict(x, **y) is not recommended in part1. But I don't think we're going to modify parametrized tests too much, so LGTM.

qinhanmin2014 · 2018-06-05T01:22:40Z

sklearn/mixture/tests/test_gmm.py

-
-    assert_raises(ValueError, mixture.GMM, n_components=20,
-                  covariance_type='badcovariance_type')
+    with pytest.raises(ValueError):


Why getting rid of assert_raises here? I can't find any explanation. Also, I think there are many other places using assert_raises. Maybe revert it and open an issue if needed?

You are right - better be systematic. Reverted this.

qinhanmin2014 · 2018-06-05T01:44:21Z

sklearn/tree/tests/test_tree.py

-def test_1d_input():
-    for name in ALL_TREES:
-        yield check_raise_error_on_1d_input, name
+# XXX


What's happening here for the XXX?

It was probably a note in the making, don't remember now - possibly related to #11109 about incompatibility of ignore_warnings with parametrization. Thanks for catching this.

Removed it, and used ignore_warnings as a context manager. Hopefully this will be fixed in #11109

qinhanmin2014 · 2018-06-05T03:46:40Z

Hmm, @rth I still get these in Appveyor. I think they're valid.

sklearn/metrics/tests/test_common.py::test_single_sample
  yield tests are deprecated, and scheduled to be removed in pytest 4.0
sklearn/metrics/tests/test_common.py::test_averaging_multiclass
  yield tests are deprecated, and scheduled to be removed in pytest 4.0
sklearn/metrics/tests/test_common.py::test_averaging_multilabel
  yield tests are deprecated, and scheduled to be removed in pytest 4.0
sklearn/metrics/tests/test_common.py::test_averaging_multilabel_all_zeroes
  yield tests are deprecated, and scheduled to be removed in pytest 4.0
sklearn/metrics/tests/test_common.py::test_averaging_multilabel_all_ones
  yield tests are deprecated, and scheduled to be removed in pytest 4.0
sklearn/metrics/tests/test_common.py::test_sample_weight_invariance
  yield tests are deprecated, and scheduled to be removed in pytest 4.0

I won't be strict about the completeness of parametrization, but I think we should remove all the yield tests here.

rth · 2018-06-05T20:56:33Z

Thanks for the detailed review @qinhanmin2014 !

but I think we should remove all the yield tests here.

You are right, looks like I missed parametrizations in sklearn/metrics/tests/test_common.py file. Will add it tomorrow.

rth · 2018-06-06T13:50:05Z

I added the missing parametrizations in dd89184 for metrics @qinhanmin2014 . Had to split a few tests to remove all yields.

qinhanmin2014

ping @lesteve to double check your +1 since the new diff is large.

qinhanmin2014 · 2018-06-07T02:08:48Z

sklearn/metrics/tests/test_classification.py

-                                 y_true, y_pred,
-                                 beta=beta, average=average)
-            assert_almost_equal(fbeta, 0)
+    p, r, f, s = assert_warns(UndefinedMetricWarning,


Seems a bit awkward since this part will run multiple times. I'd rather have another test here (e.g.,test_precision_recall_f1_no_labels_average_none), or some better ways to avoid running same tests for multiple times.

Good point done.

qinhanmin2014 · 2018-06-07T02:17:12Z

sklearn/metrics/tests/test_common.py

+        'name',
+        (set(ALL_METRICS) - set(REGRESSION_METRICS)
+         - set(METRICS_WITHOUT_SAMPLE_WEIGHT)
+         - METRIC_UNDEFINED_BINARY_MULTICLASS))


Adding set to keep consistent? I'm fine with current version though.

Yes, this is a bit awkward because ALL_METRICS and REGRESSION_METRICS are dicts, while METRICS_WITHOUT_SAMPLE_WEIGHT and all the rest are lists, and finally METRIC_UNDEFINED_BINARY_MULTICLASS is a set.

I changed the ones that were lists to sets which removes the need to cast them to set in parametrizations (since we are doing a number of set operations here).

rth · 2018-06-07T08:21:06Z

@lesteve could you please have a look to the last 2 commits? Thanks!

qinhanmin2014

LGTM, ping @lesteve

lesteve

LGTM, merging! Thanks a lot for working on this, this is really appreciated!

lesteve · 2018-06-08T12:45:34Z

sklearn/metrics/tests/test_common.py

-        if name in METRIC_UNDEFINED_BINARY_MULTICLASS:
-            continue
-
+    with ignore_warnings():


Just curious, did you find a problem with the interaction of ignore_warnings and parametrize? I bumped into some quirks as I indicated in #11151 (comment).

Yes, I think same things as what you mentioned in your comment, if I recall correctly,

ignore_warning (as decorator) + parametrize failed on Python 2

ignire_warning (as decorator or not) + pytest.mark.filterwarnings didn't seem to work (but that might be fixed in [MRG+2] catch more expected warnings in common tests #11151)

rth · 2018-06-09T08:34:01Z

Thanks for the review @qinhanmin2014 and @lesteve ! In this case reviewing was probably more work than making the changes :)

HolgerPeters · 2018-06-19T12:25:49Z

@rth thanks for picking up my branch, if I had known someone wanted to work with it I would have left it in a better state :)

HolgerPeters and others added 14 commits February 8, 2017 22:48

Migrate to pytest

070ca35

Replace nosetests with pytest on travis

3d95262

Makefile to pytest

607f1b9

Merge branch 'master' into pytest-parametrize-non-common-tests

c99041d

Fix conflicts and add a few more parametrizations

86d4908

More pytest parametrizations

85fe86a

Parametrize test_forest.py

b51cc6a

Migrate test_gradient_boosting.py

e720097

More parametrization

f7d092a

Replace last yields in tests

4140c99

More test parametrization

c4663f6

Merge remote-tracking branch 'upstream/master' into pytest-parametriz…

978a5f9

…e-non-common-tests

More parametrizations

639488c

Pytest parametrize sklearn.ensemble

75e4292

This was referenced May 7, 2018

[WIP] Pytest test suite completely independent of nose #8321

Closed

[MRG+1] Pytest parametrize unit tests (part 1) - ensemble module #11075

Merged

rth added 2 commits May 7, 2018 17:52

Fix CI

a060efa

Fix CI

f96210a

rth mentioned this pull request May 7, 2018

[MRG+1] MAINT Parametrize common estimator tests with pytest #11063

Merged

rth added 3 commits May 8, 2018 12:21

Ensure sklearn/utils/testing.py doesn't import pytest

a581f55

Don't use dict(x, **y)

501bae7

Remove skip decorators for nose from sklearn/utils/testing.py

0bc2ccb

Merge branch 'pytest-parametrize-part1' into pytest-parametrize-non-c…

fbfb976

…ommon-tests

rth force-pushed the pytest-parametrize-non-common-tests branch from 554902b to fbfb976 Compare May 24, 2018 08:02

rth added 4 commits May 24, 2018 10:27

Fix Py2 compatibility in test_mldata tmpdir fixture

4f4c543

Fix Py2 compatibility in test_mldata tmpdir fixture

b4b15d1

Merge branch 'master' into pytest-parametrize-non-common-tests

848774a

Merge branch 'pytest-parametrize-non-common-tests' of github.com:rth/…

fb4a8cf

…scikit-learn into pytest-parametrize-non-common-tests

rth added 2 commits June 1, 2018 08:57

Merge branch 'master' into pytest-parametrize-non-common-tests

2cf2486

PEP8

75f16c5

rth changed the title ~~[WIP] Pytest parametrize unit tests~~ [MRG] Pytest parametrize unit tests Jun 1, 2018

lesteve reviewed Jun 4, 2018

View reviewed changes

lesteve changed the title ~~[MRG] Pytest parametrize unit tests~~ [MRG+1] Pytest parametrize unit tests Jun 4, 2018

Merge branch 'master' into pytest-parametrize-non-common-tests

a9f5e81

rth mentioned this pull request Jun 4, 2018

Instability in test_ridge.py::test_ridge_sample_weights #11200

Closed

qinhanmin2014 approved these changes Jun 5, 2018

View reviewed changes

Hanmin's comments

645fcd5

More parametrizations in sklearn/metrics/tests/

dd89184

qinhanmin2014 approved these changes Jun 7, 2018

View reviewed changes

Review comments and use sets in metrics/tests/test_common.py

588fb49

qinhanmin2014 approved these changes Jun 7, 2018

View reviewed changes

qinhanmin2014 changed the title ~~[MRG+1] Pytest parametrize unit tests~~ [MRG+2] Pytest parametrize unit tests Jun 7, 2018

lesteve reviewed Jun 8, 2018

View reviewed changes

lesteve merged commit e8d8b8e into scikit-learn:master Jun 8, 2018

rth deleted the pytest-parametrize-non-common-tests branch June 8, 2018 15:37

jnothman mentioned this pull request Jun 9, 2018

yield tests are deprecated in pytest #10728

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+2] Pytest parametrize unit tests #11074

[MRG+2] Pytest parametrize unit tests #11074

[MRG+2] Pytest parametrize unit tests #11074

[MRG+2] Pytest parametrize unit tests #11074

Conversation

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment