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

Port core module to pytest #1443

Merged
merged 26 commits into from
Jul 2, 2017
Merged

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 Port core module to pytest [WIP]Port core module to pytest Jun 28, 2017
@utkbansal
Copy link
Member Author

I hope this fixes it.

@utkbansal
Copy link
Member Author

I'm pretty sure there is something odd going on with yield based tests. I'm trying to isolate the issue and hopefully make a smaller example from it.

@utkbansal
Copy link
Member Author

@richardjgowers @kain88-de I think we have an issue here.

yield-based methods don’t support setup properly because the setup method is always called in the same class instance. There are no plans to fix this currently because yield-tests are deprecated in pytest 3.0, with pytest.mark.parametrize being the recommended alternative.

Source: https://docs.pytest.org/en/latest/nose.html#unsupported-idioms-known-issues

@utkbansal
Copy link
Member Author

utkbansal commented Jun 29, 2017

Oh crap, pytest shows me that the tests are collected, but they aren't being executed, they just pass.

Try running this pytest core/test_atomselections.py::TestImplicitOr --collect-only
Output is

<Module 'MDAnalysisTests/core/test_atomselections.py'>
  <UnitTestCase 'TestImplicitOr'>
    <TestCaseFunction 'test_range_selections'>
    <TestCaseFunction 'test_string_selections'>

But neither of the two methods are executed! They just show up as pass. I can confirm this because I've tried adding print statements/using the debugger, but its not executed.

The only solution is to get rid of yeild based tests and use parameterize.

@utkbansal
Copy link
Member Author

@richardjgowers @kain88-de

What's wrong with the following conversion to parameterize?

    def test_string_selections(self):
        for ref, sel in (
                ('name NameABA or name NameACA or name NameADA',
                 'name NameABA NameACA NameADA'),
                ('type TypeE or type TypeD or type TypeB',
                 'type TypeE TypeD TypeB'),
                ('resname RsC or resname RsY', 'resname RsC RsY'),
                ('name NameAB* or name NameACC', 'name NameAB* NameACC'),
                ('(name NameABC or name NameABB) and (resname RsD or resname RsF)',
                 'name NameABC NameABB and resname RsD RsF'),
                ('segid SegA or segid SegC', 'segid SegA SegC'),
        ):
            yield self._check_sels, ref, sel

    @pytest.mark.parametrize('ref, sel', (
        ('name NameABA or name NameACA or name NameADA',
         'name NameABA NameACA NameADA'),
        ('type TypeE or type TypeD or type TypeB',
         'type TypeE TypeD TypeB'),
        ('resname RsC or resname RsY', 'resname RsC RsY'),
        ('name NameAB* or name NameACC', 'name NameAB* NameACC'),
        ('(name NameABC or name NameABB) and (resname RsD or resname RsF)',
         'name NameABC NameABB and resname RsD RsF'),
        ('segid SegA or segid SegC', 'segid SegA SegC'),
    ))
    def test_string_selections_py(self, ref, sel):
        self._check_sels(ref, sel)

I am getting an error TypeError: test_string_selections_py() takes exactly 3 arguments (1 given)

@utkbansal
Copy link
Member Author

utkbansal commented Jun 29, 2017

Oh man, more mess. 😭

@pytest.mark.parametrize doesn't work if I inherit from TestCase

Edit
Source: https://stackoverflow.com/questions/18182251/does-pytest-parametrized-test-work-with-unittest-class-based-tests

@richardjgowers
Copy link
Member

@utkbansal we're moving away from TestCase and yield anyway, so it sounds like you'll have to port this 100% to pytest rather than the 50% you've been able to do with other tests. In the example you've copied, it should be pretty easy to make u from setUp a fixture, then just pass that to the thing you've written.

@utkbansal
Copy link
Member Author

@richardjgowers Yeah, I'm worried that this will take some additional time and will mess up with my timeline. Though I have to do this work anyway, but the time taken on Phase 1 gets extended.

@kain88-de
Copy link
Member

@utkbansal you might want to work on analysis then before you complete this. But yeah you have to do the work anyway. And since this is one of the last tests still on nose it isn't that bad.

@kain88-de
Copy link
Member

Also a quick hack would be

def test_string_selections_py(self):
     stuff = (('name NameABA or name NameACA or name NameADA',  'name NameABA NameACA NameADA'),
     ('type TypeE or type TypeD or type TypeB', 'type TypeE TypeD TypeB'),
        ('resname RsC or resname RsY', 'resname RsC RsY'),
        ('name NameAB* or name NameACC', 'name NameAB* NameACC'),
        ('(name NameABC or name NameABB) and (resname RsD or resname RsF)',
         'name NameABC NameABB and resname RsD RsF'),
        ('segid SegA or segid SegC', 'segid SegA SegC'))
    for ref, sel in stuff:
        self._check_sels(ref, sel)

I don't recommend it though because this has to be converted to proper pytests anyway.

@utkbansal
Copy link
Member Author

Update: @dec.skipif doesn't play well if the fixture is marked as @staticmethod

@kain88-de
Copy link
Member

@utkbansal use pytest.mark.skipif. Also where did you come across the skipif?

@kain88-de
Copy link
Member

@utkbansal this should work. https://docs.pytest.org/en/latest/skipping.html#id1

('prop mass < 4.0 hello', universe), # unused token
('prop mass > 10. and group this', universe), # missing group
('prop mass > 10. and fullgroup this', universe), # missing fullgroup
], indirect=['universe'])
Copy link
Member

Choose a reason for hiding this comment

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

