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_util.py #1494

Merged
merged 23 commits into from Jul 29, 2017

Conversation

Projects
None yet
4 participants
@utkbansal
Member

utkbansal commented Jul 15, 2017

Fixes #

Changes made in this Pull Request:

PR Checklist

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

@utkbansal utkbansal changed the title from Pytest Style: test_util.py to [WIP]Pytest Style: test_util.py Jul 15, 2017

@utkbansal

@kain88-de @richardjgowers @jbarnoud @mnmelo Please have a look at this fixture I've created. I have used lambdas to save myself from writing a function that sets up the data and is then called in parametrize. Is this style okay (it works fine)?

map(lambda format: format[0], formats)) +
map(lambda ext: ext.lower(), map(lambda format: format[0], formats)
))
def test_get_extention(self, ext):

This comment has been minimized.

@utkbansal

utkbansal Jul 16, 2017

Member

This is the new function. (FYI this will be done directly on _check_get_ext - this wrapper method will not be there in the final version).

This comment has been minimized.

@utkbansal

utkbansal Jul 16, 2017

Member

Turns out that I don't really need nested calls, I can just do

map(lambda format: format[0].upper(), formats) +
map(lambda format: format[0].lower(), formats)
for extention in [format.upper(), format.lower()]:
file_name = 'file.{0}'.format(extention)
# check extention doesn't trip up get_ext or guess_format
yield self._check_get_ext, extention, file_name

This comment has been minimized.

@utkbansal

utkbansal Jul 16, 2017

Member

This call is replaced by that fixture funciton.

@jbarnoud

I had just a quick check. Some changes, though.

@@ -199,7 +199,6 @@ enable=abstract-class-instantiated,
print-statement,
xrange-builtin,
zip-builtin-not-iterating,
map-builtin-not-iterating,

This comment has been minimized.

@jbarnoud

jbarnoud Jul 16, 2017

Contributor

You should not remove that check.

This comment has been minimized.

@utkbansal

utkbansal Jul 16, 2017

Member

That check is erroring out the build. Do you know of any alternate way to write the labdas I've written?

This comment has been minimized.

@richardjgowers

richardjgowers Jul 16, 2017

Member

Just make the list somewhere in the module namespace, you keep reusing it anyway

This comment has been minimized.

@jbarnoud

jbarnoud Jul 16, 2017

Contributor

Look bellow. It probably fails because you use + on map. In python 3, map is an iterator so you cannot use + on it. You need to convert the map output to a list to extend it.

By the way, if you use map, you should import it from six.

In general, you should never remove checks from pylint. If you understand exactly why the test fail, and you are sure about why you are doing, you may deactivate the check locally.

This comment has been minimized.

@kain88-de

kain88-de Jul 16, 2017

Member

Instead of a map you can also use a normal list comprehension.

This comment has been minimized.

@utkbansal

utkbansal Jul 16, 2017

Member

List comprehensions dont work. TypeError: zip argument #1 must support iteration

