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

Switching tests to pytest #1652

Merged
merged 45 commits into from Jan 26, 2019

Conversation

@skoudoro
Copy link
Member

commented Oct 16, 2018

This PR replace nose by pytest.

Unfortunately numpy.testing depends on nose until 1.15 so we need to keep this dependency. We just do not use it directly anymore

This PR should fix #1280 request and supersede #1414

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

Todo:

  • check setup_module. Add global variable
  • check setup_test. it does not work. should be replace by conftest.py and setup.cfg
  • check + Skip if in doctest. Add the decorator
  • update appveyor when travis is ok

@skoudoro skoudoro force-pushed the skoudoro:switch_to_pytest branch 2 times, most recently from d657783 to d9718ca Oct 22, 2018

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

Hi @arokem, Pytest is failing because of this error which I find it strange because we can not reproduce it with Nosetests.
I would like your opinion about this error. Thank you

@arokem

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

Hmmm. Have you been able to reproduce that error locally? I am trying it right now.

@arokem

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

OK - yeah - I see this locally as well. Will investigate.

@arokem

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

Hmm. I am now seeing this on master as well, with a new environment I made on my local machine. Did anything else change around this? I think there's a new release of nibabel. Maybe that's causing this somehow?

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

Did anything else change around this?

Currently, No idea...

@arokem

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Hey @skoudoro : would you mind rebasing this on master? I'd like to give this another look.

@skoudoro skoudoro force-pushed the skoudoro:switch_to_pytest branch from d9718ca to 3ad8e93 Nov 30, 2018

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

Done, sorry for the delay.

@arokem

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Looks like that test is still failing on Travis (but not on appveyor?!). But I still cannot replicate that locally... The Travis log is also endlessly growing because of a warning coming out of cvxpy, related to this issue. I might have to make a PR on cvxpy to resolve that...

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

Looks like that test is still failing on Travis

Unfortunately, exactly the same error....

(but not on appveyor?!)

because I did not change this file yet... I prefer to fix first Travis...

I might have to make a PR on cvxpy to resolve that...

great! 👍

@arokem

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

OK: more information here. I am definitely seeing this error locally, but it's intermittent, happening maybe one out of 10 times that I run it. And I do also see this on master, so this is not a pytest specific thing. See also #1679

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Dec 8, 2018

Finally, it is intermittent because of a np.random.random here.

I think I will just create a static array for this test and it should work. What do you think @arokem?

@skoudoro skoudoro force-pushed the skoudoro:switch_to_pytest branch from 8adb979 to be2fc25 Dec 11, 2018

@arokem

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

I just ran this on my mac laptop and am glad to report that everything is passing.

I still get a lot of PendingDeprecation warnings from cvxpy. I hope they review my PR soon.

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2018

I just ran this on my mac laptop and am glad to report that everything is passing.

Great! it seems to fail on python2.7 but this is an easy fix. My main concern is about our nose dependence due to numpy.testing. Should we just get rid of numpy.testing or we just accept it. Numpy drop Nosetests on version 1.15 and I can not imagine us having numpy 1.15 as minimal version 😄 which means we will keep nose for still a long time.

In any case, if we have to do this change, it will be for a new PR. What do you think @arokem or devs?

I still get a lot of PendingDeprecation warnings from cvxpy. I hope they review my PR soon.

I hope so too!

@arokem

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

My inclination is to say that it's fine for us to keep indirectly depending on nose for testing as long as we still depend on versions of numpy that depend on nose for testing.

@arokem

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Good news! Looks like my PRs on cvxpy were merged, so we shouldn't see these warnings anymore once they make a release.

@codecov-io

This comment has been minimized.

Copy link

commented Dec 13, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@bdb2b97). Click here to learn what that means.
The diff coverage is 83.58%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1652   +/-   ##
=========================================
  Coverage          ?   84.24%           
=========================================
  Files             ?      115           
  Lines             ?    13566           
  Branches          ?     2140           
=========================================
  Hits              ?    11428           
  Misses            ?     1642           
  Partials          ?      496
Impacted Files Coverage Δ
dipy/tracking/streamline.py 69.26% <ø> (ø)
dipy/tracking/metrics.py 81.02% <ø> (ø)
dipy/tracking/life.py 97.79% <ø> (ø)
dipy/core/geometry.py 90.63% <ø> (ø)
dipy/tracking/utils.py 88.88% <ø> (ø)
dipy/reconst/dsi.py 80.21% <ø> (ø)
dipy/viz/projections.py 20% <100%> (ø)
dipy/io/utils.py 53.12% <100%> (ø)
dipy/workflows/reconst.py 77.49% <100%> (ø)
dipy/utils/optpkg.py 69.56% <40%> (ø)
... and 3 more

@skoudoro skoudoro force-pushed the skoudoro:switch_to_pytest branch from 846f3ec to 8f57b60 Dec 30, 2018

skoudoro added 4 commits Jan 2, 2019
@pep8speaks

This comment has been minimized.

Copy link

commented Jan 2, 2019

Hello @skoudoro, Thank you for updating !

Line 3:1: W293 blank line contains whitespace

Line 544:57: E128 continuation line under-indented for visual indent
Line 545:57: E128 continuation line under-indented for visual indent

Line 73:53: E128 continuation line under-indented for visual indent

Line 485:1: E305 expected 2 blank lines after class or function definition, found 1

  • Complete extra results for this file :

file_to_check.py:538:-654: W605 invalid escape sequence '\l'file_to_check.py:571:-719: W605 invalid escape sequence '\l'---

Line 628:47: E127 continuation line over-indented for visual indent

Line 181:81: E501 line too long (81 > 80 characters)

Line 32:1: W293 blank line contains whitespace

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

Yeah !!! It took me time to understand the warning management and issues but it is perfect now and ready to go !!! I fixed the pep8 issue but pep8speaks do not update. I do not know why this tool does not work correctly on DIPY repo. I have no issue with other's repos.

Ready to be merged! Can someone have a look at this PR? @arokem? @Garyfallidis?

@arokem
Copy link
Member

left a comment

Looks good to me. I had a couple of comments/questions.

if LooseVersion(np.__version__) >= LooseVersion('1.14'):
np.set_printoptions(legacy='1.13')
# Temporary fix until scipy release in October 2018
# must be removed after that

This comment has been minimized.

Copy link
@arokem

arokem Jan 3, 2019

Member

Did this ever happen?

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jan 3, 2019

Author Member

yes, this refers to #1601

This comment has been minimized.

Copy link
@arokem

arokem Jan 4, 2019

Member

So should we remove this?

fi
- nosetests --with-doctest --verbose $COVER_ARGS dipy
- $COVER_CMD pytest -s --doctest-modules --verbose --durations=10 --pyargs dipy

This comment has been minimized.

Copy link
@arokem

arokem Jan 3, 2019

Member

I get Coverage.py warning: --include is ignored because --source is set (include-ignored) when I run this command line locally on my laptop.

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jan 3, 2019

Author Member

Thanks for this info. I did not try locally with coverage. So I will look at this

@@ -39,9 +39,9 @@ branch points. To check how much coverage the tests have, you will need.

When running:

nosetests --with-coverage --cover-package=dipy
coverage run -m pytest -s --doctest-modules --verbose dipy

This comment has been minimized.

Copy link
@arokem

arokem Jan 3, 2019

Member

Also get Coverage.py warning: --include is ignored because --source is set (include-ignored) when I run this locally.

Makefile Outdated
@@ -27,7 +27,7 @@ ext: recspeed.so propspeed.so vox2track.so \
mrf.so

test: ext
nosetests .
pytest .

This comment has been minimized.

Copy link
@arokem

arokem Jan 3, 2019

Member

Do we want to add the full incantation here?

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jan 3, 2019

Author Member

Good question. I think we should just add --doctest-modules option

dipy/conftest.py Show resolved Hide resolved
raise AssertionError(msg.format(str(value2), str(value1)))


assert_greater_equal = partial(assert_operator, op=operator.ge,

This comment has been minimized.

Copy link
@arokem

arokem Jan 3, 2019

Member

Are these defined here because pytest doesn't have these? Looks like something that might be useful in numpy?

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jan 3, 2019

Author Member

Numpy and Pytest do not have them. they use assert x > y, but the error message is never explicit, it returns True or False. And yes, it can be useful for them.

@@ -47,39 +45,39 @@ def reconst_mmri_core(flow, lap, pos):

rtop = mmri_flow.last_generated_outputs['out_rtop']
rtop_data = nib.load(rtop).get_data()
eq_(rtop_data.shape, volume.shape[:-1])
assert_equal(rtop_data.shape, volume.shape[:-1])

This comment has been minimized.

Copy link
@arokem

arokem Jan 3, 2019

Member

If you want consistent style, you might go with npt.assert_equal here, as you had in other files (I think that's better -- more explicit).

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jan 3, 2019

Author Member

I agree,

I will update that

doc/installation.rst Show resolved Hide resolved
scratch/very_scratch/gqsampling_stats.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
@arokem

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

Nice work! This PR reminded me that I have been wanting to clean up "scractch", so I added #1713 to keep that in mind.

@arokem

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

I am happy to report that it's all passing on my laptop. We still do have a lot of warnings, but we should take those on separate PRs.

@arokem

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

OK. This looks good to me. Considering that it is a fairly crucial and sensitive transition, I think that it would be really good to get another review of this, before we consider merging it.

@arokem

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

Does anyone else have time to take a look through this? It's all fairly straightforward, but because it's our test suite, I think that it would be a good idea to have one more set of eyes on this, before we merge this. If I don't hear anything in the next week or so, I'll assume everyone is just too busy and I'll merge this as is.

@arokem

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

OK. Looks like no one else has the bandwidth to take a look, so I will go ahead and merge this. We'll want to keep and eye out for things that should fail and don't, or vice versa.

@arokem arokem merged commit 51234a4 into nipy:master Jan 26, 2019

4 checks passed

codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2019

Exciting! Thanks @arokem!

@skoudoro skoudoro deleted the skoudoro:switch_to_pytest branch Feb 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.