what does the indirect= do? Is that documented somewhere?

@utkbansal
Copy link
Member Author

utkbansal commented Jun 30, 2017 via email

]:
yield self.selection_fail, selstr

@pytest.mark.parametrize('selstr, universe', [
Copy link
Member

Choose a reason for hiding this comment

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

why is universe being parametrized here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the function doesn't get it directly from the fixture. Have a look at the link I posted above.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but why can't we just use the Universe fixture above? ie just

@parametrize('selstr', [etc etc])
def test(self, selstr, 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.

I tried that, it didn't work. I don't have a reason to why it didn't work. I'll try to look it up.

Copy link
Member

Choose a reason for hiding this comment

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

The suggestion of @richardjgowers work for me locally


def selection_fail(self, selstr):
assert_raises(SelectionError, self.u.select_atoms,
def selection_fail(self, selstr, universe):
Copy link
Member

Choose a reason for hiding this comment

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

now we're not using yield (and having to provide a function to call for each test), we can collapse these functions into the test function itself

sel.format(typ=seltype))

class TestICodeSelection(object):
@pytest.mark.parametrize('ref, sel, universe',[
Copy link
Member

Choose a reason for hiding this comment

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

same as above, why is Universe in the parametrize?

@utkbansal
Copy link
Member Author

The drop in coverage should be fixed now, hopefully.

@utkbansal
Copy link
Member Author

@richardjgowers @kain88-de I've got the coverage back to normal. Could you please review. Then I can do all the cleanup in one go.

@@ -497,125 +509,134 @@ def choosemeth(sel, meth, periodic):

return sel

def _check_around(self, meth, periodic):
sel = Parser.parse('around 5.0 resid 1', self.u.atoms)
@pytest.mark.parametrize('u, meth, periodic', [
Copy link
Member

Choose a reason for hiding this comment

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

If you change the u fixture above to a staticmethod, you don't have to parametrize around it here.

ie

@staticmethod
@pytest.fixture
def u():

@pytest.mark.parametrize('meth, periodic', [])
def test_around(self, u, meth, periodic):

will work

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes, when I was trying I had a dec.skipif and that doesn't work well with static methods. That's why I got different results.

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.

needs the indirects removing first

('improper', u)
], indirect=['u'])
def test_VEs(self, btype, u):
self._check_VE(btype, u)
Copy link
Member

Choose a reason for hiding this comment

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

can you directly include the _check_VE function here.

# 'prop mass > 10. and group this', # missing group
# 'prop mass > 10. and fullgroup this', # missing fullgroup
# ]:
# yield self.selection_fail, selstr
Copy link
Member

Choose a reason for hiding this comment

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

this should be removed before the final merge

]:
yield self.selection_fail, selstr

@pytest.mark.parametrize('selstr, universe', [
Copy link
Member

Choose a reason for hiding this comment

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

The suggestion of @richardjgowers work for me locally

* Get rid of the indirect argument
* Use @pytest.mark.skipif in place of @dec.skipif

def tearDown(self):
del self.u
@pytest.mark.skipif(parser_not_found('DCD'),'DCD parser not available. Are you using python 3?')
Copy link
Member

Choose a reason for hiding this comment

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

Can you double check that marking fixtures like this works? Ie does this mark then get transferred onto tests which use this fixture?

Copy link
Member Author

Choose a reason for hiding this comment

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

On my local system, I only have the minimal dependencies, all the test methods are skipped.

Copy link
Member

Choose a reason for hiding this comment

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

You run on python3? These tests will always run on Python 2

Copy link
Member Author

Choose a reason for hiding this comment

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

That's odd. I'm python 2.

Copy link
Member

Choose a reason for hiding this comment

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

dcd is available on python 2 as a parser. Those tests should run for you. But since we soon have a python3 DCD parser I'm not to worried if the tests are skipped but they should be run on phython2.

Copy link
Member

Choose a reason for hiding this comment

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

This should work on Python 2

Copy link
Member

Choose a reason for hiding this comment

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

@utkbansal do these tests work under python 2 now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, they work now.

@staticmethod
@pytest.fixture()
def u(self):
# TODO: This is dummy. Needs review!
Copy link
Member

Choose a reason for hiding this comment

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

Is this still Todo, or is it Todone?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we don't need to pass universe to the parametrize call, we can get rid of this completely.

@utkbansal
Copy link
Member Author

@richardjgowers @kain88-de What else needs to be done?


def tearDown(self):
del self.u
@pytest.mark.skipif(parser_not_found('DCD'),'DCD parser not available. Are you using python 3?')
Copy link
Member

Choose a reason for hiding this comment

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

@utkbansal do these tests work under python 2 now?

('{typ} 1-5 or {typ} 7:10 or {typ} 12', '{typ} 1-5 7:10 12'),
('{typ} 1 or {typ} 3 or {typ} 5:10', '{typ} 1 3 5:10'),
])
def test_range_selections(self, seltype, ref, sel, universe):
Copy link
Member

Choose a reason for hiding this comment

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

how many tests are generated for this? Does it equal the number of tests the old double loop used in nose?

Copy link
Member Author

Choose a reason for hiding this comment

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

18 test cases are generated as expected.

@kain88-de
Copy link
Member

@richardjgowers you should have a final look for the core module.

@richardjgowers
Copy link
Member

Yep looks good

@utkbansal utkbansal changed the title [WIP]Port core module to pytest Port core module to pytest Jul 2, 2017
@kain88-de kain88-de merged commit b97f2ab into MDAnalysis:develop Jul 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants