Skip to content
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

TEST: Convert remainder of nibabel.tests to pytest #876

Merged
merged 79 commits into from
Feb 6, 2020

Conversation

djarecka
Copy link
Collaborator

@djarecka djarecka commented Feb 4, 2020

I was continuing wit tests from nibabel.tests. I finished them, but I have to:

  • address Chris comments to my old PR

  • it looks like that in Nov I haven't removed all yield statement carefully, so have to come back to it it looks fine after checking

@codecov
Copy link

codecov bot commented Feb 4, 2020

Codecov Report

Merging #876 into pytest will increase coverage by 1.77%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           pytest     #876      +/-   ##
==========================================
+ Coverage   89.74%   91.51%   +1.77%     
==========================================
  Files          97       97              
  Lines       12403    12403              
  Branches     2185     2185              
==========================================
+ Hits        11131    11351     +220     
+ Misses        926      708     -218     
+ Partials      346      344       -2
Impacted Files Coverage Δ
nibabel/testing/np_features.py 33.33% <0%> (-66.67%) ⬇️
nibabel/testing/__init__.py 67.3% <0%> (-23.08%) ⬇️
nibabel/nifti1.py 91.96% <0%> (+0.45%) ⬆️
nibabel/parrec.py 92.5% <0%> (+0.7%) ⬆️
nibabel/volumeutils.py 84.13% <0%> (+0.96%) ⬆️
nibabel/loadsave.py 92.59% <0%> (+1.85%) ⬆️
nibabel/cmdline/utils.py 80.64% <0%> (+12.9%) ⬆️
nibabel/testing_pytest/__init__.py 100% <0%> (+14.95%) ⬆️
nibabel/pydicom_compat.py 100% <0%> (+36.36%) ⬆️
nibabel/cmdline/nifti_dx.py 100% <0%> (+55.55%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 979b439...6838af6. Read the comment docs.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Some preliminary comments. Thanks again for the time you're putting into this.

nibabel/tests/nibabel_data.py Outdated Show resolved Hide resolved
nibabel/tests/nibabel_data.py Outdated Show resolved Hide resolved
nibabel/tests/test_spaces.py Outdated Show resolved Hide resolved
nibabel/tests/test_spaces.py Outdated Show resolved Hide resolved
nibabel/tests/test_scripts.py Show resolved Hide resolved
nibabel/tests/test_spatialimages.py Outdated Show resolved Hide resolved
nibabel/tests/test_spatialimages.py Outdated Show resolved Hide resolved
nibabel/tests/test_spatialimages.py Outdated Show resolved Hide resolved
nibabel/tests/test_trackvis.py Outdated Show resolved Hide resolved
nibabel/tests/test_trackvis.py Outdated Show resolved Hide resolved
@effigies
Copy link
Member

effigies commented Feb 5, 2020

@djarecka The diff seems to have gotten full of commits from the main branch. Next time you commit, could you merge or rebase onto the latest pytest?

No rush though. I'm going to stop fiddling with nibabel and do real work for the rest of the day.

@djarecka
Copy link
Collaborator Author

djarecka commented Feb 5, 2020

@effigies - that's what I tried to do, but apparently I failed... by main branch, you mean pytest branch?
you think that rebasing can help now?

@effigies
Copy link
Member

effigies commented Feb 5, 2020

Yup, pytest branch. It may be that GitHub screwed up because we pushed at similar times.

Merging or rebasing usually resolves it.

return skipif(not have_files,
"Need files in {0} for these tests".format(required_path))
return pytest.mark.skipif(not have_files,
reason="Need files in {0} for these tests".format(required_path))
Copy link
Member

Choose a reason for hiding this comment

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

Because this is now a pytest.mark.skipif, nose isn't skipping these. You should grep for @needs_nibabel_data, and any files that use it should be added to the ignore list.

@djarecka djarecka changed the title [WIP] updates for nibabel.tests updates for nibabel.tests Feb 6, 2020
@djarecka
Copy link
Collaborator Author

djarecka commented Feb 6, 2020

@effigies - ready to review.
One travis fails for tests cifti2/tests/test_cifti2io_axes, not sure if I should worry about this right now, or after the cifti2 tests are converted to pytest

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Nice! A few last suggestions...

nibabel/tests/test_casting.py Outdated Show resolved Hide resolved
nibabel/tests/test_deprecator.py Show resolved Hide resolved
nibabel/tests/test_deprecator.py Show resolved Hide resolved
nibabel/tests/test_deprecator.py Show resolved Hide resolved
nibabel/tests/test_deprecator.py Show resolved Hide resolved
nibabel/tests/test_nifti1.py Show resolved Hide resolved
nibabel/tests/test_spatialimages.py Outdated Show resolved Hide resolved
nibabel/tests/test_spatialimages.py Outdated Show resolved Hide resolved
nibabel/tests/test_spatialimages.py Outdated Show resolved Hide resolved
nibabel/tests/test_spm99analyze.py Outdated Show resolved Hide resolved
djarecka and others added 3 commits February 6, 2020 11:41
@djarecka
Copy link
Collaborator Author

djarecka commented Feb 6, 2020

ok, I think I addressed everything

@effigies effigies changed the title updates for nibabel.tests TEST: Convert remainder of nibabel.tests to pytest Feb 6, 2020
@effigies effigies merged commit 5f4436f into nipy:pytest Feb 6, 2020
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.

2 participants