Skip to content

/ mdanalysis

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

Jump to bottom

Port topology module #1420

Merged
merged 4 commits into from Jun 24, 2017
Merged

Port topology module #1420

merged 4 commits into from Jun 24, 2017

Conversation

@utkbansal
Copy link
Member

@utkbansal utkbansal commented Jun 21, 2017

Fixes #

Changes made in this Pull Request:

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?
utkbansal added 2 commits Jun 21, 2017
@utkbansal
Port topology to pytest
Verified
This commit was signed with a verified signature.
utkbansal Utkarsh Bansal
GPG key ID: 359A4CFC0731150A Learn about signing commits
Loading status checks…
ba05619
@utkbansal
Add __test__ attributes and get rid of static methods
Verified
This commit was signed with a verified signature.
utkbansal Utkarsh Bansal
GPG key ID: 359A4CFC0731150A Learn about signing commits
Loading status checks…
1f64af4
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 22, 2017

This should hopefully get coverage back to normal.
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 22, 2017

@richardjgowers @kain88-de Minor increase in coverage! A review would be helpful.
@kain88-de
Copy link
Member

@kain88-de kain88-de commented Jun 23, 2017

@jbarnoud & @mnmelo can you have a look I don't have time currently.
@jbarnoud
jbarnoud requested changes Jun 23, 2017
View changes
Copy link
Contributor

@jbarnoud jbarnoud left a comment

Style is much better than in the PR on utils. Just one file where the imports are reordered.

I did not notice any tests that are missing, but I figured out why I had test that appeared discovered in pytest but not in nose: while we generally avoided docstring in tests to have a readable log in nose, we still have some. Nose does display the docstring instead of the test name in the verbose view; pytest doesn't. It means that it is difficult to accurately compare the two logs, but also that we will be able to have docstrings in tests. Yeah!
testsuite/MDAnalysisTests/topology/test_mmtf.py Outdated
@@ -1,19 +1,22 @@
from __future__ import absolute_import
from numpy.testing import (

This comment has been minimized.

Sign in to view
@jbarnoud

jbarnoud Jun 23, 2017
Contributor

Please, fix the import order.
testsuite/MDAnalysisTests/topology/test_tprparser.py
@@ -63,42 +66,81 @@ class TPRBase(TPRAttrs):
# All these classes should be generated in a loop. Yet, nose test generation
# seems to work only with functions, and not with classes.
class TestTPR400(TPRBase):

__test__ = True

This comment has been minimized.

Sign in to view
@jbarnoud

jbarnoud Jun 23, 2017
Contributor

This all file will be much better with fixtures. Not for this PR, but this test suite will really profits from the move to pytest.
@utkbansal
Fix imports
Verified
This commit was signed with a verified signature.
utkbansal Utkarsh Bansal
GPG key ID: 359A4CFC0731150A Learn about signing commits
Loading status checks…
92cfc72
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 23, 2017

@jbarnoud @richardjgowers This should be ready to merge now.
@utkbansal
Import TestCase
Verified
This commit was signed with a verified signature.
utkbansal Utkarsh Bansal
GPG key ID: 359A4CFC0731150A Learn about signing commits
Loading status checks…
e400486
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 23, 2017

Apparently, the hostname "dropbox.com" cannot be resolved during the full build.
@kain88-de
Copy link
Member

@kain88-de kain88-de commented Jun 23, 2017

Apparently, the hostname "dropbox.com" cannot be resolved during the full build.

We need more information. This message alone doesn't give me much when the build is restarted. When did it try to access dropbox would be valuable information for example.
@jbarnoud
Copy link
Contributor

@jbarnoud jbarnoud commented Jun 23, 2017

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 23, 2017 •
edited 

if [[ $NAME == 'full' ]]; then \
      bash ./maintainer/install_hole.sh $TRAVIS_OS_NAME "${HOME}/${MDA_OPTPACKAGES}"; \
      HOLE_BINDIR="${HOME}/${MDA_OPTPACKAGES}/hole2/exe"; \
      export PATH=${PATH}:${HOLE_BINDIR}; \
  fi
  
Downloading hole2-NotForProfit-2.2.004-Linux-x86_64.tar.gz from https://www.dropbox.com/s/jukpwlohhi20r17/hole2-NotForProfit-2.2.004-Linux-x86_64.tar.gz?dl=1...
curl: (6) Couldn't resolve host 'www.dropbox.com'
[install_hole.sh] ERROR: Failed to download hole2-NotForProfit-2.2.004-Linux-x86_64.tar.gz from https://www.dropbox.com/s/jukpwlohhi20r17/hole2-NotForProfit-2.2.004-Linux-x86_64.tar.gz?dl=1 [1]

It tries to download some hole dependency from dropbox and that is a part of the ci-helpers we are using.
@jbarnoud
Copy link
Contributor

@jbarnoud jbarnoud commented Jun 23, 2017

@kain88-de @richardjgowers Can I merge, or should I wait for 0.16.2 to be released?
@jbarnoud
jbarnoud approved these changes Jun 23, 2017
View changes
@richardjgowers
Copy link
Member

@richardjgowers richardjgowers commented Jun 23, 2017

@jbarnoud yeah tests aren't user facing, so we can merge this
@richardjgowers
Copy link
Member

@richardjgowers richardjgowers commented Jun 23, 2017

@jbarnoud I take that back, maybe we're not doing pytest in 16.2... #1401
@orbeckst
Copy link
Member

@orbeckst orbeckst commented Jun 24, 2017 •
edited

#1420 (comment)

It tries to download some hole dependency from dropbox and that is a part of the ci-helpers we are using.

Just some background: hole is not part of the ci-helpers. It is an external software that we use for the MDAnalysis.analysis.hole module. In order to test the module, we need to install the software. The license of hole is very unfriendly so the only legal approach (as far as we could figure out) is to always download it from the official site, which happens to be a dropbox location...

Or at least that was the story until 2 minutes ago when I figured out that hole is now open source: https://github.com/osmart/hole2 ... so I think we will soon have conda packages. ;-) #1429
@richardjgowers richardjgowers merged commit c27ace3 into MDAnalysis:develop Jun 24, 2017
3 checks passed
3 checks passed
QuantifiedCode 14 minor issues introduced.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 88.856%
Details
@richardjgowers
Copy link
Member

@richardjgowers richardjgowers commented Jun 25, 2017

@utkbansal So this PR didn't actually add topology to the pytest list in travis.yml 😆
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 25, 2017

Oh man! Rebasing and stuff can be very confusing if you have 4 PRs in parallel. 😆
@richardjgowers
Copy link
Member

@richardjgowers richardjgowers commented Jun 25, 2017

Put it in the xdist pr
@jbarnoud
Copy link
Contributor

@jbarnoud jbarnoud commented Jun 25, 2017

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 25, 2017

@jbarnoud Makes more sense now!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbarnoud jbarnoud

Assignees

@jbarnoud jbarnoud

Labels
testing
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@utkbansal @kain88-de @jbarnoud @richardjgowers @orbeckst