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

Pytest #31

Merged
merged 15 commits into from
Sep 18, 2018
Merged

Pytest #31

merged 15 commits into from
Sep 18, 2018

Conversation

RomainBrault
Copy link
Contributor

#30

@coveralls
Copy link

coveralls commented Jan 11, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 5420785 on RomainBrault:pytest into ac1f099 on scikit-learn-contrib:master.

Copy link
Contributor

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

  • Please also replace as needed other mentions of nosetests in particular in the readme and in setup.cfg.

  • You can also replace nose setup by pytest in appveyor.yml

@@ -24,7 +24,7 @@ popd

# Configure the conda environment and put it in the path using the
# provided versions
conda create -n testenv --yes python=$PYTHON_VERSION pip nose \
conda create -n testenv --yes python=$PYTHON_VERSION pip pytest pytest-cov \
Copy link
Contributor

Choose a reason for hiding this comment

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

It might still be worth keeping nose in the dependencies (in addition to pytest), as e.g. older versions of numpy.testing will use it, if I remember correctly.

@RomainBrault
Copy link
Contributor Author

I kept nose in install.sh and appveyor.yml and .gitignore; and added pytest to install.sh and appveyor.yml. No other mentions of nose should be present.

@RomainBrault
Copy link
Contributor Author

Also, I'm wondering why requirements.txt includes nose? I think neither pytest or nose should be in.

Copy link
Contributor

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks for the update; a few more comments are below.

It might still be worth keeping nose in the dependencies (in addition to pytest), as e.g. older versions of numpy.testing will use it, if I remember correctly.

I kept nose in install.sh and appveyor.yml and .gitignore; and added pytest to install.sh and appveyor.yml. No other mentions of nose should be present.

I'm not sure what should be done there; maybe @lesteve would know...

.gitignore Outdated
@@ -47,6 +47,7 @@ htmlcov/
.coverage
.coverage.*
.cache
.test.sh.swp
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not necessary, it's specific to your editor (vim I imagine) and should be set in your global .gitignore

else
nosetests -s $MODULE
py.test -v -x --doctest-modules --ignore=setup.py --pyargs $MODULE
Copy link
Contributor

Choose a reason for hiding this comment

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

--ignore doesn't work with --pyargs unfortunately pytest-dev/pytest#3287

else
py.test -v -x --doctest-modules --ignore=setup.py --pyargs $MODULE
py.test -v -x --doctest-modules --ignore=setup.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was a bit confused. Actually pyargs is mandatory to get the path right easily and ignore is superfluous

@lesteve
Copy link

lesteve commented Mar 30, 2018

It might still be worth keeping nose in the dependencies (in addition to pytest), as e.g. older versions of numpy.testing will use it, if I remember correctly.

Very unfortunately scikit-learn 0.19.1 has a hard dependency on nose (try importing sklearn.utils.testing without nose installed) so it may be the case that we still need nose (as a dependency not to run the tests) in the CIs for scikit-learn-contrib.

numpy.testing is irrelevant, basically nose was still used in a weird edge case (maybe numpy had something like numpy.testing.assert_equal that was using nose). The thing is that there is no actual need to use numpy.testing.assert_equal (or whatever the actual function was) and you can emulate it like we do in scikit-learn with unittest.

@NicolasHug
Copy link

Is there anything keeping from merging this?
It could make the badges go green again :)

@rth
Copy link
Contributor

rth commented Sep 17, 2018

Is there anything keeping from merging this?

Finding someone who has merge rights probably..

cc @glemaitre

@NicolasHug
Copy link

@amueller ?

@glemaitre glemaitre merged commit 509e91f into scikit-learn-contrib:master Sep 18, 2018
@glemaitre
Copy link
Member

Merging this one. I think that we should have more update regarding this template.

@rth rth mentioned this pull request Sep 18, 2018
@rth
Copy link
Contributor

rth commented Sep 18, 2018

Thanks @glemaitre ! A few thoughts on what could be simplified in #40 ..

@glemaitre
Copy link
Member

I am working on it. I will make a todo list there.

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.

None yet

6 participants