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_persistence.py #1470

Merged
merged 2 commits into from Jul 13, 2017

Conversation

Projects
None yet
3 participants
@utkbansal
Member

utkbansal commented Jul 10, 2017

Fixes #

Changes made in this Pull Request:

PR Checklist

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

This comment has been minimized.

Member

utkbansal commented Jul 10, 2017

@kain88-de Have a look at test_unpickle_missing in TestAtomGroupPickle. That case fails. I think that has something to do with del not working with fixtures?

# def setUp(self):

This comment has been minimized.

@richardjgowers

richardjgowers Jul 10, 2017

Member

This needs removing

This comment has been minimized.

@utkbansal

utkbansal Jul 10, 2017

Member

I haven't cleaned this up because the tests are still failing. Need to fix that first.

@orbeckst orbeckst added the testing label Jul 11, 2017

# Kill AtomGroup and Universe
del self.ag_n
del self.universe_n
del ag_n

This comment has been minimized.

@orbeckst

orbeckst Jul 11, 2017

Member

I don't think that del makes sense with these fixtures.

Try not to pass them as arguments and leave out the del lines. Maybe they never appear... but in this case, I have no idea what the deep pytest magic actually does.

You should also be able to remove the gc.collect() call.

This comment has been minimized.

@richardjgowers

richardjgowers Jul 12, 2017

Member

So the del might not be playing nice with the fixtures, but it's essential to the test. These tests are trying to see what happens when a Universe goes out of scope (is deleted and garbage collected).

@orbeckst

You might be able to leave out the fixtures and then not need to explicitly del them – if pytest only creates the fixture for each method.

@richardjgowers

This comment has been minimized.

Member

richardjgowers commented Jul 12, 2017

@utkbansal fot the tests which rely on del on a Universe, try rewriting these to not use a fixture, but instead just put the setup inside the test method and see if it starts working again.

@richardjgowers richardjgowers self-assigned this Jul 12, 2017

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jul 12, 2017

@richardjgowers The test does work that way. But it's a hack.

@richardjgowers

This comment has been minimized.

Member

richardjgowers commented Jul 12, 2017

@utkbansal the test relies on the Universe becoming garbage collected, so I think having the fixture hold a reference to the Universe is breaking the test. I can't really see a way round a fixture providing an object but not retaining a reference to it.

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jul 13, 2017

@richardjgowers Anything left here?

@richardjgowers richardjgowers merged commit d4b27a6 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