launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/memoryfeaturefixture into lp:launchpad.

Commit message:
Add MemoryFeatureRuleSource and MemoryFeatureFixture for database-free feature flag tests.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/memoryfeaturefixture/+merge/208275

Our default FeatureController uses StormFeatureRuleSource, which obviously requires database access. FeatureFixture assumes that it can just poke in the database to set flags, which makes it not terribly useful in tests that don't run with a database.

This branch adds MemoryFeatureRuleSource and MemoryFeatureFixture, allowing testing of feature flags in quick DB-less infrastructure tests.
-- 
https://code.launchpad.net/~wgrant/launchpad/memoryfeaturefixture/+merge/208275
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/memoryfeaturefixture into lp:launchpad.

=== modified file 'lib/lp/services/features/rulesource.py'
--- lib/lp/services/features/rulesource.py	2013-04-09 08:29:04 +0000
+++ lib/lp/services/features/rulesource.py	2014-02-26 05:05:24 +0000
@@ -6,6 +6,7 @@
 __all__ = [
     'DuplicatePriorityError',
     'FeatureRuleSource',
+    'MemoryFeatureRuleSource',
     'NullFeatureRuleSource',
     'StormFeatureRuleSource',
     ]
@@ -152,6 +153,24 @@
         store.flush()
 
 
+class MemoryFeatureRuleSource(FeatureRuleSource):
+    """Access feature rules stored in non-persistent memory.
+    """
+
+    def __init__(self):
+        self.rules = []
+
+    def getAllRulesAsTuples(self):
+        return self.rules
+
+    def setAllRules(self, new_rules):
+        """Replace all existing rules with a new set.
+
+        :param new_rules: List of (name, scope, priority, value) tuples.
+        """
+        self.rules = [Rule(*r) for r in new_rules]
+
+
 class NullFeatureRuleSource(FeatureRuleSource):
     """For use in testing: everything is turned off"""
 

=== modified file 'lib/lp/services/features/testing.py'
--- lib/lp/services/features/testing.py	2013-08-14 11:15:13 +0000
+++ lib/lp/services/features/testing.py	2014-02-26 05:05:24 +0000
@@ -4,7 +4,10 @@
 """Helpers for writing tests that use feature flags."""
 
 __metaclass__ = type
-__all__ = ['FeatureFixture']
+__all__ = [
+    'FeatureFixture',
+    'MemoryFeatureFixture',
+    ]
 
 
 from fixtures import Fixture
@@ -17,6 +20,7 @@
     )
 from lp.services.features.flags import FeatureController
 from lp.services.features.rulesource import (
+    MemoryFeatureRuleSource,
     Rule,
     StormFeatureRuleSource,
     )
@@ -40,22 +44,7 @@
     return dbadmin_retry
 
 
-class FeatureFixture(Fixture):
-    """A fixture that sets a feature.
-
-    The fixture takes a dictionary as its constructor argument. The keys of
-    the dictionary are features to be set. All existing flags will be cleared.
-
-    Call the fixture's `setUp()' method to install the features with the
-    desired values.  Calling `cleanUp()' will restore the original values.
-    You can also install this fixture by inheriting from
-    `fixtures.TestWithFixtures' and then calling the TestCase's
-    `self.useFixture()' method.
-
-    The fixture can also be used as a context manager. The value of the
-    feature within the context block is set to the dictionary's key's value.
-    The values are restored when the block exits.
-    """
+class FeatureFixtureMixin:
 
     def __init__(self, features_dict, full_feature_rules=None,
             override_scope_lookup=None):
@@ -73,13 +62,9 @@
 
     def setUp(self):
         """Set the feature flags that this fixture is responsible for."""
-        super(FeatureFixture, self).setUp()
+        super(FeatureFixtureMixin, self).setUp()
 
-        rule_source = StormFeatureRuleSource()
-        self.addCleanup(
-            dbadmin(rule_source.setAllRules),
-            dbadmin(rule_source.getAllRulesAsTuples)())
-        dbadmin(rule_source.setAllRules)(self.makeNewRules())
+        rule_source = self.makeRuleSource(self.makeNewRules())
 
         original_controller = get_relevant_feature_controller()
 
@@ -93,7 +78,6 @@
             FeatureController(scope_lookup, rule_source))
         self.addCleanup(install_feature_controller, original_controller)
 
-    @dbadmin
     def makeNewRules(self):
         """Make a set of new feature flag rules."""
         # Create a list of the new rules. Note that rules with a None
@@ -118,3 +102,52 @@
                 for rule_spec in self.full_feature_rules)
 
         return new_rules
+
+
+class FeatureFixture(FeatureFixtureMixin, Fixture):
+    """A fixture that sets a feature in a database-backed feature controller.
+
+    The fixture takes a dictionary as its constructor argument. The keys of
+    the dictionary are features to be set. All existing flags will be cleared.
+
+    Call the fixture's `setUp()' method to install the features with the
+    desired values.  Calling `cleanUp()' will restore the original values.
+    You can also install this fixture by inheriting from
+    `fixtures.TestWithFixtures' and then calling the TestCase's
+    `self.useFixture()' method.
+
+    The fixture can also be used as a context manager. The value of the
+    feature within the context block is set to the dictionary's key's value.
+    The values are restored when the block exits.
+    """
+
+    def makeRuleSource(self, rules):
+        rule_source = StormFeatureRuleSource()
+        self.addCleanup(
+            dbadmin(rule_source.setAllRules),
+            dbadmin(rule_source.getAllRulesAsTuples)())
+        dbadmin(rule_source.setAllRules)(rules)
+        return rule_source
+
+
+class MemoryFeatureFixture(FeatureFixtureMixin, Fixture):
+    """A fixture that sets a feature in an in-memory feature controller.
+
+    The fixture takes a dictionary as its constructor argument. The keys of
+    the dictionary are features to be set. All existing flags will be cleared.
+
+    Call the fixture's `setUp()' method to install the features with the
+    desired values.  Calling `cleanUp()' will restore the original values.
+    You can also install this fixture by inheriting from
+    `fixtures.TestWithFixtures' and then calling the TestCase's
+    `self.useFixture()' method.
+
+    The fixture can also be used as a context manager. The value of the
+    feature within the context block is set to the dictionary's key's value.
+    The values are restored when the block exits.
+    """
+
+    def makeRuleSource(self, rules):
+        rule_source = MemoryFeatureRuleSource()
+        rule_source.setAllRules(rules)
+        return rule_source

=== modified file 'lib/lp/services/features/tests/test_flags.py'
--- lib/lp/services/features/tests/test_flags.py	2011-12-28 17:03:06 +0000
+++ lib/lp/services/features/tests/test_flags.py	2014-02-26 05:05:24 +0000
@@ -13,7 +13,10 @@
     install_feature_controller,
     )
 from lp.services.features.flags import FeatureController
-from lp.services.features.rulesource import StormFeatureRuleSource
+from lp.services.features.rulesource import (
+    MemoryFeatureRuleSource,
+    StormFeatureRuleSource,
+    )
 from lp.testing import (
     layers,
     TestCase,
@@ -207,19 +210,17 @@
     ]
 
 
-class TestStormFeatureRuleSource(TestCase):
-
-    layer = layers.DatabaseFunctionalLayer
+class FeatureRuleSourceTestsMixin:
 
     def test_getAllRulesAsTuples(self):
-        source = StormFeatureRuleSource()
+        source = self.makeSource()
         source.setAllRules(test_rules_list)
         self.assertEquals(
             test_rules_list,
             list(source.getAllRulesAsTuples()))
 
     def test_getAllRulesAsText(self):
-        source = StormFeatureRuleSource()
+        source = self.makeSource()
         source.setAllRules(test_rules_list)
         self.assertEquals(
             """\
@@ -232,7 +233,7 @@
 
     def test_setAllRulesFromText(self):
         # We will overwrite existing data.
-        source = StormFeatureRuleSource()
+        source = self.makeSource()
         source.setAllRules(test_rules_list)
         source.setAllRulesFromText("""
 
@@ -250,3 +251,19 @@
                 ],
             },
             source.getAllRulesAsDict())
+
+
+class TestStormFeatureRuleSource(FeatureRuleSourceTestsMixin, TestCase):
+
+    layer = layers.DatabaseFunctionalLayer
+
+    def makeSource(self):
+        return StormFeatureRuleSource()
+
+
+class TestMemoryFeatureRuleSource(FeatureRuleSourceTestsMixin, TestCase):
+
+    layer = layers.FunctionalLayer
+
+    def makeSource(self):
+        return MemoryFeatureRuleSource()

=== modified file 'lib/lp/services/features/tests/test_helpers.py'
--- lib/lp/services/features/tests/test_helpers.py	2011-12-28 17:03:06 +0000
+++ lib/lp/services/features/tests/test_helpers.py	2014-02-26 05:05:24 +0000
@@ -6,43 +6,32 @@
 __metaclass__ = type
 __all__ = []
 