def _check_compressed(self, f, fn):
@pytest.mark.parametrize('extention',
map(lambda format: format[0].upper(), formats) +

This comment has been minimized.

@jbarnoud

jbarnoud Jul 16, 2017

Contributor

The + will break on python 3. You need to consume the iterator in a list.

This comment has been minimized.

@kain88-de

kain88-de Jul 16, 2017

Member

turn it into a list comprehension [f[0].upper() for f in formats]. Please also try not to use keywords of the language itself.

This comment has been minimized.

@utkbansal

utkbansal Jul 16, 2017

Member

@kain88-de List comprehensions don't work inside parametrize calls. I get a TypeError.

assert_equal(self.obj.val3, self.obj.ref3)
assert_equal('val3' in self.obj._cache, True)
assert obj.val3 == obj.ref3
assert ('val3' in obj._cache) == True

This comment has been minimized.

@jbarnoud

jbarnoud Jul 16, 2017

Contributor

'val3' in obj._cache is enough. The == True is useless.

This comment has been minimized.

@jbarnoud

jbarnoud Jul 16, 2017

Contributor

Same above.

def test_convert_aa_code_long(self):
for resname1, strings in self.aa:
for resname3 in strings:
yield check_convert_aa_3to1, resname3, resname1
yield self.check_convert_aa_3to1, resname3, resname1

This comment has been minimized.

@kain88-de

kain88-de Jul 16, 2017

Member

doesn't the yield still raise a internal pytest warning? We should remove all the yields.

This comment has been minimized.

@utkbansal

utkbansal Jul 16, 2017

Member

It is marked as TODO.

# self.e2 = np.array([0, 1., 0])
# self.e3 = np.array([0, 0, 1.])
# self.a = np.array([np.cos(np.pi / 3), np.sin(np.pi / 3), 0])
# self.null = np.zeros(3)

This comment has been minimized.

@kain88-de

kain88-de Jul 16, 2017

Member

please remove these comments.

class TestGeometryFunctions(object):
e1 = np.array([1., 0, 0])
e2 = np.array([0, 1., 0])
e3 = np.array([0, 0, 1.])

This comment has been minimized.

@kain88-de

kain88-de Jul 16, 2017

Member

more concise would be e1, e2, e3 = np.eye(3)

# ])
# def test_normal(self, vec1, vec2, value):
# assert mdamath.normal(vec1, vec2) == value
# # add more non-trivial tests

This comment has been minimized.

@kain88-de

kain88-de Jul 16, 2017

Member

why does this fail?

assert_equal(ret, self.obj.ref1)
obj._clear_caches()
ret = obj.val1()
assert ('val1' in obj._cache) == True

This comment has been minimized.

@kain88-de

kain88-de Jul 16, 2017

Member

as @jbarnoud mentioned you don't need to do assert foo == True just using assert foo is enough

This comment has been minimized.

@kain88-de

kain88-de Jul 16, 2017

Member

You should check the whole file for this pattern

def _load_bonds(self):
def _load_bonds(self, universe):

This comment has been minimized.

@richardjgowers

richardjgowers Jul 16, 2017

Member

Could we make a fixture called universe_with_bonds which is just the universe fixture and this function called? Ie put the body of this method into a fixture and use that instead?

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jul 20, 2017

I am able to get rid of lambda functions but oddly my linter complaints about references. Let's see what the Travis lint build says.

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jul 20, 2017

@richardjgowers @kain88-de Can you help me with these lint errors

testsuite/MDAnalysisTests/lib/test_util.py:596: [E1136(unsubscriptable-object), TestGuessFormat.test_get_extention] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:597: [E1136(unsubscriptable-object), TestGuessFormat.test_get_extention] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:606: [E1136(unsubscriptable-object), TestGuessFormat.test_compressed_without_compression_extention] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:607: [E1136(unsubscriptable-object), TestGuessFormat.test_compressed_without_compression_extention] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:615: [E1136(unsubscriptable-object), TestGuessFormat.test_compressed] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:616: [E1136(unsubscriptable-object), TestGuessFormat.test_compressed] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:626: [E1136(unsubscriptable-object), TestGuessFormat.test_guess_format] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:626: [E1136(unsubscriptable-object), TestGuessFormat.test_guess_format] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:634: [E1136(unsubscriptable-object), TestGuessFormat.test_guess_format_compressed] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:634: [E1136(unsubscriptable-object), TestGuessFormat.test_guess_format_compressed] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:643: [E1136(unsubscriptable-object), TestGuessFormat.test_get_parser] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:643: [E1136(unsubscriptable-object), TestGuessFormat.test_get_parser] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:643: [E1136(unsubscriptable-object), TestGuessFormat.test_get_parser] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:652: [E1136(unsubscriptable-object), TestGuessFormat.test_get_parser_compressed] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:652: [E1136(unsubscriptable-object), TestGuessFormat.test_get_parser_compressed] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:652: [E1136(unsubscriptable-object), TestGuessFormat.test_get_parser_compressed] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:662: [E1136(unsubscriptable-object), TestGuessFormat.test_get_parser_invalid] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:662: [E1136(unsubscriptable-object), TestGuessFormat.test_get_parser_invalid] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:662: [E1136(unsubscriptable-object), TestGuessFormat.test_get_parser_invalid] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:670: [E1136(unsubscriptable-object), TestGuessFormat.test_get_reader] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:670: [E1136(unsubscriptable-object), TestGuessFormat.test_get_reader] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:670: [E1136(unsubscriptable-object), TestGuessFormat.test_get_reader] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:679: [E1136(unsubscriptable-object), TestGuessFormat.test_get_reader_compressed] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:679: [E1136(unsubscriptable-object), TestGuessFormat.test_get_reader_compressed] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:679: [E1136(unsubscriptable-object), TestGuessFormat.test_get_reader_compressed] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:689: [E1136(unsubscriptable-object), TestGuessFormat.test_get_reader_invalid] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:689: [E1136(unsubscriptable-object), TestGuessFormat.test_get_reader_invalid] Value 'format' is unsubscriptable
testsuite/MDAnalysisTests/lib/test_util.py:689: [E1136(unsubscriptable-object), TestGuessFormat.test_get_reader_invalid] Value 'format' is unsubscriptable

My IDE also complaints the same, but it is subscriptable! The tests pass on my system.

@richardjgowers

This comment has been minimized.

Member

richardjgowers commented Jul 20, 2017

@utkbansal it's because format is a builtin function, and the linter doesn't understand that you've shadowed it. Change it to something else and it should be fine

('TRZ', None, mda.coordinates.TRZ.TRZReader),
]
# list of possible compressed extensions
# include no extension too!
compressed_extensions = ['.bz2', '.gz']
def _check_get_ext(self, f, fn):
@pytest.mark.parametrize('extention', [format[0].upper() for format in formats] +

This comment has been minimized.

@kain88-de

kain88-de Jul 20, 2017

Member

what happens if you choose another name. A static analyzer might confuse this with the built-in format.

This comment has been minimized.

@utkbansal

utkbansal Jul 20, 2017

Member

@kain88-de @richardjgowers Let's say I rename format to foo. Then the linter complaints Unresolved reference The tests still work though.

This comment has been minimized.

@kain88-de

kain88-de Jul 20, 2017

Member

I don't believe that. Please give it a try and let travis run on it.

This comment has been minimized.

@utkbansal

@utkbansal utkbansal changed the title from [WIP]Pytest Style: test_util.py to Pytest Style: test_util.py Jul 20, 2017

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jul 20, 2017

@kain88-de @richardjgowers The build passed. I still have lint errors in my IDE though. Is there anything else that needs to be done here?

def check_convert_aa_1to3(resname1, resname3_canonical):
assert_equal(util.convert_aa_code(resname1), resname3_canonical)
def test_convert_aa_code_long_data():

This comment has been minimized.

@richardjgowers

richardjgowers Jul 20, 2017

Member

Is this getting also ran as a test because of the name?

Could this get rewritten as a generator?

This comment has been minimized.

@utkbansal

utkbansal Jul 22, 2017

Member

Can think of a list comprehension to replace that.

@jbarnoud

You need to rebase against develop and to fix the order of the imports.

data = []
for resname1, strings in aa:
for resname3 in strings:
data.append((resname3, resname1))

This comment has been minimized.

@jbarnoud

jbarnoud Jul 22, 2017

Contributor

This could be a generator. Would not change much, though.

This comment has been minimized.

@utkbansal

utkbansal Jul 27, 2017

Member

@kain88-de @jbarnoud I need help converting this to a list comprehension.

This comment has been minimized.

@jbarnoud

jbarnoud Jul 27, 2017

Contributor

The point is not to make it a list comprehension, but a generator. To do that, replace this line by yield (resname3, resname1), remove the data list, and remove the return. But as I said, it does not change much, I like the generator better but the list is fine too.

import MDAnalysis.lib.mdamath as mdamath
from MDAnalysis.lib.util import cached
import MDAnalysis.lib.util as util

This comment has been minimized.

@jbarnoud

jbarnoud Jul 22, 2017

Contributor

The imports are disordered.

def test_VE_1(self):
assert_raises(ValueError, util.convert_aa_code, 'XYZXYZ')
with pytest.raises(ValueError):

This comment has been minimized.

@jbarnoud

jbarnoud Jul 22, 2017

Contributor

test_VE_1 and test_VE_2 test the same thing, it could be a parametrized test. It could also be renamed to test_ValueError or something.

])
def test_string(self, name, ext, keep, actual_name):
file_name = util.filename(name, ext, keep)
assert file_name, actual_name #TODO: why is a comma here?

This comment has been minimized.

@kain88-de

kain88-de Jul 23, 2017

Member

Please fix the todo

@kain88-de

This comment has been minimized.

Member

kain88-de commented Jul 26, 2017

@utkbansal can you finish this up.

utkbansal added some commits Jul 15, 2017

utkbansal added some commits Jul 18, 2017

@@ -605,103 +591,138 @@ class TestGuessFormat(object):
('XYZ', mda.topology.XYZParser.XYZParser, mda.coordinates.XYZ.XYZReader),
]
if six.PY2:
# DCD, LAMMPS, and TRZ are not supported on Python 3 yet
# TRZ is not supported on Python 3 yet

This comment has been minimized.

@jbarnoud

jbarnoud Jul 27, 2017

Contributor

Do we still need that?

This comment has been minimized.

@richardjgowers

richardjgowers Jul 27, 2017

Member

Everything is supported in py3, this should get rebased onto develop

This comment has been minimized.

@utkbansal

utkbansal Jul 27, 2017

Member

So should I remove the check?

@@ -817,32 +819,38 @@ def _check_multiframe_fails(fmt):
]
if six.PY2:

This comment has been minimized.

@richardjgowers

richardjgowers Jul 27, 2017

Member

this if can get removed too

@jbarnoud

This comment has been minimized.

