Fix importing anydbm within pytest

[pfhayes]

Target: master

Addresses the issue here: #2248

I'm not sure how to test this but am happy to add tests

[coveralls]

Coverage increased (+0.0008%) to 92.709% when pulling 00ec303 on pfhayes:anydbmfix into 427bf42 on pytest-dev:master.

[nicoddemus]

Thanks a lot @pfhayes for the PR, we appreciate it.

[nicoddemus]

Probably this can be made more general, since it is not exclusive to anydbm. Perhaps: Fix issue in assertion rewriting breaking due to modules silently discarding other modules when importing fails?

[nicoddemus]

This looks good, I think we should add a test for it though.

I tried this test on master, but it passes:

pytest_plugins = ['pytester']

def test_import_error(testdir):
    testdir.syspathinsert()
    testdir.makepyfile(top_module='''
        try:
            import other_module
        except ImportError:
            pass
    ''')
    testdir.makepyfile('''
        def test_import_error():
            import top_module
            assert 1
    ''')
    r = testdir.runpytest()
    r.stdout.fnmatch_lines(['* 1 passed *'])

Perhaps anydbm is doing some more magic behind the scenes?

[coveralls]

Coverage increased (+0.0008%) to 92.709% when pulling 49289fe on pfhayes:anydbmfix into 427bf42 on pytest-dev:master.

[pfhayes]

Thanks @nicoddemus

I updated the CHANGELOG at your request

I've been trying to add in a test for this behaviour but can't get one to work. Here's where I landed, which matches exactly the anydbm -> dbhash -> bsddb import chain, but passes in test mode.

+def test_troublesome_imports(testdir):
+    testdir.syspathinsert()
+    testdir.makepyfile(third_module='''
+        import fakething
+    ''')
+    testdir.makepyfile(other_module='''
+        import sys
+        try:
+            import third_module
+        except ImportError:
+            del sys.modules[__name__]
+            raise
+    ''')
+    testdir.makepyfile(top_module='''
+        import sys
+        try:
+            __import__('other_module')
+        except ImportError:
+            pass
+    ''')
+    testdir.makepyfile('''
+        def test_import_error():
+            import top_module
+            assert 1
+    ''')
+    r = testdir.runpytest()
+    r.stdout.fnmatch_lines(['* 1 passed *'])

However, this test also passes, which is surprising to me

+    testdir.makepyfile('''
+        def test_import_error():
+            import anydbm
+            assert 1
+    ''')
+    r = testdir.runpytest()
+    r.stdout.fnmatch_lines(['* 1 passed *'])

Is it possible that this error would not occur while testing?

[coveralls]

Coverage increased (+0.0008%) to 92.709% when pulling 6b5566d on pfhayes:anydbmfix into 427bf42 on pytest-dev:master.

[RonnyPfannschmidt]

lib/python2.7/dbhash.py is

import sys
import warnings
warnings.warnpy3k("in 3.x, the dbhash module has been removed", stacklevel=2)
try:
    import bsddb
except ImportError:
    # prevent a second import of this module from spuriously succeeding
    del sys.modules[__name__]
    raise

__all__ = ["error","open"]

error = bsddb.error                     # Exported for anydbm

def open(file, flag = 'r', mode=0666):
    return bsddb.hashopen(file, flag, mode)

so a module that removes itself is possible and an import hook may be supposed NOT to clean up
we may need to revisit the last statement

@benjaminp do you have any insights on this detail?

[RonnyPfannschmidt]

@pfhayes the main and interesting question is, how and why the interaction is set up to fail,
because the module should not be part of the assertion rewriter interaction to begin with

so what you see in your environment, is something that should happen neither in test not in normal usage and its not surprising that it doesnt happen

i believe marking one of those modules for assertion rewriting may trigger the exception
do you perhaps use any pytest-plugin thats a bit eager to mark more than desired for rewriting

[nicoddemus]

Cool @RonnyPfannschmidt, nice find.

I think the module is not actually being rewritten for asserts, it is just being handled by the AssertRewriteHook: I always found it strange that we rewrite the modules during find_module instead of load_module, but that's how it works today and @pfhayes patch seems correct.

[pfhayes]

Regarding plugins: In the past we have used pytest-mock and hypothesis however I was able to reproduce the error in a fresh virtualenv without those plugins installed.

[pfhayes]

I'm happy to add a test for this case but a bit lost about how to add one that reproduces this behaviour. Any advice? Or can we merge this in without a test?

[nicoddemus]

I can't reproduce either. Since the change is dead simple (one might consider a small oversight in the implementation) I think we can merge it without an explicit test.

What do you think @RonnyPfannschmidt?

[pfhayes]

Sounds good to me! Let me know if I can provide anything else

[nicoddemus]

@RonnyPfannschmidt gentle ping 😄

[nicoddemus]

Thanks again @pfhayes!