-from lp.services.features import getFeatureFlag
-from lp.services.features.testing import FeatureFixture
+from lp.services.features import (
+    get_relevant_feature_controller,
+    getFeatureFlag,
+    )
+from lp.services.features.rulesource import (
+    MemoryFeatureRuleSource,
+    StormFeatureRuleSource,
+    )
+from lp.services.features.testing import (
+    FeatureFixture,
+    MemoryFeatureFixture,
+    )
 from lp.testing import (
     layers,
     TestCase,
     )
 
 
-class TestFeaturesContextManager(TestCase):
-    """Tests for the feature flags context manager test helper."""
-
-    layer = layers.DatabaseFunctionalLayer
-
-    def test_setting_one_flag_with_manager(self):
-        flag = self.getUniqueString()
-        value_outside_manager = getFeatureFlag(flag)
-        value_in_manager = None
-
-        with FeatureFixture({flag: u'on'}):
-            value_in_manager = getFeatureFlag(flag)
-
-        self.assertEqual(value_in_manager, u'on')
-        self.assertEqual(value_outside_manager, getFeatureFlag(flag))
-        self.assertNotEqual(value_outside_manager, value_in_manager)
-
-
-class TestFeaturesFixture(TestCase):
-    """Tests for the feature flags test fixture."""
-
-    layer = layers.DatabaseFunctionalLayer
+class FeatureFixturesTestsMixin:
 
     def test_fixture_sets_one_flag_and_cleans_up_again(self):
         flag = self.getUniqueString()
         value_before_fixture_setup = getFeatureFlag(flag)
         value_after_fixture_setup = None
 
-        fixture = FeatureFixture({flag: 'on'})
+        fixture = self.fixture_cls({flag: 'on'})
         fixture.setUp()
         value_after_fixture_setup = getFeatureFlag(flag)
         fixture.cleanUp()
@@ -53,18 +42,58 @@
             value_before_fixture_setup, value_after_fixture_setup)
 
     def test_fixture_deletes_existing_values(self):
-        self.useFixture(FeatureFixture({'one': '1'}))
-        self.useFixture(FeatureFixture({'two': '2'}))
+        self.useFixture(self.fixture_cls({'one': '1'}))
+        self.useFixture(self.fixture_cls({'two': '2'}))
 
         self.assertEqual(getFeatureFlag('one'), None)
         self.assertEqual(getFeatureFlag('two'), u'2')
 
     def test_fixture_overrides_previously_set_flags(self):
-        self.useFixture(FeatureFixture({'one': '1'}))
-        self.useFixture(FeatureFixture({'one': '5'}))
+        self.useFixture(self.fixture_cls({'one': '1'}))
+        self.useFixture(self.fixture_cls({'one': '5'}))
 
         self.assertEqual(getFeatureFlag('one'), u'5')
 
     def test_fixture_does_not_set_value_for_flags_that_are_None(self):
-        self.useFixture(FeatureFixture({'nothing': None}))
+        self.useFixture(self.fixture_cls({'nothing': None}))
         self.assertEqual(getFeatureFlag('nothing'), None)
+
+    def test_setting_one_flag_with_context_manager(self):
+        flag = self.getUniqueString()
+        value_outside_manager = getFeatureFlag(flag)
+        value_in_manager = None
+
+        with self.fixture_cls({flag: u'on'}):
+            value_in_manager = getFeatureFlag(flag)
+
+        self.assertEqual(value_in_manager, u'on')
+        self.assertEqual(value_outside_manager, getFeatureFlag(flag))
+        self.assertNotEqual(value_outside_manager, value_in_manager)
+
+
+class TestFeatureFixture(FeatureFixturesTestsMixin, TestCase):
+    """Tests for the feature flags test fixture."""
+
+    layer = layers.DatabaseFunctionalLayer
+
+    fixture_cls = FeatureFixture
+
+    def test_fixture_uses_storm(self):
+        self.useFixture(self.fixture_cls({'one': '1'}))
+        self.assertIsInstance(
+            get_relevant_feature_controller().rule_source,
+            StormFeatureRuleSource)
+
+
+class TestMemoryFeatureFixture(FeatureFixturesTestsMixin, TestCase):
+    """Tests for the feature flags test fixture."""
+
+    layer = layers.FunctionalLayer
+
+    fixture_cls = MemoryFeatureFixture
+
+    def test_fixture_uses_memory(self):
+        self.useFixture(self.fixture_cls({'one': '1'}))
+        self.assertIsInstance(
+            get_relevant_feature_controller().rule_source,
+            MemoryFeatureRuleSource)