launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/enum into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/enum/+merge/95846

This is something I needed for a branch I'm working on, but it also turned out to overlap with an existing factory method: enumerating an enum.

We may solve this by moving to a proper enum class later, but that involves bikeshedding over which one to use — and even then, it's not a given, perversely, that enumeration is a built-in functionality.  So for now I'm hiding it all behind a simple but flexible function.

At its core, an enum is a dict.  The things you may need to do with it are already in dict, or easily built on top: list keys, list values, look up item, sort by key or value.  So the easiest way to provide flexible introspection is to map the enum into a dict.  It may mean a tiny bit of extra work on top, e.g. "sort keys," but at least a map conversion reduces it to idiomatic standard-library-based code with a minimum of API.

To avoid silly mistakes, I added factory tests.  It may seem overzealous to test things like getRandomBoolean, but we really did have a non-obvious bug in there due to insufficient test coverage.  I wish I could test that full ranges are covered, but it's hard to test that reliably for random.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/enum/+merge/95846
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/enum into lp:maas.

=== added file 'src/maasserver/testing/enum.py'
--- src/maasserver/testing/enum.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/testing/enum.py	2012-03-05 06:16:18 +0000
@@ -0,0 +1,28 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Enumeration helpers."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'map_enum',
+    ]
+
+
+def map_enum(enum_class):
+    """Map out an enumeration class as a "NAME: value" dict."""
+    # Filter out anything that starts with '_', which covers private and
+    # special methods.  We can make this smarter later if we start using
+    # a smarter enumeration base class etc.  Or if we switch to a proper
+    # enum mechanism, this function will act as a marker for pieces of
+    # code that should be updated.
+    return {
+        key: value
+        for key, value in vars(enum_class).items()
+            if not key.startswith('_')
+    }

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2012-02-29 13:48:37 +0000
+++ src/maasserver/testing/factory.py	2012-03-05 06:16:18 +0000
@@ -24,6 +24,7 @@
     Node,
     NODE_STATUS,
     )
+from maasserver.testing.enum import map_enum
 
 
 class Factory():
@@ -37,16 +38,23 @@
         return random.choice((True, False))
 
     def getRandomEnum(self, enum):
-        enum_choices = [
-            value for key, value in vars(enum).items()
-            if not key.startswith('__')]
-        return random.choice(enum_choices)
+        """Pick a random item from an enumeration class.
+
+        :param enum: An enumeration class such as `NODE_STATUS`.
+        :return: The value of one of its items.
+        """
+        return random.choice(list(map_enum(enum).values()))
 
     def getRandomChoice(self, choices):
-        # Get a random choice from the passed-in 'choices'.  'choices'
-        # must use Django form choices format:
-        # [('choice_id_1': "Choice name 1"), ('choice_id_2', "Choice
-        # name 2")].  A random choice id will be returned.
+        """Pick a random item from `choices`.
+
+        :param choices: A sequence of choices in Django form choices format:
+            [
+                ('choice_id_1', "Choice name 1"),
+                ('choice_id_2', "Choice name 2"),
+            ]
+        :return: The "id" portion of a random choice out of `choices`.
+        """
         return random.choice(choices)[0]
 
     def make_node(self, hostname='', set_hostname=False, status=None,

=== added file 'src/maasserver/testing/tests/test_enum.py'
--- src/maasserver/testing/tests/test_enum.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/testing/tests/test_enum.py	2012-03-05 06:16:18 +0000
@@ -0,0 +1,50 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""..."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from maasserver.testing.enum import map_enum
+from maastesting import TestCase
+
+
+class TestEnum(TestCase):
+
+    def test_map_enum_includes_all_enum_values(self):
+
+        class Enum:
+            ONE = 1
+            TWO = 2
+
+        self.assertItemsEqual(['ONE', 'TWO'], map_enum(Enum).keys())
+
+    def test_map_enum_omits_private_or_special_methods(self):
+
+        class Enum:
+            def __init__(self):
+                pass
+
+            def __repr__(self):
+                return "Enum"
+
+            def _save(self):
+                pass
+
+            VALUE = 9
+
+        self.assertItemsEqual(['VALUE'], map_enum(Enum).keys())
+
+    def test_map_enum_maps_values(self):
+
+        class Enum:
+            ONE = 1
+            THREE = 3
+
+        self.assertEqual({'ONE': 1, 'THREE': 3}, map_enum(Enum))

=== added file 'src/maasserver/testing/tests/test_factory.py'
--- src/maasserver/testing/tests/test_factory.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/testing/tests/test_factory.py	2012-03-05 06:16:18 +0000
@@ -0,0 +1,44 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the factory where appropriate.  Don't overdo this."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from random import randint
+
+from maasserver.testing.factory import factory
+from maastesting import TestCase
+
+
+class TestFactory(TestCase):
+
+    def test_getRandomString_respects_size(self):
+        sizes = [1, 10, 100]
+        random_strings = [factory.getRandomString(size) for size in sizes]
+        self.assertEqual(sizes, [len(string) for string in random_strings])
+
+    def test_getRandomBoolean_returns_bool(self):
+        self.assertIsInstance(factory.getRandomBoolean(), bool)
+
+    def test_getRandomEnum_returns_enum_value(self):
+        random_value = randint(0, 99999)
+
+        class Enum:
+            VALUE = random_value
+            OTHER_VALUE = random_value + 3
+
+        self.assertIn(
+            factory.getRandomEnum(Enum), [Enum.VALUE, Enum.OTHER_VALUE])
+
+    def test_getRandomChoice_chooses_from_django_options(self):
+        options = [(2, 'b'), (10, 'j')]
+        self.assertIn(
+            factory.getRandomChoice(options),
+            [option[0] for option in options])