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

Dropping nose as a dependency #1480

Merged
merged 3 commits into from Jul 18, 2017
Merged

Conversation

utkbansal
Copy link
Member

@utkbansal utkbansal commented Jul 13, 2017

Fixes #884

Changes made in this Pull Request:

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@utkbansal utkbansal mentioned this pull request Jul 13, 2017
4 tasks
Copy link
Member

@kain88-de kain88-de left a comment

Choose a reason for hiding this comment

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

nose also has to be removed from setup.py our nose wrapper script should be removed. Also grep and look through the repository if we mention nose anywhere else and should replace it with pytest.

@utkbansal
Copy link
Member Author

@kain88-de We've got from nose.plugins.attrib import attr all over the tests. Can't go ahead with this until I get rid of all attr. Should that be done in this PR or a separate one?

@kain88-de
Copy link
Member

you can remove attr in this PR. It doesn't do anything special

@jbarnoud
Copy link
Contributor

Since you're at it, you can update testsuite/CHANGELOG.

@richardjgowers
Copy link
Member

This would officially fix #884!

.travis.yml Outdated
NOSE_COVERAGE='--with-coverage --cover-package MDAnalysis'
PYTEST_COVERAGE='--cov=MDAnalysis'
MAIN_CMD='export COVERAGE_FILE=$PYTEST_COVERAGE_FILE; pytest ${PYTEST_LIST} ${PYTEST_FLAGS} ${PYTEST_COVERAGE}; export COVERAGE_FILE=$NOSE_COVERAGE_FILE; python ./testsuite/MDAnalysisTests/mda_nosetests ${NOSE_TEST_LIST} ${NOSE_FLAGS} ${NOSE_COVERAGE}'
MAIN_CMD='pytest testsuite/MDAnalysisTests/ ${PYTEST_FLAGS} --cov=MDAnalysis'
Copy link
Member

Choose a reason for hiding this comment

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

can't you just update the PYTEST_FLAGS variable here instead of rewriting MAIN_CMD?

Copy link
Member

@kain88-de kain88-de left a comment

Choose a reason for hiding this comment

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

the setup.py changes are still missing

@@ -150,28 +147,4 @@
in_dir,
assert_nowarns,
)
from MDAnalysisTests.core.util import make_Universe

def run(*args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice if you could keep this method. This is shown in our docs how to run the tests yourself from an installation of MDAnalysisTests

Copy link
Member Author

Choose a reason for hiding this comment

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

So this will then be deleted when we do the next release?

Copy link
Member Author

@utkbansal utkbansal Jul 14, 2017

Choose a reason for hiding this comment

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

@kain88-de This function has calls to loaded_plugins which comes from from MDAnalysisTests.plugins which in turn depend upon nose. What should I do?

Copy link
Member

Choose a reason for hiding this comment

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

modify this function so that I can still to MDAnalysisTests.run() in an interpreter. This should start the pytest tests. I don't really care how the function is doing that. But it has to be rewritten.

Copy link
Member Author

Choose a reason for hiding this comment

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

So basically, I'm writing a wrapper to run pytest via this run method?

Copy link
Member

Choose a reason for hiding this comment

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

yes

@utkbansal
Copy link
Member Author

@kain88-de @richardjgowers @jbarnoud Is the maintainer/run_tests.sh file obsolete? Should I drop the file?

@kain88-de
Copy link
Member

sure remove that as well. No one of us uses it anymore.

@jbarnoud
Copy link
Contributor

Have a look at how to run pytest from within python.

Copy link
Member

@kain88-de kain88-de left a comment

Choose a reason for hiding this comment

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

testsuite/INSTALL and package/MDAnalysis/tests/init.py also contain comments about nose. Please update and replace them with appropriate pytest information if applicable.

@@ -114,6 +114,9 @@
"""
from __future__ import absolute_import
import logging

import pytest

Copy link
Member

Choose a reason for hiding this comment

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

there are still a lot of nose comments in this file

Copy link
Member Author

Choose a reason for hiding this comment

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

@kain88-de Do I have to update all the comments involving nose in all the files?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please. There is no sense in having them. Only a few might be useful.


def run(*args, **kwargs):
pytest.main()
Copy link
Member

Choose a reason for hiding this comment

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

did you test this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's still running. Everything fine till now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do I have to update all comments related to nose in all files?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kain88-de Confirmed, it works.

@orbeckst
Copy link
Member

We also need new instructions for the wiki #1489.

@@ -92,16 +92,14 @@
Writing test cases
==================

The unittests use the :mod:`unittest` module together with nose_. See the
The unittests use the :mod:`pytest` module. See the
Copy link
Member

Choose a reason for hiding this comment

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

This should be pytest_ so that it links in the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kain88-de :mod:pytest_ ??

Copy link
Member

Choose a reason for hiding this comment

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

with out the mod sphinx likely wouldn't link to the right page

Copy link
Member

@kain88-de kain88-de left a comment

Choose a reason for hiding this comment

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

test_streamio and test_trpparser still contain comments about nose.

@kain88-de
Copy link
Member

@utkbansal I'd appreciate if you prioritize this PR

@utkbansal
Copy link
Member Author

@kain88-de The test in test_streamio that has a comment on nose is a yield based test. I'd rather leave it there till I fix it properly.

@utkbansal
Copy link
Member Author

@kain88-de Will this PR be squashed and merged or can I clean up the history?

Remove nose from dependencies & implement run method
Copy link
Member

@kain88-de kain88-de left a comment

Choose a reason for hiding this comment

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

minor clean up still do to. But good progress.

@@ -84,9 +84,9 @@ From sources

From the top directory of the checked out sources:

./testsuite/MDAnalysisTests/mda_nosetests --exe -v --processes=4 --process-timeout=120
pytest /testsuite/MDAnalysisTests/ -n=3
Copy link
Member

Choose a reason for hiding this comment

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

please add a note that the pytest-xdist package should be installed to run tests in parallel.

Copy link
Member

Choose a reason for hiding this comment

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

and use the new pytest.xdist variable numprocesses

@@ -90,22 +67,13 @@
Writing test cases
==================

The unittests use the :mod:`unittest` module together with nose_. See the
The unittests use the :mod:`pytest` module. See the
Copy link
Member

Choose a reason for hiding this comment

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

here we should also have a link

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -36,9 +36,7 @@
assert_array_almost_equal,
assert_raises,
Copy link
Member

Choose a reason for hiding this comment

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

can you remove this import as well. Seems like you already changed all occurrences of this

@@ -213,7 +213,7 @@ def dynamic_author_list():
long_description=LONG_DESCRIPTION,
install_requires=[
'MDAnalysis=={0!s}'.format(RELEASE), # same as this release!
'nose>=1.3.7',
'numpy>=1.10.4',
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this line when you do the rebase. It's not there in develop right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kain88-de Remove numpy?

Copy link
Member

Choose a reason for hiding this comment

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

yes it's already installed with MDAnalysis anyway.

Fix Install docs

Update CHANGELOG

Cleanup docs
@kain88-de kain88-de merged commit 3dd9d3c into MDAnalysis:develop Jul 18, 2017
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

5 participants