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

Remove yield based tests #1642

Merged
merged 3 commits into from Sep 4, 2017

Conversation

Projects
None yet
4 participants
@utkbansal
Copy link
Member

utkbansal commented Aug 21, 2017

Fixes #

Changes made in this Pull Request:

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?
@@ -0,0 +1,4 @@
import itertools

This comment has been minimized.

@orbeckst

orbeckst Aug 21, 2017

Member

I think this file should not be in the commit.

@@ -714,7 +714,12 @@ def _from_coords(self, p, v, f):

return ts

def _check_from_coordinates(self, p, v, f):
@pytest.mark.parametrize('p, v, f',

This comment has been minimized.

@richardjgowers

richardjgowers Aug 22, 2017

Member

if we keep reusing the pattern of itertools.product() it's probably better to define it as a generator outside the class

This comment has been minimized.

@kain88-de

kain88-de Sep 3, 2017

Member

I changed this to a fixture some_ts in the other occurrences. Here this isn't possible.

@orbeckst

This comment has been minimized.

Copy link
Member

orbeckst commented Aug 26, 2017

@utkbansal remove the WIP from the title when you want reviews.

@utkbansal

This comment has been minimized.

Copy link
Member

utkbansal commented Sep 1, 2017

Update: Need help with the five remaining yield occurrences. All of them are in BaseTimestepTest.

@orbeckst

This comment has been minimized.

Copy link
Member

orbeckst commented Sep 1, 2017

Can you paste the link to the specific code pieces (from the GitHub source).

@kain88-de

This comment has been minimized.

Copy link
Member

kain88-de commented Sep 3, 2017

Patch to remove remaining yield tests

EDIT force pushed changes

@kain88-de kain88-de force-pushed the utkbansal:coord-base branch 3 times, most recently from 8430b28 to 8ce5204 Sep 3, 2017

@kain88-de kain88-de force-pushed the utkbansal:coord-base branch from 8ce5204 to e6c420b Sep 3, 2017

ts = self._from_coords(p, v, f)
@pytest.fixture(params=filter(any,
itertools.product([True, False], repeat=3)))
def some_ts(self, request):

This comment has been minimized.

@kain88-de

kain88-de Sep 3, 2017

Member

I don't have a better name for this fixture. Also this can only be a function scope fixture.

ts1.forces = self.reffor.copy()
ts2.forces = self.reffor.copy()

self._check_ts_equal(ts1, ts2, '')

This comment has been minimized.

@richardjgowers

richardjgowers Sep 4, 2017

Member

Should inline _check_ts_equal

This comment has been minimized.

@kain88-de

kain88-de Sep 4, 2017

Member

So you mean write directly

assert ts1 == ts2
assert ts2 == ts1

We use the function in two separate places and it does a nice assert that the equal is commutative. Maybe better we make it a stand alone function (not a class method) named assert_ts_equal

def assert_ts_equal(ts1, ts2, msg=''):
    assert ts1 == ts2, msg
    assert ts2 == ts1, msg

This comment has been minimized.

@richardjgowers

richardjgowers Sep 4, 2017

Member

I can only see it used once? Either way yeah, let's split it out into an assert function

This comment has been minimized.

@kain88-de

kain88-de Sep 4, 2017

Member

https://github.com/MDAnalysis/mdanalysis/search?q=_check_ts_equal&type=Code&utf8=%E2%9C%93

The other usage is in a different file. That makes it hard to see. I admit.


def _check_bad_slice(self, p, v, f):
ts = self._from_coords(p, v, f)
@pytest.fixture(params=filter(any,

This comment has been minimized.

@richardjgowers

richardjgowers Sep 4, 2017

Member

does this filter do the same as the other itertools.product call above? If so can we use just one way of doing this (don't care which)

This comment has been minimized.

@kain88-de

kain88-de Sep 4, 2017

Member
filter(any, itertools.product([True, False], repeat=3))

is equivalent to

[pvf for pvf in itertools.product([True, False], repeat=3) if any(pvf)]

I personally like the filter call here because it is the clearest to read with the indentations due to being a decorator argument. But the problem is that we can't use tuple unpacking like in the list-comprehension above. I found the tuple unpacking more readable in the above example so I left it.

So yeah they are slightly differently written but personally this gives the best readability in each case. I can unify it though if you like. What would you prefer the filter or list-comprehension?

This comment has been minimized.

@richardjgowers

richardjgowers Sep 4, 2017

Member

Sure, lets use filter then. It's just confusing to see two ways for this, implies that maybe there's a difference

This comment has been minimized.

@kain88-de

kain88-de Sep 4, 2017

Member

changed.


def test_copy_slice_no_yield(self, some_ts):
ts = some_ts
self._check_copy(self.name, ts)

This comment has been minimized.

@richardjgowers

richardjgowers Sep 4, 2017

Member

I'd rather all these method calls became separate tests

This comment has been minimized.

@kain88-de

kain88-de Sep 4, 2017

Member

Why? Just for style reasons?

This comment has been minimized.

@richardjgowers

richardjgowers Sep 4, 2017

Member

What if one of the _check calls alters ts?

@@ -701,7 +701,7 @@ def test_forces_remove(self):
with pytest.raises(NoDataError):
getattr(ts, 'forces')

def _empty_ts(self):
def check_ts(self):

This comment has been minimized.

@richardjgowers

richardjgowers Sep 4, 2017

Member

Does this get detected by pytest? Shouldn't it start with test_

@richardjgowers richardjgowers self-assigned this Sep 4, 2017

@kain88-de kain88-de force-pushed the utkbansal:coord-base branch 2 times, most recently from a5906fa to 14ba61f Sep 4, 2017

@kain88-de kain88-de changed the title [WIP]Remove yield based tests Remove yield based tests Sep 4, 2017

kain88-de added some commits Sep 3, 2017

replace yield based tests
use fixture for timestep instead of parametrize
add assert_timestep_equal
standalone function to replace _check_ts_equal

@kain88-de kain88-de force-pushed the utkbansal:coord-base branch from 6663762 to bf6df36 Sep 4, 2017

@kain88-de kain88-de merged commit b786cd3 into MDAnalysis:develop Sep 4, 2017

2 checks passed

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