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

[Review Needed]Pytest Style topology/test_gro.py #1590

Merged
merged 3 commits into from Aug 15, 2017

Conversation

@utkbansal
Copy link
Member

@utkbansal utkbansal commented Aug 5, 2017

Fixes #

Changes made in this Pull Request:

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?
Copy link
Contributor

@jbarnoud jbarnoud left a comment

One more file about to bite the dust. A few things to fix, though.

@@ -54,11 +48,11 @@ class TestGROParser(ParserBase):

def test_attr_size(self, top):
for attr in ['ids', 'names']:

This comment has been minimized.

@jbarnoud

jbarnoud Aug 5, 2017
Contributor

The loops are meaningless: the attr variable is not used.

resids = [1, 99999, 100000, 100001, 199999, 200000, 200001]

assert_(top.tt.size == (126, 7, 1))
assert top.tt.size == (126, 7, 1)
for i, (r, n, l) in enumerate(zip(resids, self.names, self.lengths)):

This comment has been minimized.

@jbarnoud

jbarnoud Aug 5, 2017
Contributor

We probably can compare the full arrays rather than comparing element per element.

def test_sameresid_diffresname(args):
i = args[0]
resid = args[1][0]
resname = args[1][1]

This comment has been minimized.

@jbarnoud

jbarnoud Aug 5, 2017
Contributor

It may be clearer written like: i, (resid, resname) = args.


def test_sameresid_diffresname():
@pytest.mark.parametrize(

This comment has been minimized.

@richardjgowers

richardjgowers Aug 5, 2017
Member

this doesn't really need to be parametrized, it all tests the same thing, rather than different cases of something

utkbansal added 2 commits Aug 8, 2017
@jbarnoud jbarnoud merged commit 69730f4 into MDAnalysis:develop Aug 15, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 90.575%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants