Pytest #1722

djarecka · 2016-11-26T03:20:07Z

I removed nose and unittest libraries; and rewrote the tests to pytest. I still kept mock library in a few tests. I tried to not change the tests coverage, they should tests exactly the same functionality. However, py.test stops execution after the first assert statement that fails.

This is still not completely finished:

there are some notes (search for "NOTE_dj") that should be removed, but just kept them for now to think and discuss.
there are 2 places that I marked as "FAIL", I had to comment them to not get Fails... not sure if I should change the tests or the tested function.
a few tests are marked as xfail, they were skipif(True) before.
there are tests that are always skipped (I used the test docker containers)
circle.yml runs run_pytests.py instead of run_nosetests.py, run_pytests.py needs to be improved

I also slightly adjusted docstrings so doctests with pytest can be run.

codecov-io · 2016-11-26T04:36:31Z

Current coverage is 71.11% (diff: 62.06%)

Merging #1722 into master will increase coverage by 0.01%

@@             master      #1722   diff @@
==========================================
  Files          1030       1028      -2   
  Lines         51927      50608   -1319   
  Methods           0          0           
  Messages          0          0           
  Branches       7346       7331     -15   
==========================================
- Hits          36919      35988    -931   
+ Misses        13883      13495    -388   
  Partials       1125       1125

Powered by Codecov. Last update 18468d1...7b00617

oesteban

This is a great effort! Thanks a lot!

oesteban · 2016-11-28T19:15:23Z

nipype/algorithms/tests/test_auto_NormalizeProbabilityMapSet.py

@@ -11,7 +10,7 @@ def test_NormalizeProbabilityMapSet_inputs():

    for key, metadata in list(input_map.items()):
        for metakey, value in list(metadata.items()):
-            yield assert_equal, getattr(inputs.traits()[key], metakey), value
+            assert getattr(inputs.traits()[key], metakey) == value


are these changes done in the tools/checkspecs.py file? If not, they will go away with make specs.

yes the tools/checkspecs.py has changed, all auto tests are generated by running python2 tools/checkspecs.py

I know this PR is almost impossible to read, I have another branch pytest_oldAutoTests (and try to keep updated) without new auto tests, so I can compare other changes:
https://github.com/djarecka/nipype/compare/master...djarecka:pytest_oldAutoTests?expand=1#diff-e661fcf16301968039679532e45d0224

oesteban · 2016-11-28T19:17:45Z

circle.yml

-        timeout: 2600
-    - docker run -v /etc/localtime:/etc/localtime:ro -e FSL_COURSE_DATA="/root/examples/nipype-fsl_course_data" -v ~/examples:/root/examples:ro -v ~/scratch:/scratch -w /root/src/nipype nipype/nipype_test:py27 /usr/bin/run_nosetests.sh py27 :
-        timeout: 5200
+#    - docker run -v /etc/localtime:/etc/localtime:ro -e FSL_COURSE_DATA="/root/examples/nipype-fsl_course_data" -v ~/examples:/root/examples:ro -v ~/scratch:/scratch -w /root/src/nipype nipype/nipype_test:py35 /usr/bin/run_nosetests.sh py35 :


