[MRG] Make test execution on CI possible through py.test command. by kdexd · Pull Request #433 · joblib/joblib

kdexd · 2016-11-30T11:33:26Z

Checkpoint PR for #411 ( Succeeding PR #430 )

This PR makes it possible to run tests through py.test command instead of nosetests on both Appveyor and Travis CI. Suitable changes have been made in config files of both CIs as well as the Makefile.
Three tests in test_parallel.py failed ( #411 (comment) ) as pytest doesn't intend to support yield based tests very well. Those tests now have sequential assertion written within, and a TODO comment is added for parametrization in next phase.

Future Roadmap:

By the merge of this PR we reach a checkpoint where the bare minimum setup atleast runs by issuing py.test command. Hence we hit a safe hard bottom that our migration won't break beneath this. Although current tests are written by keeping the nosetests flavor in mind, and the whole flow can be really simplified by using pytest fixtures, markers and parametrizations.
Empty out joblib/test/common.py into two files - joblib/testing.py and joblib/test/conftest.py. Session wide setup/ teardowns if necessary will be included in this file as it is interpreted by pytest in its own way. Certain decorators can be registered as pytest markers.
Parametrize all yield based tests module by module. I will keeping on sequentially opening PRs where I will completely ✨ pytestify ✨ each module. Although the pattern will be same, all imports from joblib.testing...

Work left in this PR:

The command python setup.py test still runs tests with nosetests. Need to replace it.
how to run doctests with py.test (seems like they are not run at the moment) and make sure that doctests in doc .rst files are run as well.
run git grep nose, to see whether there is any lingering nose stuff.

lesteve · 2016-11-30T13:39:09Z

[This section will be removed after I complete this task and edit the PR description].

Do you know you can use task lists in Github Flavored Markdown: https://github.com/blog/1375-task-lists-in-gfm-issues-pulls-comments?

lesteve

I know it is a WIP, but still a few comments.

lesteve · 2016-11-30T14:12:13Z

.travis.yml

-    - PYTHON_VERSION="2.7" NUMPY_VERSION="1.7"
-    - PYTHON_VERSION="3.3" NUMPY_VERSION="1.8"
-    - PYTHON_VERSION="3.4" NUMPY_VERSION="1.9"
+    - PYTHON_VERSION="2.6" NUMPY_VERSION="1.6" PYTEST_VERSION="2.6.3"


Any particular reason why you want to test with different py.test versions?

pytest 2.9.1 didn't go well with Python 2.6. I'll report back after some experimenting.

I see that you have reverted your changes, but don't worry about Python 2.6 we are going to drop support for it pretty soon.

lesteve · 2016-11-30T14:12:58Z

appveyor.yml

  # installed library.
  - "cd C:\\"
-  - "python -c \"import nose; nose.main()\" -v -s joblib"
+  - "py.test --disable-pytest-warnings --pyargs joblib"


Is the --disable-pytest-warnings needed/recommended on Windows?

pytest > 3.0.0 raises more than 300 pytest warnings as yield based tests have been officially deprecated. Since they're expected to arise, and a long list messes up the CI log, I have disable them for a while, now from both Windows and Linux. I'll remove this once I parametrize yield based tests in subsequent PRs.

Now that you have it in setup.cfg do you need it here?

Right, not needed here. Removing.

--disable-pytest-warnings flag is not available in older pytest versions. For python 2.7,, installing pytest without version specified installs 2.7.2. As the pytest warnings are anyhow going to be cleared out later, I'm not adding this right now. We'll think of enforcing a version > 2.8 once Python 2.6 support is dropped...

lesteve · 2016-11-30T14:13:06Z

appveyor/requirements.txt

@@ -4,3 +4,4 @@
 numpy==1.9.2
 nose


Remove nose

Still valid.

Actually my bad, we need that because we still have a nose dependency.

It's so frustrating: nose is only needed for the with_setup. We need to find a way around this.

lesteve · 2016-11-30T14:13:28Z

appveyor/requirements.txt

 numpy==1.9.2
 nose
 wheel
+pytest==3.0.4


I would say, just use the latest one i.e. without version number

lesteve · 2016-11-30T14:13:37Z

continuous_integration/install.sh

    #   - for numpy, NUMPY_VERSION is used
    #   - for scikit-learn, SCIKIT_LEARN_VERSION is used
-    TO_INSTALL_ALWAYS="pip nose"
+    TO_INSTALL_ALWAYS="pip nose "


You can remove nose here.

lesteve · 2016-11-30T14:14:10Z

continuous_integration/install.sh

+    TO_INSTALL_ALWAYS="pip nose "
    REQUIREMENTS="$TO_INSTALL_ALWAYS"
-    TO_INSTALL_MAYBE="python numpy flake8"
+    TO_INSTALL_MAYBE="pytest python numpy flake8"


Same question as above about why you need to try different pytest versions.

I don't know the CI was unhappy initially. Once it became green, I opened a PR fr further discussions. Although I am still trying to settle at a proper version.

lesteve · 2016-11-30T14:23:34Z

joblib/test/test_my_exceptions.py

 Test my automatically generate exceptions
 """
 from joblib import my_exceptions
+from _pytest.assertion.util import BuiltinAssertionError


Are all these changes really needed?

Yes, it raised an AssertionError as JoblibAssertionError is not an instance of _pytest.assertion.util.AssertionError. While running the test with pytest, we have to reference the original AssertionError shipped by core python like this.

I don't get any error while running joblib/test/test_my_exceptions.py in master so I don't get why this is needed. Maybe it is needed for a specific OS + python version + pytest version combination?

@lesteve I fixed it just to make sure it doesn;t happen anywhere else. This happens on my machine:

I came to know by this that pytest has its own variant of AssertionError.

OK this happens for Python 2.7 fair enough. To be honest, I would say just make the test simpler by just using another kind of exception for example ImportError.

Oh okay. I thought AssertionError as a testcase must be existing due to some corner case.

lesteve · 2016-11-30T14:26:07Z

joblib/test/test_parallel.py

-            yield check_backend_context_manager, test_backend
+            # TODO: parametrize this block later
+            # yield check_backend_context_manager, test_backend
+            check_backend_context_manager(test_backend)


OK so this is the actual fix for the tests failing with py.test in master. No clue why this yield does not work but the other yield does not seem to cause any issue.

I pushed this fix in master so we can already start using py.test locally. The downside is that you need to rebase on master to fix the conflict.

I've rebased the branch, thanks!

lesteve · 2016-11-30T14:27:39Z

setup.cfg

 [bdist_rpm]
 doc-files = doc

-[nosetests]


nose was use for coverage you can not remove this like this. This is probably why the coveralls status is shown as waiting for result.

Also it would be good to make sure that doctests are tested.

Oh okay, I have restored this back.

What I meant (just in case it wasn't clear): you need to figure out how to generate a coverage report with py.test.

Sorry for the noise, I see you have done something like this. The end goal would be to make sure we have some coverall status in this PR.

Yes, @lesteve I don't mind the noise anyways.. Actually I am enjoying this discussion for getting the PR merged. 🍀

kdexd · 2016-12-01T09:12:26Z

@lesteve: nose still has to remain as a requirement due to its with_setup functionality (just this one !). pytest does not have such mechanism, it has markers, so it needs a change in design pattern of certain decorators. I guess I will be completely removing nose in the last PR. Right now I am making sure that tests run uniformly with one suitable version of pytest locally as well as on both CIs. I am afraid if I even try to touch that, how much this whole diff would get confusing 😅 I hope it is okay.

I will try to enable doctests and shift coveralls through pytest though.

Do you know you can use task lists in Github Flavored Markdown

I used it in the issue comment, so funny why didn't this strike me right now! I'll go forward and amend the description, thanks !

[EDIT]: I read the modified description after writing this comment, thanks for editing the description for me !

lesteve · 2016-12-01T10:39:44Z

setup.cfg

 doc-files = doc

+[wheel]
+universal=1


Why is this necessary?

Strange, I don't remember writing this line. I'll revert this back.

Oh okay, this section existed below. It just got reordered when I restored [nosetests] section back.

lesteve · 2016-12-01T13:04:45Z

setup.cfg

+    --cov="joblib"
+    --disable-pytest-warnings
+    --doctest-glob="*.rst"
+    --doctest-modules


Is there an equivalent of doctest-fixtures from nosetests? AFAIR this is something that gets executed before the .rst doctest are run.

Well there should be, I'll check it out..

kdexd · 2016-12-01T16:40:10Z

@lesteve I am not sure why this one test fails in multiprocessing enabled mode...

lesteve · 2016-12-01T17:00:20Z

@lesteve I am not sure why this one test fails in multiprocessing enabled mode...

Hmmm not sure, maybe related to #413. I'll restart the build to see whether it goes away.

lesteve · 2016-12-01T18:08:02Z

@lesteve I am not sure why this one test fails in multiprocessing enabled mode...

Hmmm not sure, maybe related to #413. I'll restart the build to see whether it goes away.

It seems to be failing consistently. I can not reproduce the failure locally though.

Also if you could disable the warnings in setup.cfg as you were doing at one point it would be great. This way the Travis log would be a bit less verbose.

kdexd · 2016-12-02T04:55:59Z

Hi @lesteve: I have done all the needful work required in this PR. I guess only the CI has to be made green, and work on any review comments you make.. Appveyor is failing on python setup.py bdist_wheel which looks strange to me, and Travis CI passes on all jobs except that one. (#413)

lesteve · 2016-12-06T21:49:58Z

Hmm I rebased on master to solve the conflicts and it seems like we lost coveralls in the process. @karandesai-96 I remember it was happening before did you do something specific to get it to work?

kdexd · 2016-12-07T04:30:26Z

Hi @lesteve:
The commit ordering at the end was a little bit interchanged so I rebased locally once more, I was hoping that triggering a fresh build would do the needful, as in the earlier case, adding pytest-cov dependency and --cov=joblib argument restored back coveralls.

Interestingly I came across this issue and I see people are complaining regularly...

kdexd · 2016-12-07T04:33:38Z

@lesteve and another issue was opened just 9 hours ago, just around the time when our PR went off on Coveralls: lemurheavy/coveralls-public#892

Apparently we won't have to worry !

kdexd · 2016-12-07T04:35:58Z

Oh, just when I was typing the previous comment, Coveralls passed ! 🎉

kdexd · 2016-12-07T06:13:31Z

@GaelVaroquaux I have already made some commits on a separate branch and removed nose completely. The thing is, its diff is comparable to the current PR and this PR comments have grown too much so I have decided to keep that branch separate and make a PR within ten minutes after this one is merged. 😄

lesteve · 2016-12-07T07:32:25Z

OK great let's merge this one!

@GaelVaroquaux trying to separate the work into meaningful chunks in order to ease the migration and the review process.

From this PR onwards, you are supposed to run the tests with py.test. nose will be completely removed eventually.

lesteve · 2016-12-07T07:33:25Z

Great stuff @karandesai-96, keep up the good work!

* commit '0.10.2-55-g660fe5d': (52 commits) MAINT simplify way to skip doctests Do not print frames with IPython internals in format_stack BF(TST): compare for version in pickled filename ignoring full path, and looking for _ as well (joblib#445) [MRG] Remove nose dependency completely. (joblib#441) MAINT add Python 3.5 in setup.py Remove some Python 2.6 specific code [MRG] Make test execution on CI possible through py.test command. (joblib#433) Remove more python 2.6 related code (joblib#440) [MRG] Use commit range for diff in flake8_diff.sh (joblib#439) MAINT remove support for python 2.6 (joblib#437) Improve doc about caching methods (joblib#432) Get the tests passing with py.test MAINT update flake8_diff.sh COSMIT move stdlib imports where they belong [MRG] Replace assert_* methods with "assert" keyword statements. (joblib#430) Reraise when the exception is not a TransportableException (joblib#429) Make sure LICENSE.txt is included in the wheel Add LICENSE.txt Update doc/paralle.rst [MRG+1] Remove all nose imports from test scripts. (joblib#422) ...

* releases: (52 commits) MAINT simplify way to skip doctests Do not print frames with IPython internals in format_stack BF(TST): compare for version in pickled filename ignoring full path, and looking for _ as well (joblib#445) [MRG] Remove nose dependency completely. (joblib#441) MAINT add Python 3.5 in setup.py Remove some Python 2.6 specific code [MRG] Make test execution on CI possible through py.test command. (joblib#433) Remove more python 2.6 related code (joblib#440) [MRG] Use commit range for diff in flake8_diff.sh (joblib#439) MAINT remove support for python 2.6 (joblib#437) Improve doc about caching methods (joblib#432) Get the tests passing with py.test MAINT update flake8_diff.sh COSMIT move stdlib imports where they belong [MRG] Replace assert_* methods with "assert" keyword statements. (joblib#430) Reraise when the exception is not a TransportableException (joblib#429) Make sure LICENSE.txt is included in the wheel Add LICENSE.txt Update doc/paralle.rst [MRG+1] Remove all nose imports from test scripts. (joblib#422) ...

lesteve reviewed Nov 30, 2016

View reviewed changes

kdexd force-pushed the rewrite-pytest-failing-tests branch 3 times, most recently from b89f864 to 81e4322 Compare December 1, 2016 09:03

lesteve reviewed Dec 1, 2016

View reviewed changes

kdexd force-pushed the rewrite-pytest-failing-tests branch from bf9fe4f to 45a818d Compare December 1, 2016 12:40

lesteve reviewed Dec 1, 2016

View reviewed changes

kdexd force-pushed the rewrite-pytest-failing-tests branch 10 times, most recently from 9408639 to 48457c4 Compare December 1, 2016 16:35

kdexd force-pushed the rewrite-pytest-failing-tests branch 6 times, most recently from 5a201c7 to aac94d0 Compare December 2, 2016 04:45

kdexd force-pushed the rewrite-pytest-failing-tests branch from aac94d0 to 8a31b2b Compare December 2, 2016 07:18

lesteve force-pushed the rewrite-pytest-failing-tests branch 2 times, most recently from 2c5aa88 to b68e69e Compare December 6, 2016 18:22

Karan Desai and others added 14 commits December 7, 2016 08:30

Modify test_backend_context_manager to work with pytest.

aec8b05

Modify test__mk_exception to work with pytest.

7410df2

Provide a python setup.py test command for py.test

0f6d888

Update README.rst with new py.test commands.

2aac81f

Modify appveyor and travis configs for pytest runs.

1a925b8

Provide fixtures to doctests from a conftest file.

4383bc9

Replace nosetests section with pytest in setup.cfg

c98b9af

Reduce usage of nose wherever possible at this stage.

b52ee6e

Fix CI PermissionError- use tmpdir fixture in test_pool.py

e46e9cb

Add temporary skip marker to test_nested_parallel_warnings.

73ad11d

Cleanup test_script.sh and Makefile.

8ac5e0f

Replace few yield statements with simple assert in test_memory.

9f5af3a

Add notes in CHANGES.rst, remove unused variables from joblib.testing

aa90a9c

Tweak wording in changelog

78b61eb

kdexd force-pushed the rewrite-pytest-failing-tests branch from b68e69e to 78b61eb Compare December 7, 2016 03:02

lesteve merged commit 10c7639 into joblib:master Dec 7, 2016

kdexd deleted the rewrite-pytest-failing-tests branch December 7, 2016 07:45

kdexd mentioned this pull request Dec 7, 2016

[MRG] Remove nose dependency completely from project. #441

Merged

2 tasks

kdexd mentioned this pull request Feb 2, 2017

Move to pytest? scikit-learn/scikit-learn#7319

Closed

Conversation

kdexd commented Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checkpoint PR for #411 ( Succeeding PR #430 )

Future Roadmap:

Work left in this PR:

Uh oh!

lesteve commented Nov 30, 2016

Uh oh!

lesteve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdexd Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdexd commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kdexd commented Nov 30, 2016 •

edited

Loading

kdexd Dec 1, 2016 •

edited

Loading

kdexd commented Dec 1, 2016 •

edited

Loading

lesteve Dec 1, 2016 •

edited

Loading

lesteve commented Dec 6, 2016 •

edited

Loading

kdexd commented Dec 7, 2016 •

edited

Loading