Skip to content

Pytest follow-up #1751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Dec 24, 2016
Merged

Pytest follow-up #1751

merged 19 commits into from
Dec 24, 2016

Conversation

djarecka
Copy link
Collaborator

This is a follow-up to my previous Pytes PR (#1722), this time I've read the Satra review :)

  • removed some files from tools directory that are not used anymore
  • removed fixes
  • changed .coveragerc (workflows is not omitted)
  • removed coverage from travis, so it is now calculated from CircleCI only
  • added one auto tests test_auto_ACompCor.py and changing test_auto_TCompCor.py, so it agrees with checkspec.py output
  • recovered test_BEDPOSTX.py and test_Xfibres.py
  • removed nosetes from __init__.py
  • changed one test from test_math.py that was failing
  • some smaller changes suggested in the review

Still TODO:

  • tests test_BEDPOSTX.py and test_Xfibres.py don't affect anything IMO. Should I create test_BEDPOSTX5.py and test_Xfibres5.py, and delete test_auto_BEDPOSTX5.py and test_auto_Xfibres5.py?
  • __init__.py is not finished, nipype.test() doesn't work

@codecov-io
Copy link

codecov-io commented Dec 16, 2016

Current coverage is 72.41% (diff: 96.77%)

Merging #1751 into master will increase coverage by 1.30%

@@             master      #1751   diff @@
==========================================
  Files          1028       1055     +27   
  Lines         50616      52708   +2092   
  Methods           0          0           
  Messages          0          0           
  Branches       7333       7665    +332   
==========================================
+ Hits          35994      38170   +2176   
+ Misses        13495      13323    -172   
- Partials       1127       1215     +88   

Powered by Codecov. Last update ff36588...3786640

#NOTE_dj: not sure if this should pass, it uses cmdline from interface.base.CommandLine
#assert stder.cmdline == "fslmaths a.nii -Tstd %s"%os.path.join(testdir, "a_std.nii")
stder = fsl.StdImage(in_file="a.nii", output_type='NIFTI')
assert stder.cmdline == "fslmaths a.nii -Tstd %s"%os.path.join(testdir, "a_std.nii")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use .format()

@@ -19,4 +19,4 @@ def test_ICC_rep_anova():
assert round(icc, 2) == 0.71
assert dfc == 3
assert dfe == 15
assert r_var / (r_var + e_var) == icc
assert np.isclose(r_var / (r_var + e_var), icc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer allclose for compatibility with earlier numpy versions.

@satra
Copy link
Member

satra commented Dec 18, 2016

@djarecka - test_bedpostx and test_xfibres simply prevent the autocreation of the auto tests for those which would duplicate the test_auto_*5 at a point in the past we had two versions of these tools. so your current changes are fine and it doesn't need test_*5.py

@satra
Copy link
Member

satra commented Dec 18, 2016

@djarecka - if you can finish the nipype.test() this should be ready. i'll also send a PR to your branch with some updated other tests.

@djarecka
Copy link
Collaborator Author

@satra even without test_bedpostx and test_xfibres auto tests are NOT created.

@djarecka
Copy link
Collaborator Author

@satra I'm a bit stuck with nipype.test(). If you want to merge this ASAP, it might be better if I move nipype.test() to another PR, it somehow independent issue.

@satra
Copy link
Member

satra commented Dec 18, 2016

@djarecka - you are correct - the autotests are not created because they are the same exact class as the *5.py classes in this case. so yes, in this case those two tests can be deleted.

@satra
Copy link
Member

satra commented Dec 18, 2016

test_BEDPOSTX.py and test_Xfibres.py

@djarecka
Copy link
Collaborator Author

@satra ok, I've removed the extra tests. Just wasn't sure, if you want to disable auto tests for BEDPOSTX5 and XFibres5.

I also changed earlier %s to format() in test_math in the places where I could do it without changing the code itself (you're using %s style in nipype.interfaces.fsl.maths)

@djarecka
Copy link
Collaborator Author

@satra can you also check this assert? https://github.com/nipy/nipype/blob/master/nipype/interfaces/tests/test_base.py#L137
I've removed it since I'm only getting a UserWarning

@satra
Copy link
Member

satra commented Dec 18, 2016

@djarecka - i can't seem to figure out why that should raise a TypeError - ok to remove for now.

@satra
Copy link
Member

satra commented Dec 18, 2016

regarding .format to slowly start moving towards current python standards - so a new change should always reflect current coding standards.

@satra satra mentioned this pull request Dec 18, 2016
@satra satra merged commit 3786640 into nipy:master Dec 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants