Merge pull request #18 from NextThought/issue17

Handle NoTransaction when aborting transactions as a cleanup. Fixes #17

CHANGES.rst

@@ -6,7 +6,12 @@
 2.2.1 (unreleased)
 ==================
 
-- Nothing changed yet.
+- Make transaction cleanup safer if the default transaction manager
+  has been made explicit.
+
+  Also, reset the default transaction manager to implicit.
+
+  See `issue 17 <https://github.com/NextThought/nti.testing/issues/17>`_.
 
 
 2.2.0 (2018-08-24)

src/nti/testing/__init__.py

@@ -3,15 +3,20 @@
 """
 nti.testing.
 
-Importing this module has side-effects when zope.testing is in use:
+Importing this module has side-effects when zope.testing is
+importable:
 
+    - Add a zope.testing cleanup to ensure that transactions never
+      last past the boundary of a test. If a test begins a transaction
+      and then fails to abort or commit it, subsequent uses of the
+      transaction package may find that they are in a bad state,
+      unable to clean up resources. For example, the dreaded
+      ``ConnectionStateError: Cannot close a connection joined to a
+      transaction``.
+
+    - A zope.testing cleanup also ensures that the global transaction manager
+      is in its default implicit mode, at least for the current thread.
 
-Ensure that transactions never last past the boundary of a test. If a
-test begins a transaction and then fails to abort or commit it,
-subsequent uses of the transaction package may find that they are in a
-bad state, unable to clean up resources. For example, the dreaded
-``ConnectionStateError: Cannot close a connection joined to a
-transaction``
 """
 
 from __future__ import absolute_import
@@ -23,5 +28,18 @@
 
 __docformat__ = "restructuredtext en"
 
-
-zope.testing.cleanup.addCleanUp(transaction.abort)
+def transactionCleanUp():
+    try:
+        transaction.abort()
+    except transaction.interfaces.NoTransaction:
+        # An explicit transaction manager, with nothing
+        # to do. Perfect.
+        pass
+    finally:
+        # Note that we don't catch any other transaction errors.
+        # Those usually mean there's a bug in a resource manager joined
+        # to the transaction and it should fail the test.
+        transaction.manager.explicit = False
+
+
+zope.testing.cleanup.addCleanUp(transactionCleanUp)

src/nti/testing/base.py

@@ -20,7 +20,7 @@
 
 import six
 from six import with_metaclass
-import transaction
+
 from zope import component
 from zope.component import eventtesting
 from zope.component.hooks import setHooks
@@ -32,6 +32,7 @@
 from hamcrest import assert_that
 from hamcrest import is_
 
+from . import transactionCleanUp
 
 logger = __import__('logging').getLogger(__name__)
 
@@ -115,7 +116,7 @@ def _doTearDown(obj, clear_configuration_context=True, super_tear_down=None):
         eventtesting.clearEvents() # redundant with zope.testing.cleanup
         # we never actually want to do this, it's not needed and can mess up other fixtures
         # resetHooks()
-        transaction.abort() # see comments above
+        transactionCleanUp()
         if clear_configuration_context:
             obj.configuration_context = None
         if super_tear_down is not None:
@@ -136,13 +137,15 @@ def get_configuration_package_for_class(klass):
         ``nti.appserver``.
         """
         module = klass.__module__
-        if module:
-            module_parts = module.split('.')
-            if module_parts[-1].startswith('test') and module_parts[-2] == 'tests':
-                module = '.'.join(module_parts[0:-2])
+        if not module: # pragma: no cover
+            return None
+
+        module_parts = module.split('.')
+        if module_parts[-1].startswith('test') and module_parts[-2] == 'tests':
+            module = '.'.join(module_parts[0:-2])
 
-            package = sys.modules[module]
-            return package
+        package = sys.modules[module]
+        return package
 
 
 class AbstractTestBase(zope.testing.cleanup.CleanUp, unittest.TestCase):
@@ -191,7 +194,7 @@ class SharedTestBaseMetaclass(type):
     it's easy to workaround.)
     """
 
-    def __new__(mcs, name, bases, cdict):
+    def __new__(mcs, name, bases, cdict): # pylint:disable=bad-mcs-classmethod-argument
         the_type = type.__new__(mcs, name, bases, cdict)
         # TODO: Based on certain features of the the_type
         # like set_up_packages and features, we can probably

src/nti/testing/layers.py

@@ -14,12 +14,13 @@
 import sys
 import unittest
 
-import transaction
+
 from zope import component
 from zope.component import eventtesting
 from zope.component.hooks import setHooks
 import zope.testing.cleanup
 
+from . import transactionCleanUp
 from .base import AbstractConfiguringObject
 from .base import sharedCleanup
 
@@ -141,7 +142,7 @@ def testSetUp(cls):
     def testTearDown(cls):
         # Some tear down needs to happen always
         eventtesting.clearEvents()
-        transaction.abort() # see comments above
+        transactionCleanUp()
 
 _marker = object()
 

src/nti/testing/tests/test_base.py

@@ -54,6 +54,21 @@ def func():
 
         base._shared_cleanups.remove((func, (), {}))
 
+    def test_explicit_transaction_cleanup(self):
+        import transaction
+        import zope.testing
+        transaction.manager.explicit = True
+        transaction.begin()
+
+        zope.testing.cleanup.cleanUp()
+        assert_that(transaction.manager, has_attr('explicit', False))
+
+    def test_explicit_transaction_cleanup_no_transaction(self):
+        import transaction
+        import zope.testing
+        transaction.manager.explicit = True
+        zope.testing.cleanup.cleanUp()
+        assert_that(transaction.manager, has_attr('explicit', False))
 
     def test_shared_test_base_cover(self):
         # Just coverage.
@@ -83,9 +98,9 @@ def test_thing(self):
                 raise AssertionError("Not called")
 
         mt = MyTest('test_thing')
-        mt.setUp()
+        mt.setUp() # pylint:disable=no-value-for-parameter
         mt.configure_string('<configure xmlns="http://namespaces.zope.org/zope" />')
-        mt.tearDown()
+        mt.tearDown() # pylint:disable=no-value-for-parameter
 
     def test_shared_configuring_base(self):
         import zope.traversing.tests.test_traverser