Port analysis module to pytest #1450
Conversation
There is some unusual stuff going on in the |
@utkbansal not sure, but it might be because of |
@utkbansal looks like you have to replace |
@richardjgowers @kain88-de What should I do with this setup? class TestEncoreImportWarnings(object):
def setUp(self):
# clear cache of encore module
for mod in list(sys.modules): # list as we're changing as we iterate
if 'encore' in mod:
sys.modules.pop(mod, None)
@block_import('sklearn')
def _check_sklearn_import_warns(self, package):
warnings.simplefilter('always')
assert_warns(ImportWarning, importlib.import_module, package)
def test_import_warnings(self):
for pkg in (
'MDAnalysis.analysis.encore.dimensionality_reduction.DimensionalityReductionMethod',
'MDAnalysis.analysis.encore.clustering.ClusteringMethod',
):
yield self._check_sklearn_import_warns, pkg This isn't something that a fixture is used for. This literally is some setup stuff. One option is to name this method |
I did not test, but you should be able to do something like: @pytest.fixture()
def remove_encore_module():
# clear cache of encore module
for mod in list(sys.modules): # list as we're changing as we iterate
if 'encore' in mod:
sys.modules.pop(mod, None)
@pytest.mark.usefixtures('remove_encore_module')
class TestEncoreImportWarnings(TestCase):
... From what I understand, the Once again, I did not test so I might be wrong. |
|
for pkg in ( | ||
'MDAnalysis.analysis.encore.dimensionality_reduction.DimensionalityReductionMethod', | ||
'MDAnalysis.analysis.encore.clustering.ClusteringMethod', | ||
): | ||
yield self._check_sklearn_import_warns, pkg | ||
self._check_sklearn_import_warns(pkg, recwarn) |
jbarnoud
Jul 8, 2017
Contributor
This changes the behaviour of the test. Instead of having 2 tests that can fail individually, there is only one test that has two consecutive asserts. It is fine for now, but please add a comment as it may be forgotten when you will move to proper pytest idioms.
This changes the behaviour of the test. Instead of having 2 tests that can fail individually, there is only one test that has two consecutive asserts. It is fine for now, but please add a comment as it may be forgotten when you will move to proper pytest idioms.
jbarnoud
Jul 13, 2017
Contributor
The comment is still missing here.
The comment is still missing here.
jbarnoud
Jul 13, 2017
Contributor
The comment is bellow. I missed it.
The comment is bellow. I missed it.
It is confusing indeed, but what do you find confusing there? |
Please prioritize this PR so we can get rid of nose. |
Smaller things in the comments. For the problem with
Puzzling, because I don't think you touched For the cryptic failures such as https://travis-ci.org/MDAnalysis/mdanalysis/jobs/251523905#L1302 add more details to the test output try:
...
except Exception as err:
raise AssertionError("... blabla failed. Exception: {}".format(err)) or similar. Then we might get a better idea where the problem lies. The hbond tests are admittedly not very good and just test that the code runs somehow but at least that should work. It would be better if they checked more specific things but that's outside the scope of your PR. |
- PYTEST_FLAGS="--disable-pytest-warnings -n 2" | ||
- PYTEST_LIST="testsuite/MDAnalysisTests/lib testsuite/MDAnalysisTests/formats testsuite/MDAnalysisTests/coordinates testsuite/MDAnalysisTests/utils testsuite/MDAnalysisTests/topology testsuite/MDAnalysisTests/auxiliary testsuite/MDAnalysisTests/core" | ||
- PYTEST_LIST="testsuite/MDAnalysisTests/lib testsuite/MDAnalysisTests/formats testsuite/MDAnalysisTests/coordinates testsuite/MDAnalysisTests/utils testsuite/MDAnalysisTests/topology testsuite/MDAnalysisTests/auxiliary testsuite/MDAnalysisTests/core testsuite/MDAnalysisTests/analysis" |
orbeckst
Jul 11, 2017
Member
also remove nose
from package/setup.py
and testsuite/setup.py
— yay, major step forward in your project: getting rid of nose
.
also remove nose
from package/setup.py
and testsuite/setup.py
— yay, major step forward in your project: getting rid of nose
.
orbeckst
Jul 11, 2017
Member
And remove testsuite/mda_nosetests.py
.
And remove testsuite/mda_nosetests.py
.
orbeckst
Jul 11, 2017
Member
@mnmelo @jbarnoud @kain88-de @richardjgowers : should the nose-specific plugins also be removed at this time? We have to recreate the plugins for pytest or replace with native pytest plugins anyway,
@mnmelo @jbarnoud @kain88-de @richardjgowers : should the nose-specific plugins also be removed at this time? We have to recreate the plugins for pytest or replace with native pytest plugins anyway,
kain88-de
Jul 11, 2017
Member
@orbeckst these changes should be in a separate PR. The plugins can pretty much all go. There is the openfile plugin but otherwise pytest comes with better alternatives
@orbeckst these changes should be in a separate PR. The plugins can pretty much all go. There is the openfile plugin but otherwise pytest comes with better alternatives
- PYTEST_FLAGS="--disable-pytest-warnings -n 2" | ||
- PYTEST_LIST="testsuite/MDAnalysisTests/lib testsuite/MDAnalysisTests/formats testsuite/MDAnalysisTests/coordinates testsuite/MDAnalysisTests/utils testsuite/MDAnalysisTests/topology testsuite/MDAnalysisTests/auxiliary testsuite/MDAnalysisTests/core" | ||
- PYTEST_LIST="testsuite/MDAnalysisTests/lib testsuite/MDAnalysisTests/formats testsuite/MDAnalysisTests/coordinates testsuite/MDAnalysisTests/utils testsuite/MDAnalysisTests/topology testsuite/MDAnalysisTests/auxiliary testsuite/MDAnalysisTests/core testsuite/MDAnalysisTests/analysis" | ||
- NOSE_COVERAGE_FILE="nose_coverage" | ||
- PYTEST_COVERAGE_FILE="pytest_coverage" | ||
- MAIN_CMD="pytest ${PYTEST_LIST} ${PYTEST_FLAGS}; python ./testsuite/MDAnalysisTests/mda_nosetests ${NOSE_TEST_LIST} ${NOSE_FLAGS}" |
orbeckst
Jul 11, 2017
Member
remove use of mda_nosetests
remove use of mda_nosetests
jbarnoud
Jul 13, 2017
Contributor
I agree with @orbeckst. With NOSE_TEST_LIST
being empty, the call to mda_nosetests
becomes buggy and should be removed. Removing the dependencies to nose is out of scope, though.
I agree with @orbeckst. With NOSE_TEST_LIST
being empty, the call to mda_nosetests
becomes buggy and should be removed. Removing the dependencies to nose is out of scope, though.
class TestRotationMatrix(object): | ||
def __init__(self): | ||
class TestRotationMatrix(TestCase): | ||
def setUp(self): |
orbeckst
Jul 11, 2017
Member
@utkbansal I assume the goal of this PR is to make it run under pytest and then you will do a second round adapting it to pytest style? Is this correct? (Would be fine with me.)
@utkbansal I assume the goal of this PR is to make it run under pytest and then you will do a second round adapting it to pytest style? Is this correct? (Would be fine with me.)
kain88-de
Jul 11, 2017
Member
yes that idea is correct
yes that idea is correct
@@ -45,10 +48,10 @@ | |||
warnings.simplefilter('always') | |||
|
|||
|
|||
class Testrmsd(object): | |||
class Testrmsd(TestCase): | |||
@dec.skipif(parser_not_found('DCD'), |
orbeckst
Jul 11, 2017
Member
remove DCD skipif, we have full DCD support now
remove DCD skipif, we have full DCD support now
kain88-de
Jul 11, 2017
Member
better in a second PR
better in a second PR
orbeckst
Jul 11, 2017
Member
ok
ok
reference = MDAnalysis.Universe(PSF, DCD) | ||
reference.atoms = reference.atoms[:-1] | ||
RMSD = MDAnalysis.analysis.rms.RMSD(self.universe, | ||
reference=reference) | ||
with pytest.raises(SelectionError): |
orbeckst
Jul 11, 2017
Member
good!
good!
@@ -239,7 +241,7 @@ def run_HBA_dynamic_selections(*args): | |||
self._tearDown() | |||
|
|||
|
|||
class TestHydrogenBondAnalysisTIP3P(object): | |||
class TestHydrogenBondAnalysisTIP3P(TestCase): | |||
@dec.skipif(parser_not_found('DCD'), |
orbeckst
Jul 11, 2017
Member
remove DCD skipif
remove DCD skipif
@utkbansal You should prioritize this PR. The other PR's are nice but not as important as this one. You still haven't told us your exact problem. |
@@ -207,9 +210,11 @@ def runOK(): | |||
raise AssertionError("HydrogenBondAnalysis protein/protein failed") | |||
else: | |||
return True | |||
yield runOK | |||
runOK() |
jbarnoud
Jul 13, 2017
Contributor
This change the behaviour and transforms a test generator into a succession of assertions in a single test. Please, add a comment so you can restore the initial behaviour latter.
This change the behaviour and transforms a test generator into a succession of assertions in a single test. Please, add a comment so you can restore the initial behaviour latter.
utkbansal
Jul 13, 2017
Author
Member
I am still working on it. This isn't final.
I am still working on it. This isn't final.
yield assert_raises, SelectionError, run_HBA, s1, s2, s1type | ||
# yield assert_raises, SelectionError, run_HBA, s1, s2, s1type | ||
with pytest.raises(SelectionError): | ||
run_HBA(s1, s2, s1type) |
jbarnoud
Jul 13, 2017
Contributor
Same here. Add a comment.
Same here. Add a comment.
@@ -234,12 +239,12 @@ def run_HBA_dynamic_selections(*args): | |||
raise AssertionError("HydrogenBondAnalysis with update=True failed") | |||
else: | |||
return True | |||
yield run_HBA_dynamic_selections, s1, s2, s1type | |||
run_HBA_dynamic_selections(s1, s2, s1type) |
jbarnoud
Jul 13, 2017
Contributor
Idem. Comment.
Idem. Comment.
@kain88-de @richardjgowers @jbarnoud This is the best hack I could come up with. Review please. |
for s1, s2, s1type in itertools.product((protein, nothing), | ||
(protein, nothing), | ||
("donor", "acceptor", "both")): | ||
params.append((s1, s2, s1type)) |
mnmelo
Jul 13, 2017
Member
Must this become a list? Won't pytest.mark.parametrize
accept the itertools
generator object?
Must this become a list? Won't pytest.mark.parametrize
accept the itertools
generator object?
kain88-de
Jul 13, 2017
Member
even if this has to be a list. Why not list(itertools.product(...))
even if this has to be a list. Why not list(itertools.product(...))
kain88-de
Jul 13, 2017
Member
The iterator still works just fine.
import pytest
import itertools
@pytest.mark.parametrize('a, b', itertools.product(range(3), range(3)))
def test_d(a, b):
assert True
The iterator still works just fine.
import pytest
import itertools
@pytest.mark.parametrize('a, b', itertools.product(range(3), range(3)))
def test_d(a, b):
assert True
return params | ||
|
||
|
||
def build_parameters2(): |
mnmelo
Jul 13, 2017
Member
I'm not getting the rationale behind the two build_parameters
. Is it to have the option of eventually supplying different lists of test args to the two tests?
I'm not getting the rationale behind the two build_parameters
. Is it to have the option of eventually supplying different lists of test args to the two tests?
@@ -162,6 +162,28 @@ def setUp(self): | |||
self.values['acceptor_resnm'] = np.array([], dtype="<U3") | |||
|
|||
|
|||
def build_parameters(): |
mnmelo
Jul 13, 2017
Member
Could you instead define protein
, nothing
and the itertools.product
generator as class attributes of TestHydrogenBondAnalysisChecking
? This way the the itertools.product
will be available when the decorator is used, and there won't be any need for the build_parameters
functions.
Could you instead define protein
, nothing
and the itertools.product
generator as class attributes of TestHydrogenBondAnalysisChecking
? This way the the itertools.product
will be available when the decorator is used, and there won't be any need for the build_parameters
functions.
utkbansal
Jul 13, 2017
Author
Member
Can't access self
in @pytest.mark.parametrize
Can't access self
in @pytest.mark.parametrize
utkbansal
Jul 13, 2017
Author
Member
I've already removed build_parameters()
I've already removed build_parameters()
mnmelo
Jul 13, 2017
Member
Class attrs don't need to be accessed via self
when in the class body. They're already in the namespace. Anyway, I think the solution of having the iterator call within the decorator is the best, as it clearer what's happening and allows customization in place.
Class attrs don't need to be accessed via self
when in the class body. They're already in the namespace. Anyway, I think the solution of having the iterator call within the decorator is the best, as it clearer what's happening and allows customization in place.
utkbansal
Jul 13, 2017
Author
Member
So is the latest change ok?
I've done this
@pytest.mark.parametrize('s1, s2, s1type', itertools.product(
("protein", "resname ALA and not backbone"),
("protein", "resname ALA and not backbone"),
("donor", "acceptor", "both"),
))
So is the latest change ok?
I've done this
@pytest.mark.parametrize('s1, s2, s1type', itertools.product(
("protein", "resname ALA and not backbone"),
("protein", "resname ALA and not backbone"),
("donor", "acceptor", "both"),
))
|
@kain88-de @richardjgowers @jbarnoud The build technically passes because there are no errors in the pytest part. |
@richardjgowers So would be able to merge this as is(once everyone confirms the changes)? I'm already working on #1480 to remove nose specific stuff. |
Still a few comments, mostly about hbonds. Also, I would like you to remove the calls to Finally, you could squash your commits from today (starting from "Quick fix"). Keep the commit that remove the calls to |
for pkg in ( | ||
'MDAnalysis.analysis.encore.dimensionality_reduction.DimensionalityReductionMethod', | ||
'MDAnalysis.analysis.encore.clustering.ClusteringMethod', | ||
): | ||
yield self._check_sklearn_import_warns, pkg | ||
self._check_sklearn_import_warns(pkg, recwarn) |
jbarnoud
Jul 13, 2017
Contributor
The comment is still missing here.
The comment is still missing here.
protein = "protein" | ||
|
||
if s1 == s2 == protein: | ||
def runOK(): |
jbarnoud
Jul 13, 2017
Contributor
The way the test is written now, what is the point of having the nested function?
The way the test is written now, what is the point of having the nested function?
utkbansal
Jul 13, 2017
Author
Member
Nothing really. I can pull that out.
Nothing really. I can pull that out.
|
||
self._tearDown() | ||
|
||
def run_HBA2(self, s1, s2, s1type): |
jbarnoud
Jul 13, 2017
Contributor
I do not like having functions named run_HBA
and run_HBA2
. It was OK that they had the same name when they were nested because the parent function was giving the context. Now the names do not tell how the two functions differ from each other. run_HBA_no_update
and run_HBA_update
would be better names, other names may work as well.
I do not like having functions named run_HBA
and run_HBA2
. It was OK that they had the same name when they were nested because the parent function was giving the context. Now the names do not tell how the two functions differ from each other. run_HBA_no_update
and run_HBA_update
would be better names, other names may work as well.
- PYTEST_FLAGS="--disable-pytest-warnings -n 2" | ||
- PYTEST_LIST="testsuite/MDAnalysisTests/lib testsuite/MDAnalysisTests/formats testsuite/MDAnalysisTests/coordinates testsuite/MDAnalysisTests/utils testsuite/MDAnalysisTests/topology testsuite/MDAnalysisTests/auxiliary testsuite/MDAnalysisTests/core" | ||
- PYTEST_LIST="testsuite/MDAnalysisTests/lib testsuite/MDAnalysisTests/formats testsuite/MDAnalysisTests/coordinates testsuite/MDAnalysisTests/utils testsuite/MDAnalysisTests/topology testsuite/MDAnalysisTests/auxiliary testsuite/MDAnalysisTests/core testsuite/MDAnalysisTests/analysis" | ||
- NOSE_COVERAGE_FILE="nose_coverage" | ||
- PYTEST_COVERAGE_FILE="pytest_coverage" | ||
- MAIN_CMD="pytest ${PYTEST_LIST} ${PYTEST_FLAGS}; python ./testsuite/MDAnalysisTests/mda_nosetests ${NOSE_TEST_LIST} ${NOSE_FLAGS}" |
jbarnoud
Jul 13, 2017
Contributor
I agree with @orbeckst. With NOSE_TEST_LIST
being empty, the call to mda_nosetests
becomes buggy and should be removed. Removing the dependencies to nose is out of scope, though.
I agree with @orbeckst. With NOSE_TEST_LIST
being empty, the call to mda_nosetests
becomes buggy and should be removed. Removing the dependencies to nose is out of scope, though.
@jbarnoud If I understand correctly, I have to squash all my commits now and then have one more commit that removes |
All your commits until the one titled "Quick fix" are logical units. I would squash them but I do not mind having them separate. From "Quick fix" and latter, the commits are iterative fixes that should be squashed. The removal of |
Hack in parameterize Cleanup Use itertools Cleanup
It's hard to believe, but the full build completed in 16.5 mins. |
@jbarnoud All good? |
The build passed! Plus a minor increase in coverage. |
@orbeckst do you have any more comments? I would like to merge this. |
Fixes #
Changes made in this Pull Request:
PR Checklist