pytest cleaning #2252
pytest cleaning #2252
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2252 +/- ##
==========================================
+ Coverage 72.32% 72.32% +<.01%
==========================================
Files 1183 1183
Lines 59008 59008
Branches 8490 8490
==========================================
+ Hits 42677 42679 +2
+ Misses 14947 14945 -2
Partials 1384 1384
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #2252 +/- ##
=========================================
+ Coverage 72.37% 72.5% +0.12%
=========================================
Files 1188 1188
Lines 59184 59131 -53
Branches 8506 8505 -1
=========================================
+ Hits 42837 42872 +35
+ Misses 14961 14873 -88
Partials 1386 1386
Continue to review full report at Codecov.
|
| @@ -43,7 +43,7 @@ def test_dvars(tmpdir): | |||
| in_mask=example_data('ds003_sub-01_mc_brainmask.nii.gz'), | |||
| save_all=True, | |||
| intensity_normalization=0) | |||
| os.chdir(str(tmpdir)) | |||
| os.chdir(tmpdir.strpath) | |||
There was a problem hiding this comment.
tmpdir is a py.path.local object which has a lot of methods:
http://py.readthedocs.io/en/latest/path.html#py-path-local-local-file-system-path
There was a problem hiding this comment.
true, you can do tmpdir.chdir() here 👍
| @@ -17,7 +17,7 @@ | |||
|
|
|||
|
|
|||
| def test_modelgen1(tmpdir): | |||
| tempdir = str(tmpdir) | |||
| tempdir = tmpdir.strpath | |||
| filename1 = os.path.join(tempdir, 'test1.nii') | |||
There was a problem hiding this comment.
so this could be simply tmpdir.join('test.nii')
There was a problem hiding this comment.
ok, can try to find those places automatically
There was a problem hiding this comment.
but py.path.local won't work everywhere, I'm recalling that I did have some problems with it when I was converting tests to pytest, and there were places where string worked better. I can see that that at least for nibabel.save I'll have to keep strings.
There was a problem hiding this comment.
i meant places where the methods of py.path.local make sense, such as join. it's not a direct replacement for a string in all cases.
There was a problem hiding this comment.
my only point was that this test will not work if I change join part to tmpdir.join('test.nii'), it has to be string.
Can always do tmpdir.join('test.nii').strpath if this is preferred (included in the last commit).
|
For the doctests maybe a would work out. Worth trying. |
|
@oesteban that might be the best solution. Was trying to find something for doctests only, but haven't found yet. but would prefer to keep the docstrings as short as possible. btw. should we use |
|
could you please merge with master and resolve conflicts. |
|
@oesteban what do you think about this approach? It uses |
…hat dont take path.local
…uded in the docstrings
… be included in the docstrings" This reverts commit 952fdc9. My script removed to many things from doctest, will update it.
…so doesnt have to be included in the docstrings
djarecka
force-pushed
the
pytest_cleaning
branch
from
7326293 to
ce13ade
Compare
|
There is one extra problem with changing working directory for all doctests (or all tests). Many files with doctests start with this part and the following tests assume that they are in a directory with data and will fail if I change to a temporary directory. In @oesteban, @satra let me know if you have any opinion about it. Not sure how important to you is to run everything in readonly systems and is it ok to add this boilerplate to doctests, that should be probably as simple as possible. Or maybe we want to add some extra code only in tests you're having problem with? |
|
@djarecka - i agree that we should not be making the examples more complex. let's just focus on the unit-test cleanup first, and we can tackle the doctest issue later. we can perhaps change the doctests for the specific interfaces that write files during doctests. that's a much smaller list. @oesteban - regarding read only systems, why are we thinking of running tests on these systems? can't we test before such a container is made read only? |
|
@satra yes, you can add those two lines within specific doctests if you want: do you want me to add anything specific to this PR? |
|
@djarecka - knowing which interfaces fail doctests for read only systems would be good. if it's a small list, we can fix those rather than everything. |
|
I think I covered most of the culprits here |
|
@mgxd ok, I will review those tests |
|
@satra I'm also reviewing other tests, I remember that I left |
|
@djarecka - the goal is to be semantically uniform such that developers can rely on expectations and to use the tmpdir methods when possible. |
…ing conftest.py with datadir variable and some basic libraries (so you dont have to import them in doctests)
|
@mgxd can you check if those tests are working now in the readonly setting you were using? |
| @@ -43,7 +43,7 @@ def test_dvars(tmpdir): | |||
| in_mask=example_data('ds003_sub-01_mc_brainmask.nii.gz'), | |||
| save_all=True, | |||
| intensity_normalization=0) | |||
| os.chdir(str(tmpdir)) | |||
| os.chdir(tmpdir.strpath) | |||
There was a problem hiding this comment.
true, you can do tmpdir.chdir() here 👍
| @@ -20,7 +20,7 @@ def check_close(val1, val2): | |||
| in1 = example_data('segmentation0.nii.gz') | |||
| in2 = example_data('segmentation1.nii.gz') | |||
|
|
|||
| os.chdir(str(tmpdir)) | |||
| os.chdir(tmpdir.strpath) | |||
| mskdata = nb.load(in_mask, mmap=NUMPY_MMAP).get_data() | ||
| aff = nb.load(in_mask, mmap=NUMPY_MMAP).affine | ||
|
|
||
| dwshape = (mskdata.shape[0], mskdata.shape[1], mskdata.shape[2], 6) | ||
| dwdata = np.random.normal(size=dwshape) | ||
| os.chdir(str(tmpdir)) | ||
| os.chdir(tmpdir.strpath) |
| @@ -74,7 +74,7 @@ class ABoverlap(AFNICommand): | |||
| >>> aboverlap.inputs.in_file_a = 'functional.nii' | |||
| >>> aboverlap.inputs.in_file_b = 'structural.nii' | |||
| >>> aboverlap.inputs.out_file = 'out.mask_ae_overlap.txt' | |||
| >>> aboverlap.cmdline # doctest: +ALLOW_UNICODE | |||
There was a problem hiding this comment.
Have you tried this on Python 2 and it works? (now it is a global option 👍 )
nipype/interfaces/dcm2nii.py
Outdated
| @@ -70,13 +70,16 @@ class Dcm2nii(CommandLine): | |||
| Examples | |||
| ======== | |||
|
|
|||
| >>> tmp = getfixture('tmpdir') | |||
There was a problem hiding this comment.
Please consider the .. testsetup:: directive to hide these two lines in the documentation.
http://www.sphinx-doc.org/en/stable/ext/doctest.html#directive-testsetup
There was a problem hiding this comment.
@oesteban : do you have an example how to use it? I'm adding .. testsetup:: before those lines and my tests are working, but everything is still visible if you ask for help()
| @@ -524,7 +524,7 @@ def test_itersource_two_join_nodes(tmpdir): | |||
|
|
|||
| def test_set_join_node_file_input(tmpdir): | |||
| """Test collecting join inputs to a set.""" | |||
| wd = str(tmpdir) | |||
| wd = tmpdir.strpath | |||
| os.chdir(wd) | |||
| @@ -524,7 +524,7 @@ def test_itersource_two_join_nodes(tmpdir): | |||
|
|
|||
| def test_set_join_node_file_input(tmpdir): | |||
| """Test collecting join inputs to a set.""" | |||
| wd = str(tmpdir) | |||
| wd = tmpdir.strpath | |||
| os.chdir(wd) | |||
| open('test.nii', 'w+').close() | |||
There was a problem hiding this comment.
tmpdir.join('test.nii').close()
| @@ -524,7 +524,7 @@ def test_itersource_two_join_nodes(tmpdir): | |||
|
|
|||
| def test_set_join_node_file_input(tmpdir): | |||
| """Test collecting join inputs to a set.""" | |||
| wd = str(tmpdir) | |||
| wd = tmpdir.strpath | |||
| os.chdir(wd) | |||
| open('test.nii', 'w+').close() | |||
| open('test2.nii', 'w+').close() | |||
There was a problem hiding this comment.
tmpdir.join('test2.nii').close()
| @@ -547,7 +547,7 @@ def test_set_join_node_file_input(tmpdir): | |||
|
|
|||
| def test_nested_workflow_join(tmpdir): | |||
| """Test collecting join inputs within a nested workflow""" | |||
| wd = str(tmpdir) | |||
| wd = tmpdir.strpath | |||
| os.chdir(wd) | |||
| addopts = --doctest-modules | ||
| doctest_optionflags = ALLOW_UNICODE NORMALIZE_WHITESPACE |
|
@oesteban thanks for comments. I'm still reviewing those test, and will try to implement all you points. At the beginning I change some parts automatically, but it didn't actually work as good, so will review chdir parts etc. |
|
I'm reviewing tests, and tried to remove all remainders of
|
|
@djarecka Here's the error log I ran into (I pulled from commit 8baac42 so this may be outdated) - all i'm really doing is |
|
@mgxd : those errors are from tests that you not listed previously, it might mean that the fix works, but more tests require changes |
|
@mgxd but thanks for checking! I'll test it on my own, makes sense to just change the permission. |
|
I have still problem with one test in read-only systems, it's a well-known |
|
@djarecka - the location of where crash files are dumped can be changed via config. |
|
@satra I noticed that I can change |
yes |
|
@satra ok, i change this one test. everything should work now in read-only systems |
|
i mean, except the one test that it's failing regardless of permissions... will debug it again today |
…elated to tmpdir that is the same in fixtures and test functions, DataSInk was trying copy dir to subdir etc.)
| @@ -123,10 +123,10 @@ class Bunch(object): | |||
| -------- | |||
| >>> from nipype.interfaces.base import Bunch | |||
| >>> inputs = Bunch(infile='subj.nii', fwhm=6.0, register_to_mean=True) | |||
| >>> inputs # doctest: +ALLOW_UNICODE | |||
| >>> inputs | |||
There was a problem hiding this comment.
why remove unicode?
Expected:
Bunch(fwhm=6.0, infile='subj.nii', register_to_mean=True)
Got:
Bunch(fwhm=6.0, infile=u'subj.nii', register_to_mean=True)
There was a problem hiding this comment.
I set this in pytest.ini: https://github.com/nipy/nipype/blob/master/pytest.ini#L4
is it failing?
There was a problem hiding this comment.
You'd have to show me where it's failing. It works on my py2 and all tests for PR were also passing
| 'ls -al' | ||
|
|
||
| # Use get_traitsfree() to check all inputs set | ||
| >>> pprint.pprint(cli.inputs.get_traitsfree()) # doctest: +NORMALIZE_WHITESPACE +ALLOW_UNICODE |
There was a problem hiding this comment.
we should add these tags back in?
Expected:
{'args': '-al',
'environ': {'DISPLAY': ':1'},
'ignore_exception': False}
Got:
{'args': '-al', 'environ': {'DISPLAY': ':1'}, 'ignore_exception': False}
There was a problem hiding this comment.
Seems like +ALLOW_UNICODE did work right?
There was a problem hiding this comment.
@oesteban @mgxd - so the way how Mathias is testing is different from what I always do:
conda create -n testnipype python=2 pip
source activate testnipype
conda install -y nipype
pip install -U https://github.com/nipy/nipype/archive/master.zip
python -c "import nipype; nipype.test()"
Indeed, if you test like this, pytest is testing nipype installed in the environment, i.e. ....../envs/testnipype/lib/python2.7/site-packages/nipype/interfaces/base.py and there is no pytest.ini to collect.
There was a problem hiding this comment.
I'm just wondering if the pytest.ini shouldn't be innipype/nipype. any thoughts?
changing str(tmpdir) to tmpdir.strpath in tests (see the discussion in #1916)
update
using more of the tmpdir methods in tests
updating tests (mostly doctests) so the work on read-only system (see #1916)
adding setting for doctests in pytest.ini (and removing some options from doctests)
adding some names to doctests_namespace in conftest.py