This is actually disabling tests in CircleCI. Could you modify run_nosetests.sh (https://github.com/nipy/nipype/blob/master/docker/circleci/run_nosetests.sh) so that it calls py.test inside?

If you rename that file, make sure it starts with the prefix run_ so it is automatically added in the docker image.

yes, I know :) I was still modifying and testing on other branch without running all examples etc. (so it doesn't run 4h). I've just pushed a new version that runs run_pytests.sh (plus all examples; so let's see if it's ok)

my run_pytest.sh is far from being perfect, I'd need some help (left some notes within the file). Mostly I'm not sure if I should install pytest and other library using this file or docker containers might be changed. I also don't know how to recreate the codecov functionality: for now I removed xml files and testing pytest-cov.

satra · 2016-11-28T20:35:30Z

see: https://github.com/codecov/example-python

djarecka · 2016-11-28T20:44:58Z

@satra : Thanks, will read. In general, I'm using pytest-cov: https://github.com/djarecka/nipype/blob/pytest/docker/circleci/run_pytests.sh#L33
I'm just not sure about this xml files etc. For now I've removed codecov, but pytest should give a coverage report.

satra · 2016-12-03T17:36:08Z

.travis.yml

+  conda update --all -y python=$TRAVIS_PYTHON_VERSION pytest &&
+  pip install pytest-raisesregexp &&
+  pip install pytest-cov &&
+  pip install click && 


this is unnecessary - should happen with the requirements.txtfile.

satra · 2016-12-03T17:39:37Z

docker/circleci/run_pytests.sh

+pip install -U pytest
+pip install pytest-raisesregexp
+pip install pytest-cov
+pip install click


click is a nipype requirement. and so should pytest now. these should be updated in nipype/info.py and *requirements.txt

had to add click installation once I've started testing doctest. Had errors from nipype/scripts/cli.py and nipype/scripts/utils.py.
When I run containers on my laptop I do have an import error when I try import click

satra · 2016-12-03T17:43:30Z

nipype/algorithms/tests/test_tsnr.py


-class TestTSNR(unittest.TestCase):
+#NOTE_dj: haven't removed mock (have to understand better)


can remove this note - mock is used to simulate some interaction that may not exist on the system.

however if it's not used in a particular file, it should be removed.

I had this note, since I was thinking about trying to use pytest mock and monkeypatching instead of the mock library. I will remove the note and keep the library for now. Can try to change it in different PR, once I understand pytest better.

I haven't been super happy with the mock library, so if you do check out pytest mock, tell me what you think!

sure, I'll give a second try. Since I've never used any mocking or patching, I had problem with understanding how important the differences between the libraries might be for the project.
I will open a new PR for it, so it will be easier to review.

Cool--it's not a priority or anything :)

satra · 2016-12-03T22:21:33Z

@djarecka - this looks great and is almost there.

if you can:

merge master and
address any of the comments that require addressing, and
the tests pass,

we can merge it.

djarecka · 2016-12-07T23:52:06Z

@satra:
after you comments, I've changed:

updated various requirements files, including requirements.txt : removing nosetest, doctest-ignore-unicode and adding pytest; deleting run_nosetest.
updated Makefile, removed test-doc
changed description of the testing process within testing_nipype.rst
removed installation of pytest from travis and run_pytest.sh (travis and circle uses requiremnts.txt)
removed my comments that described my changes and some small concerns (might come back to them later)
removed import nipype.testing when numpy.testing methods are being used
changed skipif to use sys.version_info and not TRAVIS_PYTHON_VERSION, so it works also for make tests on a local machine

some issues that I still don't know how to solve (but I believe PR can me merged regardless):

the only comments I left are related to test failures (NOTE_dj, FAIL), two asserts are disabled, since they don't work, either the tests or the code has to be changed.
there is one doctest that in some situation might not work: https://github.com/djarecka/nipype/blob/pytest/nipype/interfaces/bru2nii.py#L45
Travis and Circle are ok, but the output might vary.
there is a file .coveragerc that is only used in Makefile. Should it be use also in travis and circle?
I didn't change install nosetest to install pytest in Dockerfiles, since I'm not sure if this should stay there (some dockerfiles have only install requirements.txt)
don't now what to do with nipype/fixes/numpy/testing/nosetester.py and /nipype/fixes/numpy/testing/noseclasses.py. can be just removed??
not sure about nipype/testing: most of the things are not used anymore. I can see only example_data, anatfile and funcfile
tools/report_coverage.py should be changed or removed (had problem running even the original version, so not sure if anybody cares about the file anymore)

satra · 2016-12-08T23:20:05Z

we should change the docker files as well.

djarecka · 2016-12-08T23:31:37Z

@satra: can remove pip install nose-cov doctest-ignore-unicode configparser from Dockerfile_base and Dockerfile_py27. Changing to py.test is not needed IMO, since they run pip install -r /root/src/nipype/requirements.txt (and Dockerfile_py35 doesn't have this part anyway).

Let me know, if you had something else/more in mind.

satra · 2016-12-08T23:32:32Z

i think configparser may need to stay for py27

…arametrize, fixture, skipif, shortening a few functions

…ere not tested by travis)

…structure

…ng asserts, fixture for creating files, some problems with exceptions

… but adding pdb.set_trace() or running py.test -s could change the output of the test

…st with and without output capturing should work

…ert, not yield asser_equal; all test_auto* were changed (running python2 tools/checkspec

…ype, have some errors/fails that could be related to yield)

…ircle run pip install requirements.txt first (circle is building new containers that depend on the current version of the file)

…t_equal from numpy is used; small cleaning of imports that are not used

… pytest testing framework

…-doc

…tiproc tests

djarecka · 2016-12-09T13:20:06Z

@satra all tests were passed after I changed Dockerfiles yesterday, but today I've discovered that my PR can't be merged automatically, since one new test was merged. I rebased again and running tests, but this PR will always have conflict whenever any single test will be changed on the master.

satra · 2016-12-09T14:49:57Z

@djarecka - we will merge this as soon as circle passes.

chrisgorgo · 2016-12-10T17:39:42Z

Hurray! Gratulacje!

…

On Dec 10, 2016 6:08 AM, "Satrajit Ghosh" ***@***.***> wrote: Merged #1722 <#1722>. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1722 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOkp5muSi-gdnlPt7GAOh6Sgtk9PrRbks5rGrJ1gaJpZM4K8y3Y> .

djarecka · 2016-12-11T01:23:42Z

@chrisfilo: :-)

satra · 2016-12-03T21:19:40Z

nipype/interfaces/freesurfer/tests/test_preprocess.py

 import nipype.interfaces.freesurfer as freesurfer


-def create_files_in_directory():
+@pytest.fixture()
+def create_files_in_directory(request):


can this fixture be creating in nipype.testing and imported from there. seems like this gets repeated in a couple of files.

it can be, let me do it in a separate PR, I will review some other fixtures. I believe this is not the only one that is often repeated

satra · 2016-12-03T21:23:39Z

nipype/interfaces/fsl/tests/test_BEDPOSTX.py

@@ -1,3 +1,6 @@
 # AUTO-GENERATED by tools/checkspecs.py - DO NOT EDIT
 from nipype.testing import assert_equal
 from nipype.interfaces.fsl.dti import BEDPOSTX
+
+#NOTE_dj: this test has only import statements
+#NOTE_dj: this is not a AUTO test!


this is ok. it is used only to ensure that the autotest is not written. the BEDPOSTX interface that's instantiated depends on the version of FSL installed in the system.

I've restored the file, but change the comment. However, I did not notice any new test_auto_BEDPOSTX .

checking this issue, I've discovered that I have one auto test that is untracked - nipype/algorithms/tests/test_auto_ACompCor.py. You don't have it?

satra · 2016-12-03T21:29:06Z

nipype/interfaces/fsl/tests/test_FILMGLS.py

@@ -46,11 +45,13 @@ def test_filmgls():
                      use_pava=dict(argstr='--pava',),
                      )
    instance = FILMGLS()
+    #NOTE_dj: don't understand this test: 
+    #NOTE_dj: IMO, it should go either to IF or ELSE, instance doesn't depend on any parameters


similar to BEDPOSTX, the specific FILMGLS class depends on the instance of FSL.

satra · 2016-12-03T21:29:41Z

nipype/interfaces/fsl/tests/test_XFibres.py

@@ -1,3 +1,5 @@
 # -*- coding: utf-8 -*-
 from nipype.testing import assert_equal
 from nipype.interfaces.fsl.dti import XFibres
+
+#NOTE_dj: this test contains  import statements only...


same as BEDPOSTX

satra · 2016-12-03T21:31:37Z

nipype/interfaces/fsl/tests/test_base.py

-@skipif(no_fsl)  # skip if fsl not installed)
+
+#NOTE_dj: a function, e.g. "no_fsl" always gives True, shuld always change to no_fsl in pytest skipif
+#NOTE_dj: removed the IF statement, since skipif is used? 


looks good.

satra · 2016-12-03T22:06:01Z

nipype/interfaces/tests/test_io.py



 # Test AWS creds read from env vars
-@skipif(noboto3 or not fakes3)
+#NOTE_dj: noboto3 and fakes3 are not used in this test, is skipif needed for other reason?
+@pytest.mark.skipif(noboto3 or not fakes3, reason="boto3 or fakes3 library is not available")


the test should not be run if boto or fakes3 are not available. they don't have to be used in the test.

satra · 2016-12-03T22:06:30Z

nipype/interfaces/tests/test_io.py

-def test_jsonsink():
-    import simplejson
-    import os
+#NOTE_dj: I split the test_jsonsink, didn't find connection between two parts, could easier use parametrize for the second part


satra · 2016-12-03T22:07:58Z

nipype/interfaces/tests/test_nilearn.py

+#NOTE_dj: can we change the imports, so it's more clear where the function come from
+#NOTE_dj: in ...testing there is simply from numpy.testing import * 
+from ...testing import utils
+from numpy.testing import assert_almost_equal 


satra · 2016-12-03T22:08:32Z

nipype/interfaces/tests/test_nilearn.py

 from .. import nilearn as iface
 from ...pipeline import engine as pe

+import pytest, pdb


is pdb being used?

satra · 2016-12-03T22:11:09Z

nipype/pipeline/engine/tests/test_engine.py

 from ... import engine as pe
 from ....interfaces import base as nib

+#NOTE_dj: I combined some tests that didn't have any extra description
+#NOTE_dj: some other test can be combined but could be harder to read as examples of usage


satra · 2016-12-15T04:11:03Z

@djarecka - i can't find the comments you are posting for some reason - but i receive emails. auto-tests are only generated when a corresponding named test is not available. test_auto_xyz.py will only be created if test_xyz.py does not exist. such custom tests often, but not always, start from an auto test and then modify it.

djarecka · 2016-12-15T04:30:56Z

@satra : my point was that after removing test_XFibres.py and test_BEDPOSTX.py I could not see any differences in auto tests. If you still expect that some new auto tests should be created when these 2 files are missing I can explore it later.

And independent question was about nipype/algorithms/tests/test_auto_ACompCor.py. it looks like checkspecs.py generates this file for me (it's independent on test_XFibres.py and test_BEDPOSTX.py files). Was wondering if you don't have this file. It is not tracked by git.

djarecka · 2016-12-15T19:19:13Z

@satra : the reason why I haven't seen any difference after removing test_XFibres.py and test_BEDPOSTX.py, i.e. no new auto tests were created, is here https://github.com/nipy/nipype/blob/master/nipype/interfaces/fsl/dti.py#L428
checkspecs.py consider XFibres5 and BEDPOSTX5 as valid classes, and not XFibres and BEDPOSTX.
The auto tests for XFibres5 and BEDPOSTX5 have been created long time ago. They work, but if you want to remove them, I can create test_BEDPOSTX5.py and test_XFibres5.py and remove the auto tests.

oesteban reviewed Nov 28, 2016

View reviewed changes

djarecka changed the title ~~[WIP] Pytest~~ Pytest Dec 2, 2016

satra reviewed Dec 3, 2016

View reviewed changes

djarecka force-pushed the pytest branch 2 times, most recently from 6f2f9cb to 112ffc3 Compare December 7, 2016 22:21

djarecka added 15 commits December 9, 2016 07:23

pytest version of test_utility

c00c058

removing nose from travis, testing already rewritten tests using pytest

191fb56

changing tests: removing yield and nose library, introducing pytest p…

6220e40

…arametrize, fixture, skipif, shortening a few functions

removing extra imports within functions

0f0ebb7

the simplest change to py.test; all class structure is the same

a618b18

changing import statements

ac320a8

adding previously changed tests to pytest (forgot to do it, so they w…

88c461c

…ere not tested by travis)

removing nose/unittest library and adding py.test; didn't change the …

0e4425a

…structure

changing tests in test_base to py.test: removing nose library, changi…

0126f0c

…ng asserts, fixture for creating files, some problems with exceptions

small editing in various files

93aa0f5

testing py.test and the test_bunch_hash; I don't understand behavior,…

b57cda4

… but adding pdb.set_trace() or running py.test -s could change the output of the test

checking if the object is a string and a path in get_bunch_hash; pyte…

1153383

…st with and without output capturing should work

changing generator of auto-tests, tools/checkspecs.py, so it uses ass…

858afcc

…ert, not yield asser_equal; all test_auto* were changed (running python2 tools/checkspec

testing all interface/tests using py.test (still doesnt check all nip…

ac1f6ac

…ype, have some errors/fails that could be related to yield)

removing assert_equal and imports that are not used

1a42568

djarecka added 17 commits December 9, 2016 07:25

removing install pytest from travis and run_pytest; both travis and c…

f39597b

…ircle run pip install requirements.txt first (circle is building new containers that depend on the current version of the file)

removing the comments that described my changes

8f6c1e1

removing tests that contained import statements only

551d913

applying my suggestions from my notes

ea479bc

removing my notes regarding further improvements or my small concerns

3e8bb7c

removing notes/questions from run_pytest

7af2eca

removing run_nosetest script

dec6e3c

explicitly importing numpy.testing where assert_equal or assert_almos…

9796bad

…t_equal from numpy is used; small cleaning of imports that are not used

removing some remnants of nose in test functions or docstrings

f833630

updating rtd_requirements.txt and doc/devel/testing_nipype.rst to the…

1a63a7a

… pytest testing framework

changing Makefile: updating test-code and test-coverage, removed test…

9efd32c

…-doc

checking sys.version_info instead of TRAVIS_PYTHON_VERSION in the mul…

65d1223

…tiproc tests

fixing test_confounds.py after rebasing

97689ae

all test with plugin= has been mark as skipped until the issue is solved

043be21

fixing the last commit

01f4ed8

removing pip install nose from Dockerfiles

7a2fdb5

removing nose from new tests after rebasing

7b00617

djarecka force-pushed the pytest branch from ab5188b to 7b00617 Compare December 9, 2016 12:58

satra merged commit 8ff1851 into nipy:master Dec 10, 2016

satra reviewed Dec 13, 2016

View reviewed changes

djarecka mentioned this pull request Dec 16, 2016

Pytest follow-up #1751

Merged

djarecka mentioned this pull request Mar 22, 2017

Codecov submission is not enabled on Travis #1899

Closed

shnizzedy mentioned this pull request Mar 25, 2021

REF: 👷 Migrage CI to GitHub Actions shnizzedy/mindboggle#6

Merged

6 tasks


		class TestTSNR(unittest.TestCase):
		#NOTE_dj: haven't removed mock (have to understand better)

Pytest #1722

Pytest #1722

Uh oh!

Conversation

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

Uh oh!

Uh oh!

codecov-io commented Nov 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 71.11% (diff: 62.06%)

Uh oh!

oesteban 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!

djarecka Nov 28, 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!

satra commented Nov 28, 2016

Uh oh!

djarecka commented Nov 28, 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

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!

satra commented Dec 3, 2016

Uh oh!

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

Uh oh!

Uh oh!

satra commented Dec 8, 2016

Uh oh!

djarecka commented Dec 8, 2016

Uh oh!

satra commented Dec 8, 2016

Uh oh!

djarecka commented Dec 9, 2016

Uh oh!

satra commented Dec 9, 2016

Uh oh!

chrisgorgo commented Dec 10, 2016 via email

Uh oh!

djarecka commented Dec 11, 2016

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!

djarecka commented Nov 26, 2016 •

edited

Loading

codecov-io commented Nov 26, 2016 •

edited

Loading

djarecka Nov 28, 2016 •

edited

Loading

djarecka commented Nov 28, 2016 •

edited

Loading

djarecka commented Dec 7, 2016 •

edited

Loading