Contributor

jbarnoud commented Jul 27, 2017

for resname1, strings in aa:
for resname3 in strings:
data.append((resname3, resname1))
yield (resname3, resname1)

This comment has been minimized.

@kain88-de

kain88-de Jul 27, 2017

Member

Wasn't the goal to remove yield?

This comment has been minimized.

@utkbansal

utkbansal Jul 27, 2017

Member

No, this isn't a yield based test. @jbarnoud recommended this #1494 (comment)

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jul 27, 2017

@kain88-de @jbarnoud Better now?

@jbarnoud

Sorry, a few more comments. In addition, all the tests that have a exception name abbreviated in their name could be rename to avoid the abbreviation. There are a few tests named test_*_VE or test_*_TE.

# Does ``get_writer_for`` fails with non string filename, no format
with pytest.raises(ValueError):
mda.coordinates.core.get_writer_for(filename = StringIO(), format = None)

This comment has been minimized.

@jbarnoud

jbarnoud Jul 27, 2017

Contributor

There should not be spaces around the =.

# does ``get_writer_for`` fail with invalid format and multiframe not None
with pytest.raises(TypeError):
mda.coordinates.core.get_writer_for(filename = "fail_me", format = 'UNK', multiframe = True)

This comment has been minimized.

@jbarnoud

jbarnoud Jul 27, 2017

Contributor

No spaces.

# does ``get_writer_for`` fail with invalid format and multiframe not None
with pytest.raises(TypeError):
mda.coordinates.core.get_writer_for(filename = "fail_me", format = 'UNK', multiframe = True)
mda.coordinates.core.get_writer_for(filename = "fail_me", format = 'UNK', multiframe = False)

This comment has been minimized.

@jbarnoud

jbarnoud Jul 27, 2017

Contributor

No spaces.

mda.coordinates.core.get_writer_for,
'this', format=fmt, multiframe=True)
with pytest.raises(ValueError):
mda.coordinates.core.get_writer_for(filename = 'this.gro', multiframe = 'sandwich')

This comment has been minimized.

@jbarnoud

jbarnoud Jul 27, 2017

Contributor

No spaces.

def test_setattr(self):
self.ns.this = 42
with pytest.raises(AttributeError):
deller()

This comment has been minimized.

@jbarnoud

jbarnoud Jul 27, 2017

Contributor

Does not need to be a nested function.

assert_equal(ret['delta'], 0.4)
assert_almost_equal(ret['min'], 3.9)
assert_almost_equal(ret['max'], 5.1)
with pytest.raises(ValueError):

This comment has been minimized.

@jbarnoud

jbarnoud Jul 27, 2017

Contributor

Rename test_VE to test_ValueError.

@kain88-de

This comment has been minimized.

Member

kain88-de commented Jul 29, 2017

@utkbansal this isn't fully resolved yet

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jul 29, 2017

@kain88-de @jbarnoud Fixed the whitespace and naming issues.

@kain88-de kain88-de merged commit a1e49ba into MDAnalysis:develop Jul 29, 2017

3 checks passed

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