/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

Port auxiliary module to pytest #1439

Merged
merged 7 commits into from Jun 28, 2017

Conversation

Reviewers

@kain88-de kain88-de

@richardjgowers richardjgowers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@utkbansal @richardjgowers @kain88-de
@utkbansal
Member

utkbansal commented Jun 27, 2017

Fixes #

Changes made in this Pull Request:

PR Checklist

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

This comment has been minimized.

Sign in to view
Member

utkbansal commented Jun 27, 2017

@kain88-de @richardjgowers Have a look at this.
@richardjgowers

This comment has been minimized.

Sign in to view
Member

richardjgowers commented Jun 27, 2017

@utkbansal it looks like you might have to turn BaseAuxReference into a fixture. Then the tests in BaseAuxReader should all be parametrised to use it, rather than referring to it through self.

I wouldn't blindly put everything from BaseAuxReference into the fixture though, there's a lot of junk in there that could get moved out, for example iter_list isn't actually reference data, and that could exist only in the test methods which use it.
@utkbansal

This comment has been minimized.

Sign in to view
Member

utkbansal commented Jun 27, 2017

I'm guessing, the error is arising because setUp can't take arguments?!
@utkbansal

This comment has been minimized.

Sign in to view
Member

utkbansal commented Jun 27, 2017 •
edited

@richardjgowers Okay, I'll try that.

Interesting to note that the minimal build exits once the pytest suite fails and it doesn't try to run the nose test suite. This is against our assumptions!
testsuite/MDAnalysisTests/auxiliary/base.py Outdated
@@ -34,14 +37,17 @@ def test_get_bad_auxreader_format_raises_ValueError():
# should raise a ValueError when no AuxReaders with match the specified format
mda.auxiliary.core.get_auxreader_for(format='bad-format')
class BaseAuxReference(object):
class BaseAuxReference(TestCase):

This comment has been minimized.

Sign in to view
Copy link
@kain88-de

kain88-de Jun 27, 2017

Member

this contains reference data. No need to declare it as a test class
testsuite/MDAnalysisTests/auxiliary/test_xvg.py Outdated
@@ -33,8 +33,11 @@
from MDAnalysisTests.auxiliary.base import (BaseAuxReaderTest, BaseAuxReference)
class XVGReference(BaseAuxReference):
def __init__(self):

This comment has been minimized.

Sign in to view
Copy link
@kain88-de

kain88-de Jun 27, 2017

Member

these are normal classes.

utkbansal added some commits Jun 27, 2017

@utkbansal
Use fixtures
Verified
This commit was signed with a verified signature.
@utkbansal utkbansal Utkarsh Bansal
GPG key ID: 359A4CFC0731150A Learn about signing commits
853f646
@utkbansal
Move auxiliary to PYTEST_LIST
Verified
This commit was signed with a verified signature.
@utkbansal utkbansal Utkarsh Bansal
GPG key ID: 359A4CFC0731150A Learn about signing commits
b2049d8
@utkbansal

This comment has been minimized.

Sign in to view
Member

utkbansal commented Jun 27, 2017

@kain88-de @richardjgowers I have made some progress. I still have 3 test cases failing. I'm looking into that. A quick review would be helpful. basically what I want to know is - if this the direction we want to proceed in?
@utkbansal
Drop the module scope of the fixture
Verified
This commit was signed with a verified signature.
@utkbansal utkbansal Utkarsh Bansal
GPG key ID: 359A4CFC0731150A Learn about signing commits
Loading status checks…
0979f73
@utkbansal

This comment has been minimized.

Sign in to view
Member

utkbansal commented Jun 27, 2017

Got those errors, apparently having the scope set to module causes some problem. Maybe some methods are updating some attributes?
@richardjgowers

This comment has been minimized.

Sign in to view
Member

richardjgowers commented Jun 27, 2017

Don't worry about scope yet, that's an optimisation
@utkbansal
Use fixtures in TestXVGFileReader
Verified
This commit was signed with a verified signature.
@utkbansal utkbansal Utkarsh Bansal
GPG key ID: 359A4CFC0731150A Learn about signing commits
Loading status checks…
97a1304
@utkbansal

This comment has been minimized.

Sign in to view
Member

utkbansal commented Jun 27, 2017

This should fix the drop in coverage. I still have to do some cleanup stuff.
@kain88-de

This comment has been minimized.

Sign in to view
Member

kain88-de commented Jun 27, 2017

@utkbansal. There is a possible minimal fix in #1440. You can just take the code. No need for commit attribution. Choose better names though! Simply doing the init/setup in a different named method fixes everything. As long as coverage doesn't drop this is OK for now but should be fixed with a better solution long term.
@utkbansal
Minor cleanups
Verified
This commit was signed with a verified signature.
@utkbansal utkbansal Utkarsh Bansal
GPG key ID: 359A4CFC0731150A Learn about signing commits
Loading status checks…
8f3aba5
@utkbansal

This comment has been minimized.

Sign in to view
Member

utkbansal commented Jun 27, 2017

@kain88-de @richardjgowers All green and no drop in coverage! Have cleaned this up. Please review.

@utkbansal utkbansal changed the title from [WIP]Port auxiliary module to pytest to Port auxiliary module to pytest Jun 27, 2017

@richardjgowers

richardjgowers approved these changes Jun 27, 2017
View changes

Looks good

testsuite/MDAnalysisTests/auxiliary/test_xvg.py
def test_changing_n_col_raises_ValueError(self):
@staticmethod
@pytest.fixture()
def ref():

This comment has been minimized.

Sign in to view
Copy link
@kain88-de

kain88-de Jun 27, 2017

Member

what happens when I define a global fixture ref in the same file?

This comment has been minimized.

Sign in to view
Copy link
@utkbansal

utkbansal Jun 27, 2017

Member

I tried making both of them global. 3 tests failed. So I switched back.

This comment has been minimized.

Sign in to view
Copy link
@kain88-de

kain88-de Jun 27, 2017

Member

So if I now create a global fixture the tests will break?
testsuite/MDAnalysisTests/auxiliary/test_xvg.py Outdated
def reader(ref):
return ref.reader(ref.testdata, initial_time=ref.initial_time,
dt=ref.dt, auxname=ref.name,
time_selector=None, data_selector=None)

This comment has been minimized.

Sign in to view
Copy link
@kain88-de

kain88-de Jun 27, 2017

Member

indentation
@utkbansal
Indentation fixes
Verified
This commit was signed with a verified signature.
@utkbansal utkbansal Utkarsh Bansal
GPG key ID: 359A4CFC0731150A Learn about signing commits
Loading status checks…
27a9cb2
@utkbansal

This comment has been minimized.

Sign in to view
Member

utkbansal commented Jun 27, 2017

@richardjgowers Should be ready to be merged now.
@utkbansal

This comment has been minimized.

Sign in to view
Member

utkbansal commented Jun 27, 2017 •
edited

Can someone explain why test_xvg_bz2 function has the same name as another function just above it!? https://github.com/utkbansal/mdanalysis/blob/27a9cb2ed3a0ad34b19671c28522c6a4c7c4a365/testsuite/MDAnalysisTests/auxiliary/test_xvg.py#L132

Looks like a bug.

@utkbansal utkbansal changed the title from Port auxiliary module to pytest to [WIP]Port auxiliary module to pytest Jun 27, 2017

@kain88-de

This comment has been minimized.

Sign in to view
Member

kain88-de commented Jun 27, 2017

Can someone explain why test_xvg_bz2 function has the same name as another function just above it!?

Looks like a mistake. You can rename the functions.
testsuite/MDAnalysisTests/auxiliary/test_xvg.py Outdated
def test_xvg_bz2():
reader = mda.auxiliary.XVG.XVGReader(XVG_BZ2)
assert_array_equal(reader.read_all_times(), np.array([0., 50., 100.]))
def test_xvg_bz2():

This comment has been minimized.

Sign in to view
Copy link
@kain88-de

kain88-de Jun 28, 2017

Member

If you rename the function so that the test is run as well the PR is good to go.
@utkbansal
Fix function name repetition
Verified
This commit was signed with a verified signature.
@utkbansal utkbansal Utkarsh Bansal
GPG key ID: 359A4CFC0731150A Learn about signing commits
Loading status checks…
1e92b2b

@utkbansal utkbansal changed the title from [WIP]Port auxiliary module to pytest to Port auxiliary module to pytest Jun 28, 2017

@utkbansal

This comment has been minimized.

Sign in to view
Member

utkbansal commented Jun 28, 2017

@richardjgowers @kain88-de Can be merged now!

@richardjgowers richardjgowers merged commit be8db1c into MDAnalysis:develop Jun 28, 2017
3 checks passed

3 checks passed

QuantifiedCode 38 minor issues introduced.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 89.561%
Details
@richardjgowers

This comment has been minimized.

Sign in to view
Member

richardjgowers commented Jun 28, 2017

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