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: topology/test_crd.py #1527

Merged
merged 5 commits into from Aug 4, 2017

Conversation

Projects
None yet
4 participants
@utkbansal
Member

utkbansal commented Jul 19, 2017

Changes made in this Pull Request:

Woah! Pytest is very powerful!

This is branched off another branch so the diff is a bit messy. Here is the actual change I want to show. I parametrized the whole class and it works! I can just delete the test_crd.py file!

@pytest.mark.parametrize('parser, filename, expected_attrs, guessed_attrs, expected_n_atoms, expected_n_residues, expected_n_segments',  [
    (mda.topology.CRDParser.CRDParser,
        CRD,
        ['ids', 'names', 'tempfactors', 'resids', 'resnames', 'resnums', 'segids'],
        ['masses', 'types'],
        3341,
        214,
        1),
])
class TestParserBase(object):
    """Base class for testing Topology parsers.

    All Parsers must subclass this class!
    """

    # expected_attrs = []
    # guessed_attrs = []

    @pytest.fixture()
    def top(self, parser, filename):
        with parser(filename) as p:
            yield p.parse()

    def test_output(self, parser, filename, expected_attrs, guessed_attrs, expected_n_atoms, expected_n_residues, expected_n_segments, top):
        """Testing the call signature"""
        with parser(filename) as p:
            top = p.parse()

        assert isinstance(top, Topology)

    def test_mandatory_attributes(self, parser, filename, expected_attrs, guessed_attrs, expected_n_atoms, expected_n_residues, expected_n_segments, top):
        # attributes required as part of the API
        # ALL parsers must provide these
        mandatory_attrs = ['ids', 'masses', 'types',
                           'resids', 'resnums', 'segids']

        for attr in mandatory_attrs:
            assert hasattr(top, attr), 'Missing required attribute: {}'.format(attr)

    def test_expected_attributes(self, parser, filename, expected_attrs, guessed_attrs, expected_n_atoms, expected_n_residues, expected_n_segments, top):
        # Extra attributes as declared in specific implementations
        for attr in expected_attrs:
            assert hasattr(top, attr), 'Missing expected attribute: {}'.format(attr)

    def test_guessed_attributes(self, parser, filename, expected_attrs, guessed_attrs, expected_n_atoms, expected_n_residues, expected_n_segments, top):
        # guessed attributes must be declared as guessed
        for attr in top.attrs:
            val = attr.is_guessed
            if not val in (True, False):  # only for simple yes/no cases
                continue
            assert val == (attr.attrname in guessed_attrs), 'Attr "{}" guessed= {}'.format(attr, val)

    def test_size(self, parser, filename, expected_attrs, guessed_attrs, expected_n_atoms, expected_n_residues, expected_n_segments, top):
        """Check that the Topology is correctly sized"""
        assert top.n_atoms == expected_n_atoms, '{} atoms read, {} expected in {}'.format(
            top.n_atoms, expected_n_atoms, self.__class__.__name__)

        assert top.n_residues == expected_n_residues, '{} residues read, {} expected in {}'.format(
            top.n_residues, expected_n_residues, self.__class__.__name__)

        assert top.n_segments == expected_n_segments, '{} segment read, {} expected in {}'.format(
            top.n_segments, expected_n_segments, self.__class__.__name__)

    def test_tt_size(self, parser, filename, expected_attrs, guessed_attrs, expected_n_atoms, expected_n_residues, expected_n_segments, top):
        """Check that the transtable is appropriately sized"""
        assert top.tt.size == (expected_n_atoms, expected_n_residues, expected_n_segments)

    def test_creates_universe(self, parser, filename, expected_attrs, guessed_attrs, expected_n_atoms, expected_n_residues, expected_n_segments):
        """Check that Universe works with this Parser"""
        u = mda.Universe(filename)
        assert isinstance(u, mda.Universe)

@richardjgowers @kain88-de @jbarnoud @mnmelo The only downside of this is that I have to pass all the parameters to all the methods, but that seems like a tradeoff we can afford. Thoughts?

PR Checklist

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

This comment has been minimized.

Member

richardjgowers commented Jul 19, 2017

I like the approach, but the dealbreaker for me is it is difficult to include extra test methods which are specific to a given format.. This is a pretty typical pattern across parsers and coordinate readers currently:

class ParserBase(object):
    def test_standard_1():

    def test_standard_2():


class TestFormatX(ParserBase):
    def test_X_1():

class TestFormatY(ParserBase):
    def test_Y_1():

This pattern works well for us as you can have a single file which defines all the testing done for a single format, (eg test_gro)

WRT the multiple arguments that get given to each method, it would be cleaner and easier to define an object (like CRDReference) which defines all the reference data, so you could then do CRDReference.n_atoms.

@utkbansal

This comment has been minimized.

Member

utkbansal commented Jul 19, 2017

I like the approach, but the dealbreaker for me is it is difficult to include extra test methods which are specific to a given format.. This is a pretty typical pattern across parsers and coordinate readers currently. This pattern works well for us as you can have a single file which defines all the testing done for a single format, (eg test_gro)

@richardjgowers So then how do we want to proceed? We want to parametrize base classes and then have another class/file for the format specific tests. Or do we want to continue inheriting?

@kain88-de

This comment has been minimized.

Member

kain88-de commented Jul 26, 2017

@richardjgowers do you have more thought about parametrizing of the base classes?

@jbarnoud

This comment has been minimized.

Contributor

jbarnoud commented Jul 27, 2017

I recently encountered the use case @richardjgowers described. I would suggest we continue inheriting here.

@kain88-de

This comment has been minimized.

Member

kain88-de commented Jul 29, 2017

@utkbansal inheriting it is then

utkbansal added some commits Jul 19, 2017

@utkbansal

This comment has been minimized.

Member

utkbansal commented Aug 3, 2017

@jbarnoud jbarnoud merged commit ea2ca62 into MDAnalysis:develop Aug 4, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 90.449%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment