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

[Review Needed]Pytest Style coordinates/test_timestep_api.py #1650

Merged
merged 2 commits into from Aug 28, 2017

Conversation

Projects
None yet
3 participants
@utkbansal
Copy link
Member

utkbansal commented Aug 25, 2017

Fixes #

Changes made in this Pull Request:

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?
@orbeckst
Copy link
Member

orbeckst left a comment

Please check the code related to importorskip(netCDF4). It is likely not needed anymore and can be simplified.


def setUp(self):
@pytest.fixture()
def universe(self):
pytest.importorskip('netCDF4')

This comment has been minimized.

@orbeckst

orbeckst Aug 26, 2017

Member

I don't think that we need to check for netCDF anymore. We should always be able to read ncdf files because we use scipy.io.netcdf for reading.

This comment has been minimized.

@utkbansal

utkbansal Aug 26, 2017

Member

Does this mean that pytest.importorskip('netCDF4') shouldn't be present anywhere in the tests as we don't use it anymore?

This comment has been minimized.

@orbeckst

orbeckst Aug 28, 2017

Member

With git grep I only found it in MDAnalysisTests/coordinates/test_xdr.py and there it is also not needed anymore. We should always be able to read NCDF files.

This comment has been minimized.

@orbeckst

orbeckst Aug 28, 2017

Member

(I must have overlooked these instances when I worked on #506 so thanks for fixing them as you go along.)

This comment has been minimized.

@orbeckst

orbeckst Aug 28, 2017

Member

The other PR where this should be fixed is #1655.


class TestNCDFBaseTimestepInterface(object):

This comment has been minimized.

@orbeckst

orbeckst Aug 26, 2017

Member

Does this separate class solely exist so that you can test for netCDF4? If so, this is not necessary anymore.

Could you then get rid of this class and do everything in the pararmeterized class? (You are already loading ncdf files in the parametrization anyway...).

@@ -61,7 +60,29 @@ def test_other_timestep(self, otherTS):
# TODO: Merge this into generic Reader tests
# These tests are all included in BaseReaderTest
# Once Readers use that TestClass, delete this one
class BaseTimestepInterfaceTest(TestCase):

@pytest.mark.parametrize("topology, trajectory, trajectory_format, topology_format", (

This comment has been minimized.

@orbeckst

orbeckst Aug 26, 2017

Member

This looks good.

@utkbansal

This comment has been minimized.

Copy link
Member

utkbansal commented Aug 26, 2017

@orbeckst Should be good to go now.

@orbeckst orbeckst merged commit e9f0cb9 into MDAnalysis:develop Aug 28, 2017

2 checks passed

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