TST: Added scipy import warning unit test to test_distances.py.

[tylerjereddy]

A small PR with draft code to test for scipy related import exception handling in MDAnalysis/analysis/distances.py as per the uncovered lines.

It is particularly tricky to deal with this stuff because scipy is a sort of pseudo-dependency. I don't think we'll even get coverage credit because I think the full test build has scipy and python doesn't support module unloading / hiding, etc., unless you have a really creative idea.

So, actually I think this mostly increases coverage of a minimal build (where scipy is absent and the Exception will actually be triggered at the module level & then caught / a warning issued), which I suppose is also still important, but probably not accounted for in the coverage badge.

[orbeckst]

@richardjgowers did something creative with mock somewhere to test missing modules... can't just quite remember what it was, perhaps ncdf. That might be a way to test scipy missing even in full build.

[tylerjereddy]

Alright, I'll wait for him to comment. If I'm not mistaken, Python 3.3 onward has unittest.mock, but it is not in the standard library for Python 2, which is why I didn't go that route. But I agree that mocking is really powerful for tests if we can start using it at some point.

[orbeckst]

In testsuite/setup.py we already require mock>=2.0.0 so you can use it.

…

On 29 Dec, 2016, at 02:59, Tyler Reddy ***@***.***> wrote: Alright, I'll wait for him to comment. If I'm not mistaken, Python 3.3 onward has unittest.mock, but it is not in the standard library for Python 2, which is why I didn't go that route. But I agree that mocking is really powerful for tests if we can start using it at some point. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

-- Oliver Beckstein * [email protected] skype: orbeckst * [email protected]

[richardjgowers]

I do like checking that we don't warn when not necessary though!

[richardjgowers]

I think we can wrap this with the PR I put up, so it always issue a warning. Then split the else: branch of this if into a separate test that only runs when the scipy module is present.

[tylerjereddy]

The new decorator from @richardjgowers has been merged in. I'll now refactor my tests a bit accordingly. Hopefully by later today.

[richardjgowers]

The new failing test looks like we're not allowed relative imports in the testsuite, I don't remember why this is, but I think @mnmelo might. It might have been that they stop coverage reporting working?

[tylerjereddy]

I banned relative imports in the testsuite a while ago in #189. The reason for doing this was fairly idiosyncratic, but presumably I can fix the relate import(s) without too much hassle here?

[richardjgowers]

Yeah, you might have to change the imports to MDAnalysisTests.util.block_import as they're not defined in the init file.

…

On Fri, 30 Dec 2016, 5:03 p.m. Tyler Reddy, ***@***.***> wrote: I banned relative imports in the testsuite a while ago in #189 <#189>. The reason for doing this was fairly idiosyncratic, but presumably I can fix the relate import(s) without too much hassle here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1141 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI0jB5lYwk5Gj_5tmiwgcJjP455AoOWVks5rNTlKgaJpZM4LWos2> .

[tylerjereddy]

Even with the new decorator, it still doesn't work in my hands. I have to conda remove scipy on my machine to get the test to pass. Specifically, your decorator will only raise an ImportError if used in this manner:

@block_import('scipy')
def test_stuff(self):
  import scipy # this is properly blocked   

But it won't work if another module is importing scipy, which is what I want to test:

@block_import('scipy')
def test_stuff(self):
  import MDAnalysis.analysis.distances # this can still import scipy from my machine install   

[richardjgowers]

I'm fairly happy the decorator is working, I just think the scipy import is happening before the test - the first time the module is imported elsewhere in the testing process. I think all future imports of the module are using a cached? version of the module, so the scipy import try/except block is happening only once.

The below test will pass/fail depending on if analysis.distances is imported at the top of the module. I'm not sure if there's a way to either a) clear the cache of imported modules and make the entire module setup run again, OR b) define that the import test must run before all others. b) seems fragile because the tests should all be state independent really..

from numpy.testing import assert_warns
import warnings

from MDAnalysisTests.util import block_import

# Comment / uncomment this line
from MDAnalysis.analysis import distances as d

class TestImportWarn(object):
    @block_import('scipy')
    def test_warning(self):
        warnings.simplefilter('always')

        def do_this():
            import MDAnalysis.analysis.distances as d
            return d

        assert_warns(ImportWarning, do_this)

[tylerjereddy]

This solves the issue:

198     def setUp(self):                                                            
199         sys.modules.pop('MDAnalysis.analysis.distances', None)  

All tests pass on my machine using the above setUp so I've pushed the adjustment. If Travis is also happy with that, I'll aim to cover the other import warning in this module so we can bring its coverage up to 100 %. Thanks for the insight -- I think you'll agree this was a bit tricky.

[tylerjereddy]

Ok, that worked, so I've now added in one more test to bring the coverage up to 100 % for MDAnalysis.analysis.distances, assuming Travis is happy with this new test to cover the contact_matrix exception.

[richardjgowers]

Awesome, I didn't know it was that simple to fix. Looking good now