Skip to content

[Review Needed]Pytest Style core/test_atomselections.py#1634

Merged
richardjgowers merged 2 commits intoMDAnalysis:developfrom
utkbansal:core-atom-sel
Aug 25, 2017
Merged

[Review Needed]Pytest Style core/test_atomselections.py#1634
richardjgowers merged 2 commits intoMDAnalysis:developfrom
utkbansal:core-atom-sel

Conversation

@utkbansal
Copy link
Member

Fixes #

Changes made in this Pull Request:

PR Checklist

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

@richardjgowers richardjgowers self-assigned this Aug 21, 2017
class TestSelectionsCHARMM(TestCase):
def setUp(self):
class TestSelectionsCHARMM(object):
@pytest.fixture()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we increase the scope of this? You'll need to check if any test methods modify the universe in their test. If so, they'l need a function scoped version of the same Universe

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richardjgowers Not sure what you meant by they'l need a function scoped version of the same Universe. Have pushed a commit, please have a look if this is what you meant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a test method modifies a class scoped fixture, the fixture will remain modified across all other test methods. So if we want to increase the scope of this fixture, we need to check to see if any methods will modify it. If any test methods do need to modify the fixture, they should use a different version of the fixture which is scoped to only the function.

@richardjgowers richardjgowers merged commit b997234 into MDAnalysis:develop Aug 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants