launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/launchpad/bug-1009712 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1009712 in Launchpad itself: "lp.testing.tests.test_testing.TestFeatureFlags.test_set_feature_flags_raises_if_not_available fails rarely/intermittently in parallel tests"
  https://bugs.launchpad.net/launchpad/+bug/1009712

For more details, see:
https://code.launchpad.net/~gmb/launchpad/bug-1009712/+merge/109315

This branch fixes bug 1009712, which was caused by test_set_feature_flags_raises_if_not_available assuming that there is no feature controller when it gets run (hurrah for implied test isolation).

The fix was simple: I added a function that allows us to uninstall an existing feature controller from the thread and then called that from within the test. I also updated the test's docstring, which was pretty uninformative.
-- 
https://code.launchpad.net/~gmb/launchpad/bug-1009712/+merge/109315
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/bug-1009712 into lp:launchpad.

=== modified file 'lib/lp/bugs/tests/test_bug.py'
--- lib/lp/bugs/tests/test_bug.py	2012-03-13 00:45:33 +0000
+++ lib/lp/bugs/tests/test_bug.py	2012-06-08 08:59:23 +0000
@@ -297,3 +297,8 @@
         params.setBugTarget(product=target)
         bug = getUtility(IBugSet).createBug(params)
         self.assertEqual([cve], [cve_link.cve for cve_link in bug.cve_links])
+
+    def test_noise(self):
+        import sys
+        sys.stdout.write("Hey, look at me!")
+        sys.stderr.write("No, look at me!")

=== modified file 'lib/lp/services/features/__init__.py'
--- lib/lp/services/features/__init__.py	2011-10-27 15:04:26 +0000
+++ lib/lp/services/features/__init__.py	2012-06-08 08:59:23 +0000
@@ -205,9 +205,16 @@
     per_thread.features = controller
 
 
+def uninstall_feature_controller():
+    """Remove the current feature controller from this thread.
+
+    This function is used to create a pristine environment in tests.
+    """
+    del per_thread.features
+
+
 def get_relevant_feature_controller():
     """Get a `FeatureController` for this thread."""
-
     # The noncommittal name "relevant" is because this function may change to
     # look things up from the current request or some other mechanism in
     # future.

=== modified file 'lib/lp/testing/tests/test_testing.py'
--- lib/lp/testing/tests/test_testing.py	2012-01-01 02:58:52 +0000
+++ lib/lp/testing/tests/test_testing.py	2012-06-08 08:59:23 +0000
@@ -9,7 +9,10 @@
 import tempfile
 
 from lp.services.config import config
-from lp.services.features import getFeatureFlag
+from lp.services.features import (
+    getFeatureFlag,
+    uninstall_feature_controller,
+    )
 from lp.testing import (
     feature_flags,
     NestedTempfile,
@@ -25,7 +28,13 @@
     layer = DatabaseFunctionalLayer
 
     def test_set_feature_flags_raises_if_not_available(self):
-        """set_feature_flags prevents mistakes mistakes by raising."""
+        """set_feature_flags raises an error if there is no feature
+        controller available for the current thread.
+        """
+        # Remove any existing feature controller for the sake of this
+        # test (other tests will re-add it). This prevents weird
+        # interactions in a parallel test environment.
+        uninstall_feature_controller()
         self.assertRaises(AssertionError, set_feature_flag, u'name', u'value')
 
     def test_flags_set_within_feature_flags_context(self):