From 73a348d1780d2c04faef23bc038c11ad8f91f2ff Mon Sep 17 00:00:00 2001 From: Roman Yurchak Date: Thu, 3 May 2018 15:22:19 +0300 Subject: [PATCH 1/6] Parametrize test_common.py with pytest --- sklearn/tests/test_common.py | 52 +++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 12 deletions(-) diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py index a2f17da487790..fc0e8c0e49ffa 100644 --- a/sklearn/tests/test_common.py +++ b/sklearn/tests/test_common.py @@ -13,6 +13,8 @@ import re import pkgutil +import pytest + from sklearn.utils.testing import assert_false, clean_warning_registry from sklearn.utils.testing import all_estimators from sklearn.utils.testing import assert_equal @@ -49,26 +51,52 @@ def test_all_estimators(): # properly assert_greater(len(estimators), 0) - for name, Estimator in estimators: - # some can just not be sensibly default constructed - yield check_parameters_default_constructible, name, Estimator +@pytest.mark.parametrize( + 'name, Estimator', + all_estimators(include_meta_estimators=True) +) +def test_parameters_default_constructible(name, Estimator): -def test_non_meta_estimators(): - # input validation etc for non-meta estimators - estimators = all_estimators() - for name, Estimator in estimators: + # some can just not be sensibly default constructed + check_parameters_default_constructible(name, Estimator) + + +def _tested_non_meta_estimators(): + for name, Estimator in all_estimators(): if issubclass(Estimator, BiclusterMixin): continue if name.startswith("_"): continue + yield name, Estimator + + +def _carthesian_product_checks(check_generator, estimators): + for name, Estimator in estimators: estimator = Estimator() - # check this on class - yield check_no_attributes_set_in_init, name, estimator + for check in check_generator(name, estimator): + yield name, Estimator, check - for check in _yield_all_checks(name, estimator): - set_checking_parameters(estimator) - yield check, name, estimator + +@pytest.mark.parametrize( + "name, Estimator, check", + _carthesian_product_checks(_yield_all_checks, + _tested_non_meta_estimators()) +) +def test_non_meta_estimators(name, Estimator, check): + # input validation etc for non-meta estimators + estimator = Estimator() + set_checking_parameters(estimator) + check(name, estimator) + + +@pytest.mark.parametrize("name, Estimator", + _tested_non_meta_estimators()) +def test_no_attributes_set_in_init(name, Estimator): + # input validation etc for non-meta estimators + estimator = Estimator() + # check this on class + check_no_attributes_set_in_init(name, estimator) def test_configure(): From c1a9829fced03b48246a84ba602719c9353e9195 Mon Sep 17 00:00:00 2001 From: Roman Yurchak Date: Thu, 3 May 2018 17:20:49 +0300 Subject: [PATCH 2/6] Migrate test_class_weight_balanced_linear_classifiers and address Loic's comments --- sklearn/tests/test_common.py | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py index fc0e8c0e49ffa..742b09ffd220e 100644 --- a/sklearn/tests/test_common.py +++ b/sklearn/tests/test_common.py @@ -43,8 +43,6 @@ def test_all_estimator_no_base_class(): def test_all_estimators(): - # Test that estimators are default-constructible, cloneable - # and have working repr. estimators = all_estimators(include_meta_estimators=True) # Meta sanity-check to make sure that the estimator introspection runs @@ -57,8 +55,7 @@ def test_all_estimators(): all_estimators(include_meta_estimators=True) ) def test_parameters_default_constructible(name, Estimator): - - # some can just not be sensibly default constructed + # Test that estimators are default-constructible check_parameters_default_constructible(name, Estimator) @@ -71,7 +68,7 @@ def _tested_non_meta_estimators(): yield name, Estimator -def _carthesian_product_checks(check_generator, estimators): +def _cartesian_product_checks(check_generator, estimators): for name, Estimator in estimators: estimator = Estimator() for check in check_generator(name, estimator): @@ -80,11 +77,11 @@ def _carthesian_product_checks(check_generator, estimators): @pytest.mark.parametrize( "name, Estimator, check", - _carthesian_product_checks(_yield_all_checks, - _tested_non_meta_estimators()) + _cartesian_product_checks(_yield_all_checks, + _tested_non_meta_estimators()) ) def test_non_meta_estimators(name, Estimator, check): - # input validation etc for non-meta estimators + # Common tests for non-meta estimators estimator = Estimator() set_checking_parameters(estimator) check(name, estimator) @@ -123,19 +120,21 @@ def test_configure(): os.chdir(cwd) -def test_class_weight_balanced_linear_classifiers(): +def _tested_linear_classifiers(): classifiers = all_estimators(type_filter='classifier') clean_warning_registry() with warnings.catch_warnings(record=True): - linear_classifiers = [ - (name, clazz) - for name, clazz in classifiers + for name, clazz in classifiers: if ('class_weight' in clazz().get_params().keys() and - issubclass(clazz, LinearClassifierMixin))] + issubclass(clazz, LinearClassifierMixin)): + yield name, clazz + - for name, Classifier in linear_classifiers: - yield check_class_weight_balanced_linear_classifier, name, Classifier +@pytest.mark.parametrize("name, Classifier", + _tested_linear_classifiers()) +def test_class_weight_balanced_linear_classifiers(name, Classifier): + check_class_weight_balanced_linear_classifier(name, Classifier) @ignore_warnings From 23a01f3edadd530e141d34a83c8b5bb647eca92f Mon Sep 17 00:00:00 2001 From: Roman Yurchak Date: Fri, 4 May 2018 12:52:57 +0300 Subject: [PATCH 3/6] Document the pytest -k parameter --- doc/developers/contributing.rst | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/doc/developers/contributing.rst b/doc/developers/contributing.rst index 0d6174f3b2d48..c4a87dd80ae53 100644 --- a/doc/developers/contributing.rst +++ b/doc/developers/contributing.rst @@ -304,6 +304,21 @@ rules before submitting a pull request: $ make +* The full test suite takes fairly long to run. For faster iterations, + it is possibly to select a subset of tests to run using pytest selectors. + In particular, one can run a `single test based on its node ID + `_:: + + $ pytest -v sklearn/linear_model/tests/test_logistic.py::test_sparsify + + or use the `-k pytest parameter + `_ + to select tests based on their name. For instance,:: + + $ pytest sklearn/tests/test_common.py -v -k LogisticRegression + + will run all common tests for the ``LogisticRegression`` estimator. + * When adding additional functionality, provide at least one example script in the ``examples/`` folder. Have a look at other examples for reference. Examples should demonstrate why the new functionality is useful in From 136568ecad18b867cc76d64c0c49abe48ddb9dc2 Mon Sep 17 00:00:00 2001 From: Roman Yurchak Date: Fri, 4 May 2018 13:01:30 +0300 Subject: [PATCH 4/6] Move the -k pytest parameter docs to doc/developers/tips.rst --- doc/developers/contributing.rst | 15 --------------- doc/developers/tips.rst | 20 ++++++++++++++++++-- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/doc/developers/contributing.rst b/doc/developers/contributing.rst index c4a87dd80ae53..0d6174f3b2d48 100644 --- a/doc/developers/contributing.rst +++ b/doc/developers/contributing.rst @@ -304,21 +304,6 @@ rules before submitting a pull request: $ make -* The full test suite takes fairly long to run. For faster iterations, - it is possibly to select a subset of tests to run using pytest selectors. - In particular, one can run a `single test based on its node ID - `_:: - - $ pytest -v sklearn/linear_model/tests/test_logistic.py::test_sparsify - - or use the `-k pytest parameter - `_ - to select tests based on their name. For instance,:: - - $ pytest sklearn/tests/test_common.py -v -k LogisticRegression - - will run all common tests for the ``LogisticRegression`` estimator. - * When adding additional functionality, provide at least one example script in the ``examples/`` folder. Have a look at other examples for reference. Examples should demonstrate why the new functionality is useful in diff --git a/doc/developers/tips.rst b/doc/developers/tips.rst index 0c23bb74c8ed7..9a9ff1b2b99e9 100644 --- a/doc/developers/tips.rst +++ b/doc/developers/tips.rst @@ -64,8 +64,24 @@ will be displayed as a color background behind the line number. Useful pytest aliases and flags ------------------------------- -We recommend using pytest to run unit tests. When a unit tests fail, the -following tricks can make debugging easier: +We recommend using pytest to run unit tests. + +The full test suite takes fairly long to run. For faster iterations, +it is possibly to select a subset of tests using pytest selectors. +In particular, one can run a `single test based on its node ID +`_:: + + pytest -v sklearn/linear_model/tests/test_logistic.py::test_sparsify + +or use the `-k pytest parameter +`_ +to select tests based on their name. For instance,:: + + pytest sklearn/tests/test_common.py -v -k LogisticRegression + +will run all common tests for the ``LogisticRegression`` estimator. + +When a unit tests fail, the following tricks can make debugging easier: 1. The command line argument ``pytest -l`` instructs pytest to print the local variables when a failure occurs. From d246ba33a3d61bcb11a2b4c7cf76571ad3b912ce Mon Sep 17 00:00:00 2001 From: Roman Yurchak Date: Sun, 6 May 2018 22:45:45 +0300 Subject: [PATCH 5/6] Joel's comments --- doc/developers/tips.rst | 4 +--- sklearn/tests/test_common.py | 6 +++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/doc/developers/tips.rst b/doc/developers/tips.rst index 9a9ff1b2b99e9..7e97bcfb2a2c1 100644 --- a/doc/developers/tips.rst +++ b/doc/developers/tips.rst @@ -64,8 +64,6 @@ will be displayed as a color background behind the line number. Useful pytest aliases and flags ------------------------------- -We recommend using pytest to run unit tests. - The full test suite takes fairly long to run. For faster iterations, it is possibly to select a subset of tests using pytest selectors. In particular, one can run a `single test based on its node ID @@ -79,7 +77,7 @@ to select tests based on their name. For instance,:: pytest sklearn/tests/test_common.py -v -k LogisticRegression -will run all common tests for the ``LogisticRegression`` estimator. +will run all :term:`common tests` for the ``LogisticRegression`` estimator. When a unit tests fail, the following tricks can make debugging easier: diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py index 742b09ffd220e..b238ca317375e 100644 --- a/sklearn/tests/test_common.py +++ b/sklearn/tests/test_common.py @@ -68,7 +68,7 @@ def _tested_non_meta_estimators(): yield name, Estimator -def _cartesian_product_checks(check_generator, estimators): +def _combine_checks(check_generator, estimators): for name, Estimator in estimators: estimator = Estimator() for check in check_generator(name, estimator): @@ -77,8 +77,8 @@ def _cartesian_product_checks(check_generator, estimators): @pytest.mark.parametrize( "name, Estimator, check", - _cartesian_product_checks(_yield_all_checks, - _tested_non_meta_estimators()) + _combine_checks(_yield_all_checks, + _tested_non_meta_estimators()) ) def test_non_meta_estimators(name, Estimator, check): # Common tests for non-meta estimators From 69ebbebf2c7a9337e4b80eee97185cc3f86b9c31 Mon Sep 17 00:00:00 2001 From: Roman Yurchak Date: Mon, 7 May 2018 12:17:41 +0300 Subject: [PATCH 6/6] Rename _combine_checks -> _generate_checks_per_estimator --- sklearn/tests/test_common.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py index b238ca317375e..a0e7ff82ad430 100644 --- a/sklearn/tests/test_common.py +++ b/sklearn/tests/test_common.py @@ -68,7 +68,7 @@ def _tested_non_meta_estimators(): yield name, Estimator -def _combine_checks(check_generator, estimators): +def _generate_checks_per_estimator(check_generator, estimators): for name, Estimator in estimators: estimator = Estimator() for check in check_generator(name, estimator): @@ -77,8 +77,8 @@ def _combine_checks(check_generator, estimators): @pytest.mark.parametrize( "name, Estimator, check", - _combine_checks(_yield_all_checks, - _tested_non_meta_estimators()) + _generate_checks_per_estimator(_yield_all_checks, + _tested_non_meta_estimators()) ) def test_non_meta_estimators(name, Estimator, check): # Common tests for non-meta estimators