Merge lp:~allenap/launchpad/enable-isolation-tests-bug-551751 into lp:launchpad

[Gavin Panella]

[r=abentley][ui=none][bug=551751] Fix ordering issues in TestIsolation.setUp() and re-enable its test methods.

Amends TestIsolation.setUp() - see the comment I've added to the code
- and re-enables the test methods.

[Aaron Bentley (abentley)]

Don't we always need to define test_suite? Assuming we don't, approved.

I don't really understand why store.execute('SELECT 1') solves these issues, though. Is it simply because it's unknown SQL, so Storm conservatively flushes?

review: Approve

[Gavin Panella (allenap)]

Thank you for the review :)

> Don't we always need to define test_suite? Assuming we don't, approved.

The Zope test runner seems to DTRT now (and many other test runners do too).

> I don't really understand why store.execute('SELECT 1') solves these issues,
> though. Is it simply because it's unknown SQL, so Storm conservatively
> flushes?

The difference is that it now does a "SELECT 1" in every store, not just the first (which might be set to auto-commit). At least one of the stores will be set to SERIALIZABLE isolation. As soon as a query is executed in one of these stores a transaction must be started. I suppose I could execute "BEGIN" instead; maybe that would be clearer. In fact, maybe transaction.begin() would be enough! I wonder if I tried that when I wrote these tests the first time...

=== modified file 'lib/lp/bugs/externalbugtracker/tests/test_isolation.py'
--- lib/lp/bugs/externalbugtracker/tests/test_isolation.py	2010-10-03 15:30:06 +0000
+++ lib/lp/bugs/externalbugtracker/tests/test_isolation.py	2011-01-22 18:03:11 +0000
@@ -5,8 +5,6 @@
 
 __metaclass__ = type
 
-import unittest
-
 from psycopg2.extensions import TRANSACTION_STATUS_IDLE
 from storm.zope.interfaces import IZStorm
 import transaction
@@ -24,12 +22,14 @@
     def createTransaction(self):
         stores = list(store for _, store in getUtility(IZStorm).iterstores())
         self.failUnless(len(stores) > 0, "No stores to test.")
-        stores[0].execute('SELECT 1')
+        # One or more of the stores may be set to auto-commit. The transaction
+        # status remains unchanged for these stores hence they are not useful
+        # for these tests, so execute a query in every store; one of them will
+        # have a transactional state.
+        for store in stores:
+            store.execute('SELECT 1')
 
     def test_gen_store_statuses(self):
-        # XXX: AaronBentley 2010-03-30 bug=551751: Test fails inconsistently.
-        return
-
         # All stores are either disconnected or idle when all
         # transactions have been aborted.
         transaction.abort()
@@ -44,9 +44,6 @@
                 for _, status in isolation.gen_store_statuses()))
 
     def test_is_transaction_in_progress(self):
-        # XXX: AaronBentley 2010-03-30 bug=551751: Test fails inconsistently.
-        return
-
         # is_transaction_in_progress() returns False when all
         # transactions have been aborted.
         transaction.abort()
@@ -57,9 +54,6 @@
         self.failUnless(isolation.is_transaction_in_progress())
 
     def test_check_no_transaction(self):
-        # XXX: AaronBentley 2010-03-30 bug=551751: Test fails inconsistently.
-        return
-
         # check_no_transaction() should be a no-op when there are no
         # transactions in operation.
         transaction.abort()
@@ -72,9 +66,6 @@
             isolation.check_no_transaction)
 
     def test_ensure_no_transaction(self):
-        # XXX: AaronBentley 2010-03-30 bug=551751: Test fails inconsistently.
-        return
-
         # ensure_no_transaction() is a decorator that raises
         # TransactionInProgress if a transaction has begun, else it
         # simply calls the wrapped function.
@@ -91,7 +82,3 @@
         # transaction has begun.
         self.createTransaction()
         self.assertRaises(isolation.TransactionInProgress, echo)
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)