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

Pytest Style: test_modelling.py #1468

Merged
merged 4 commits into from Jul 13, 2017

Conversation

Projects
None yet
4 participants
@utkbansal
Member

utkbansal commented Jul 10, 2017

Fixes #

Changes made in this Pull Request:

PR Checklist

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

@orbeckst orbeckst added the testing label Jul 11, 2017

@orbeckst

lgtm ... but perhaps @jbarnoud or @kain88-de should have a quick look, too.

@@ -266,9 +264,9 @@ def test_merge_with_topology_from_different_universes(self):
assert_(not hasattr(u3.atoms, 'dihedrals') or len(u3.atoms.bonds) == 0)
assert_(not hasattr(u3.atoms, 'impropers') or len(u3.atoms.bonds) == 0)

This comment has been minimized.

@kain88-de

kain88-de Jul 11, 2017

Member

these asserts should be replaced

This comment has been minimized.

@kain88-de

kain88-de Jul 11, 2017

Member

all of them in the whole file.

def test_emptyAG_ValueError(self):
u = self.universes[0]
def test_emptyAG_ValueError(self, universes):
u = universes[0]
a = AtomGroup([], u)
b = AtomGroup([], u)
assert_raises(ValueError, Merge, a, b)

This comment has been minimized.

@kain88-de

kain88-de Jul 11, 2017

Member

please use with pytest.raises

@@ -215,30 +213,30 @@ def test_empty_ValueError(self):
def test_nonsense_TypeError(self):
assert_raises(TypeError, Merge, ['1', 2])

This comment has been minimized.

@kain88-de

kain88-de Jul 11, 2017

Member

with pytest.raises

ids_new2 = [a.index for a in u.atoms]
assert_equal(ids_new, ids_new2)
def test_merge_same_universe(self):
u1, _, _ = self.universes
def test_merge_same_universe(self, universes):

This comment has been minimized.

@kain88-de

kain88-de Jul 11, 2017

Member

only use u1 as a fixture here

return [u1, u2, u3]
def test_merge(self, universes, tmpdir):
u1, u2, u3 = universes

This comment has been minimized.

@kain88-de

kain88-de Jul 11, 2017

Member

please use the fixtures explicit instead of the universes collection

@staticmethod
@pytest.fixture()
def u1():

This comment has been minimized.

@jbarnoud

jbarnoud Jul 11, 2017

Contributor

Since we're at it, could you rename u1, u2, and u3 to u_protein, u_ligand, and u_water, respectively. The current names are not informative, and there is no way anybody will remember which of u2 and u3 has the ligand.

u0 = MDAnalysis.Merge(u1.atoms, u1.atoms, u1.atoms)
assert_equal(len(u0.atoms), 3 * len(u1.atoms))
assert_equal(len(u0.residues), 3 * len(u1.residues))
assert_equal(len(u0.segments), 3 * len(u1.segments))
def test_residue_references(self):
u1, u2, u3 = self.universes
def test_residue_references(self, universes):

This comment has been minimized.

@jbarnoud

jbarnoud Jul 11, 2017

Contributor

This test does not use u3. Pass u1 and u2 explicitly.

m = Merge(u1.atoms, u2.atoms)
assert_equal(m.atoms.residues[0].universe, m,
"wrong universe reference for residues after Merge()")
def test_segment_references(self):
u1, u2, u3 = self.universes
def test_segment_references(self, universes):

This comment has been minimized.

@jbarnoud

jbarnoud Jul 11, 2017

Contributor

Same as above. Pass u1 and u2 only.

del self.u2
@staticmethod
@pytest.fixture()
def u2():

This comment has been minimized.

@jbarnoud

jbarnoud Jul 11, 2017

Contributor

This is the same as TestCaping.u2. You can move TestCaping.u1, TestCaping.u2, and TestCaping.u3 in the module scope instead of the scope of the scope of the TestCaping class. Then you can use the same fixture thoughout the file and we know they correspind to the same things. Do not forget the renaming I asked above, though.

def test_merge_with_topology_from_different_universes(self):
u3 = MDAnalysis.Merge(self.u.atoms[:110], self.u2.atoms)
def test_merge_with_topology_from_different_universes(self, u, u2):
u3 = MDAnalysis.Merge(u.atoms[:110], u2.atoms)

This comment has been minimized.

@jbarnoud

jbarnoud Jul 11, 2017

Contributor

If u1 and u2 are moved and renamed, this cannot remain u3. Maybe u_merge?

This comment has been minimized.

@utkbansal

utkbansal Jul 11, 2017

Member

I think I haven't understood what you said here. Can you confirm the change I've done is the one you wanted.

This comment has been minimized.

@jbarnoud

jbarnoud Jul 11, 2017

Contributor

This variable was named u3 because the function was already using self.u and self.u2. Beside the fact that u3 is a terrible name for a variable in general, there is no u2 any more as one could expect from the name u3.

This variable cannot keep be called u3. I suggest renaming it u_merge. u2 in the test above and the test bellow could be renamed u_merge as well. This name makes it clear that they point to the universe produced by the merge.

@staticmethod
@pytest.fixture()
def universes(u1, u2, u3):

This comment has been minimized.

@jbarnoud

jbarnoud Jul 11, 2017

Contributor

As @kain88-de suggests, you should use the individual fixtures explicitly. You can remove this one.

from numpy.testing import (TestCase, dec, assert_equal, assert_raises, assert_,
from numpy.testing import (assert_equal, assert_raises, assert_,

This comment has been minimized.

@kain88-de

kain88-de Jul 11, 2017

Member

Did you remove all assert_raises and assert_ from this file. Then they shouldn't be imported anymore

@jbarnoud

This comment has been minimized.

Contributor

jbarnoud commented Jul 12, 2017

There is still #1468 (comment) to address, then it should be good to go.

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jul 12, 2017

@jbarnoud Is it okay now?

@kain88-de kain88-de merged commit c6106f0 into MDAnalysis:develop Jul 13, 2017

3 checks passed

QuantifiedCode 10 minor issues introduced.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 90.337%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment