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

Port lib/test_util.py to pytest #1413

Merged
merged 10 commits into from Jun 21, 2017

Conversation

Projects
None yet
5 participants
@utkbansal
Member

utkbansal commented Jun 20, 2017

Fixes partially #884

Changes made in this Pull Request:

PR Checklist

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

utkbansal added some commits Jun 20, 2017

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jun 20, 2017

Looks like the original coverage.py package was developed by Ned Batchelder and cover-package used with nose and pytest-cov used with pytest are just wrappers around it. I'm guessing they will merge just fine.

@@ -21,6 +21,8 @@
#
from __future__ import absolute_import, division
from unittest import TestCase

This comment has been minimized.

@orbeckst

orbeckst Jun 20, 2017

Member

Is using unittest really necessary? I thought we can get rid of it. pytest supports setup_method()/teardown_method() (instead of setUp()) https://docs.pytest.org/en/2.7.3/xunit_setup.html

(I know that it also supports running UnitTest classes.)

This comment has been minimized.

@utkbansal

utkbansal Jun 20, 2017

Member

I missed that somehow! I'll read more about this and try to get rid of unittest. Thanks!

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jun 20, 2017

Why us Travis complaining /home/travis/.travis/job_stages: line 54: pytest: command not found ??

ci-helpers clearly state that pytest is already installed.

utkbansal added some commits Jun 20, 2017

.travis.yml Outdated
- SETUP_CMD=""
- BUILD_CMD="pip install -v package/ && pip install testsuite/"
- CONDA_DEPENDENCIES="mmtf-python nose=1.3.7 mock six biopython networkx cython joblib nose-timer"
- CONDA_ALL_DEPENDENCIES="mmtf-python nose=1.3.7 mock six biopython networkx cython joblib nose-timer matplotlib netcdf4 scikit-learn scipy seaborn coveralls clustalw=2.1"
# Install griddataformats from PIP so that scipy is only installed in the full build (#1147)
- PIP_DEPENDENCIES='griddataformats'
- PIP_DEPENDENCIES='griddataformats pytest pytest-cov'

This comment has been minimized.

@kain88-de

kain88-de Jun 20, 2017

Member

please add them do conda. They are both available on the conda-forge channel. This might also be messing up your pytest installation. But I'm not sure about that.

This comment has been minimized.

@kain88-de

kain88-de Jun 21, 2017

Member

I would still prefer pytest as a conda dependency.

This comment has been minimized.

@utkbansal

utkbansal Jun 21, 2017

Member

@kain88-de Should I add them to CONDA _DEPENDENCIES or CONDA_ALL_DEPENDENCIES?

This comment has been minimized.

@kain88-de

kain88-de Jun 21, 2017

Member

you need to add them to both. We can check in another PR why they aren't installed automatically.

@@ -124,7 +126,7 @@ def test_strings(self):
assert_equal(util.iterable(u"unicode string"), False)
class TestFilename(object):
class TestFilename(TestCase):

This comment has been minimized.

@kain88-de

kain88-de Jun 20, 2017

Member

You are using TestCase here to make the test classes discoverable to pytest. This means this class isn't normally picked up by pytest. Please make sure the class has no init method. You can also run pytest with --collect-only to make sure all tests are picked up quickly.

This comment has been minimized.

@utkbansal

utkbansal Jun 20, 2017

Member

No, I'm using it because only classes that inherit from TestCase get their setUp and tearDown methods executed.

This comment has been minimized.

@kain88-de

kain88-de Jun 20, 2017

Member

So we need this for a swift conversion and then remove it one by one again later. check

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jun 20, 2017

I have flipped the MAIN_CMD command so that pytest is executed first - this will just make my life easier and shouldn't change anything.

@kain88-de

This comment has been minimized.

Member

kain88-de commented Jun 20, 2017

@utkbansal if you could remove the yield based test that would be great. They do generate quite a lot of noise in the output.

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jun 20, 2017

We have a lot of yield based tests all over the code, I can suppress the warning for now - if that is fine. Getting rid of them and using parameterize would need significant efforts(I guess) and was planned to be done in part 2.

@kain88-de

This comment has been minimized.

Member

kain88-de commented Jun 20, 2017

Suppressing the warnings for now is fine.

utkbansal added some commits Jun 20, 2017

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jun 21, 2017

Wohoo! Coverage increased! 😬

@jbarnoud

This comment has been minimized.

Contributor

jbarnoud commented Jun 21, 2017

And the full test build is passing (likely by chance, but still!).

- CONDA_DEPENDENCIES="mmtf-python nose=1.3.7 mock six biopython networkx cython joblib nose-timer"
- CONDA_ALL_DEPENDENCIES="mmtf-python nose=1.3.7 mock six biopython networkx cython joblib nose-timer matplotlib netcdf4 scikit-learn scipy seaborn coveralls clustalw=2.1"
- CONDA_DEPENDENCIES="mmtf-python nose=1.3.7 mock six biopython networkx cython joblib nose-timer pytest=3.1.2 pytest-cov=2.5.1"
- CONDA_ALL_DEPENDENCIES="mmtf-python nose=1.3.7 mock six biopython networkx cython joblib nose-timer matplotlib netcdf4 scikit-learn scipy seaborn coveralls clustalw=2.1 pytest=3.1.2 pytest-cov=2.5.1"

This comment has been minimized.

@kain88-de

kain88-de Jun 21, 2017

Member

why the version pinning?

This comment has been minimized.

@utkbansal

utkbansal Jun 21, 2017

Member

Conda already says that it has pytest installed and still it did not work. Also, the version it has is 2.7, so there has been a major release after that.

This comment has been minimized.

@richardjgowers

richardjgowers Jun 21, 2017

Member

should be able to do conda upgrade pytest?

This comment has been minimized.

@utkbansal

utkbansal Jun 21, 2017

Member

@richardjgowers after the source ci-helpers/travis/setup_conda.sh step?

This comment has been minimized.

@kain88-de

kain88-de Jun 21, 2017

Member

the older version comes from astropy. They specifically install an older version of pytest for themselves. Leave the pinning for now. I'm asking what they use as a preferred way to use a newer pytest version.

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jun 21, 2017

Should be ready to merge, only the full build times out. We might not need the full build to succeed for the last commit as it was only shifting requirements from pip to conda and the commit previous to this verifies that coverage works and has not dropped.

@richardjgowers richardjgowers merged commit fba80e2 into MDAnalysis:develop Jun 21, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
QuantifiedCode No new issues introduced.
Details

@richardjgowers richardjgowers self-assigned this Jun 21, 2017

@richardjgowers

This comment has been minimized.

Member

richardjgowers commented Jun 21, 2017

@utkbansal awesome, one module down! Can you try and get the unfinished coordinates port PR up ASAP so we can see what changes are required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment