Skip to content
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_tprparser.py #1606

Merged
merged 6 commits into from Sep 6, 2017

Conversation

utkbansal
Copy link
Member

Fixes #

Changes made in this Pull Request:

PR Checklist

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

@utkbansal utkbansal changed the title Pytest Style topology/test_tprparser.py [Review Needed]Pytest Style topology/test_tprparser.py Aug 8, 2017
Copy link
Contributor

@jbarnoud jbarnoud left a comment

Choose a reason for hiding this comment

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

Would it be possible to parametrize the classes? It was not possible to generate the test classes with nose, but hopefully it is possible with pytest. It would drastically reduce the code repetition.

('RESTRANGLES',[(6, 7, 8)]),
('TABANGLES',[(7, 8, 9)]),
))
def test_all_angles(topology, angle):
angle_type_in_topology = functools.partial(_test_is_in_topology,
Copy link
Contributor

Choose a reason for hiding this comment

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

The partial functions were there to make the verbose output more readable, they are not useful anymore.

@jbarnoud
Copy link
Contributor

I do not see the point of having the file name a fixture? Is this something you need to allow parametrized generation f test class?

@richardjgowers
Copy link
Member

Yeah having filename as a fixture is ugly. Can we use the request fixture from pytest instead? Something like request.class.filename?

@utkbansal
Copy link
Member Author

@jbarnoud @richardjgowers Yes, to parametrize test classes, I need the fixture in the base class.

@jbarnoud
Copy link
Contributor

@utkbansal Sorry I do not have the time right now to look at the details of how to parametrize a class. But if you really have to use the fixture, could the fixture be defined only once in the base class and return the class attribute? It makes inheritance much easier: you just have to create the class with the right class attribute and not to overwrite a method.

@richardjgowers
Copy link
Member

@utkbansal Can we use the request fixture from pytest instead? Something like request.cls.filename?

@utkbansal
Copy link
Member Author

Yes, I'm trying to do that. Will push a solution soon.

@richardjgowers richardjgowers self-assigned this Aug 21, 2017
Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

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

Can use the request pytest fixture instead of parametrising filename

@utkbansal
Copy link
Member Author

utkbansal commented Aug 21, 2017

@richardjgowers If you were thinking of something like

import pytest


class A(object):
    def test_value(self, request):
        request.cls.filename == "foo"

@pytest.mark.parametrize('filename', (
    'foo',
    'bar'
))
class TestA(A):
    pass

It doesn't work.

@richardjgowers
Copy link
Member

@utkbansal something like this should work:

# in base.py
@pytest.fixture
def top(self, request):
    with self.parser(request.cls.filename) as p:
        yield p.parse()

# in test_crd
class TestCRDParser(ParserBase):
    filename = CRD

So the request fixture is inspecting the class it is called from within, and finding the filename attribute we have set there.

@orbeckst orbeckst changed the title [Review Needed]Pytest Style topology/test_tprparser.py Pytest Style topology/test_tprparser.py Sep 5, 2017
TPR407, TPR450, TPR451, TPR452, TPR453, TPR454,
TPR455, TPR502, TPR504, TPR505, TPR510, TPR2016])
def filename(self, request):
return request.param
Copy link
Member

Choose a reason for hiding this comment

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

@jbarnoud This what you had in mind to remove the classes?

Copy link
Member

Choose a reason for hiding this comment

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

@richardjgowers you are also OK with this?

Copy link
Member

Choose a reason for hiding this comment

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

I'll have a play later today

This makes writing ParserBase derived classes easier
@@ -36,14 +36,18 @@ class ParserBase(object):
expected_attrs = []
guessed_attrs = []

@pytest.fixture
def filename(self):
Copy link
Member

Choose a reason for hiding this comment

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

I still think it's ugly that we have to redefine this fixture in child classes to have more than one filename.

Copy link
Member

Choose a reason for hiding this comment

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

I played around a lot. The problem is I can't define a fixture using the params kwargs in the baseclass with a variable name ref_filename and then fill that variable name in the childclasses with meaningful values.

Copy link
Member

Choose a reason for hiding this comment

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

Ok thanks then, maybe this is the cleanest solution

filename = TPR455Double
@pytest.fixture()
def filename(self):
return TPR455Double


class TPR46xBase(TPRAttrs):
Copy link
Member

Choose a reason for hiding this comment

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

Could we instead parametrize the ref_filename inside the class?

@pytest.mark.parametrize('ref_filename', [TPR460, TPR461])
class TPR46xBase()

Copy link
Member

Choose a reason for hiding this comment

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

No. Pytest then expects every method in the class to request a parameter called ref_filename.

Copy link
Member

Choose a reason for hiding this comment

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

import pytest

@pytest.mark.parametrize('a', range(10), indirect=True)
class Test_A(object):
    def test_foo(self, a):
        assert True

    def test_bar(self):
        assert True

This fill fail collection since test_bar doesn't require a fixture called a

Copy link
Contributor

@jbarnoud jbarnoud left a comment

Choose a reason for hiding this comment

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

So, if I well understand, we now overwrite the ref_filename class attribute and everything works as before. But if we want to run the tests on a bunch of different files, we overwrite the filename fixture. Looks fine to me. In the tests for the TPR format, it really reduces the code duplication.

@kain88-de
Copy link
Member

@jbarnoud or @richardjgowers do you see why we have a drop of code coverage for the TPR tests?

So, if I well understand, we now overwrite the ref_filename class attribute and everything works as before. But if we want to run the tests on a bunch of different files, we overwrite the filename fixture.

Yes that's it.

@kain88-de kain88-de merged commit bc36840 into MDAnalysis:develop Sep 6, 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.

None yet

4 participants