Repository: canonical/operator
Pull request #61 (2019-11-29 11:28:11)

Do away with FakeModelBackend
A rework of model testing to make sure hook tool execution and result
handling is exercised.


Patch:
diff --git a/op/model.py b/op/model.py
index 87b9440..0a9e064 100644
--- a/op/model.py
+++ b/op/model.py
@@ -31,7 +31,7 @@ class Model:
 
         If relation_id is not given, this will return the Relation instance if the
         relation is established only once or None if it is not established. If this
-        same relation is established multiple times the error TooManyRelatedApps is raised.
+        same relation is established multiple times the error TooManyRelatedAppsError is raised.
         """
         if relation_id is not None:
             if not isinstance(relation_id, int):
@@ -51,7 +51,7 @@ class Model:
             else:
                 # TODO: We need something in the framework to catch and gracefully handle
                 # errors, ideally integrating the error catching with Juju's mechanisms.
-                raise TooManyRelatedApps(relation_name, num_related, 1)
+                raise TooManyRelatedAppsError(relation_name, num_related, 1)
 
     def get_unit(self, unit_name):
         return self._cache.get(Unit, unit_name)
@@ -413,7 +413,7 @@ class Pod:
 class ModelError(Exception):
     pass
 
-class TooManyRelatedApps(ModelError):
+class TooManyRelatedAppsError(ModelError):
     def __init__(self, relation_name, num_related, max_supported):
         super().__init__(f'Too many remote applications on {relation_name} ({num_related} > {max_supported})')
         self.relation_name = relation_name


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index 0d47e99..11d5007 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -13,92 +13,47 @@ import op.model
 import op.charm
 
 
-# TODO: We need some manner of test to validate the actual ModelBackend implementation, round-tripped
-# through the actual subprocess calls. Either this class could implement these functions as executables
-# that were called via subprocess, or more simple tests that just test through ModelBackend while leaving
-# these tests alone, depending on what proves easier.
-class FakeModelBackend:
-    def __init__(self):
-        self.relation_set_calls = []
-
-        self.unit_name = 'myapp/0'
-        self.app_name = 'myapp'
-
-    def relation_ids(self, relation_name):
-        return {
-            'db0': [],
-            'db1': [4],
-            'db2': [5, 6],
-        }[relation_name]
-
-    def relation_list(self, relation_id):
-        try:
-            return {
-                4: ['remoteapp1/0'],
-                5: ['remoteapp1/0'],
-                6: ['remoteapp2/0'],
-            }[relation_id]
-        except KeyError:
-            raise op.model.RelationNotFoundError()
-
-    def relation_get(self, relation_id, member_name, app):
-        try:
-            return {
-                4: {
-                    'myapp/0': {'host': 'myapp-0'},
-                    'remoteapp1/0': {'host': 'remoteapp1-0'},
-                    'myapp': {'password': 'deadbeefcafe'},
-                    'remoteapp1': {'secret': 'cafedeadbeef'}
-                },
-                5: {
-                    'myapp/0': {'host': 'myapp-0'},
-                    'remoteapp1/0': {'host': 'remoteapp1-0'},
-                    'myapp': {'password': 'deadbeefcafe'},
-                    'remoteapp1': {'secret': 'cafedeadbeef'}
-                },
-                6: {
-                    'myapp/0': {'host': 'myapp-0'},
-                    'remoteapp2/0': {'host': 'remoteapp2-0'},
-                    'myapp': {'password': 'deadbeefcafe'},
-                    'remoteapp1': {'secret': 'cafedeadbeef'}
-                },
-            }[relation_id][member_name]
-        except KeyError:
-            raise op.model.RelationNotFoundError()
-
-    def relation_set(self, relation_id, key, value, is_app):
-        if relation_id == 5:
-            raise ValueError()
-        self.relation_set_calls.append((relation_id, key, value, is_app))
-
-    def config_get(self):
-        return {
-            'foo': 'foo',
-            'bar': 1,
-            'qux': True,
-        }
-
 class TestModel(unittest.TestCase):
 
     def setUp(self):
-        self.backend = FakeModelBackend()
+        os.environ['JUJU_UNIT_NAME'] = 'myapp/0'
+        self.addCleanup(os.environ.pop, 'JUJU_UNIT_NAME')
+
+        self.backend = op.model.ModelBackend()
         meta = op.charm.CharmMeta()
         meta.relations = {'db0': None, 'db1': None, 'db2': None}
         self.model = op.model.Model('myapp/0', meta, self.backend)
 
-        os.environ['JUJU_UNIT_NAME'] = 'myapp/0'
-        self.addCleanup(os.environ.pop, 'JUJU_UNIT_NAME')
-
     def test_model(self):
         self.assertIs(self.model.app, self.model.unit.app)
 
     def test_relations_keys(self):
+        fake_script(self, 'relation-ids',
+                    """[ "$1" = db2 ] && echo '["db2:5", "db2:6"]' || echo '[]'""")
+        fake_script(self, 'relation-list',
+                    """([ "$2" = 5 ] && echo '["remoteapp1/0", "remoteapp1/1"]') || ([ "$2" = 6 ] && echo '["remoteapp2/0"]') || exit 2""")
+
         for relation in self.model.relations['db2']:
             self.assertIn(self.model.unit, relation.data)
             unit_from_rel = next(filter(lambda u: u.name == 'myapp/0', relation.data.keys()))
             self.assertIs(self.model.unit, unit_from_rel)
 
+        self.assertEqual(fake_script_calls(self), [
+            ['relation-ids', 'db2', '--format=json'],
+            ['relation-list', '-r', '5', '--format=json'],
+            ['relation-list', '-r', '6', '--format=json']
+        ])
+
     def test_get_relation(self):
+        err_msg = "ERROR invalid value \"$2\" for option -r: relation not found"
+
+        fake_script(self, 'relation-ids',
+                    """([ "$1" = db1 ] && echo '["db1:4"]') || ([ "$1" = db2 ] && echo '["db2:5", "db2:6"]') || echo '[]'""")
+        fake_script(self, 'relation-list',
+                    f"""([ "$2" = 4 ] && echo '["remoteapp1/0"]') || (echo {err_msg} >&2 ; exit 2)""")
+        fake_script(self, 'relation-get',
+                    f"""echo {err_msg} >&2 ; exit 2""")
+
         with self.assertRaises(op.model.ModelError):
             self.model.get_relation('db1', 'db1:4')
         db1_4 = self.model.get_relation('db1', 4)
@@ -109,25 +64,56 @@ class TestModel(unittest.TestCase):
         self.assertEqual(dead_rel.data[self.model.unit], {})
         self.assertIsNone(self.model.get_relation('db0'))
         self.assertIs(self.model.get_relation('db1'), db1_4)
-        with self.assertRaises(op.model.TooManyRelatedApps):
+        with self.assertRaises(op.model.TooManyRelatedAppsError):
             self.model.get_relation('db2')
 
+        self.assertEqual(fake_script_calls(self), [
+            ['relation-ids', 'db1', '--format=json'],
+            ['relation-list', '-r', '4', '--format=json'],
+            ['relation-list', '-r', '7', '--format=json'],
+            ['relation-get', '-r', '7', '-', 'myapp/0', '--app=False', '--format=json'],
+            ['relation-ids', 'db0', '--format=json'],
+            ['relation-ids', 'db2', '--format=json'],
+            ['relation-list', '-r', '5', '--format=json'],
+            ['relation-list', '-r', '6', '--format=json']
+        ])
+
     def test_remote_units_is_our(self):
+        fake_script(self, 'relation-ids',
+                    """[ "$1" = db1 ] && echo '["db1:4"]' || echo '[]'""")
+        fake_script(self, 'relation-list',
+                    """[ "$2" = 4 ] && echo '["remoteapp1/0", "remoteapp1/1"]' || exit 2""")
+
         for u in self.model.get_relation('db1').units:
             self.assertFalse(u._is_our_unit)
             self.assertFalse(u.app._is_our_app)
 
+        self.assertEqual(fake_script_calls(self), [
+            ['relation-ids', 'db1', '--format=json'],
+            ['relation-list', '-r', '4', '--format=json']
+        ])
+
     def test_our_unit_is_our(self):
         self.assertTrue(self.model.unit._is_our_unit)
         self.assertTrue(self.model.unit.app._is_our_app)
 
     def test_unit_relation_data(self):
+        fake_script(self, 'relation-ids', """[ "$1" = db1 ] && echo '["db1:4"]' || echo '[]'""")
+        fake_script(self, 'relation-list', """[ "$2" = 4 ] && echo '["remoteapp1/0"]' || exit 2""")
+        fake_script(self, 'relation-get', """([ "$2" = 4 ] && [ "$4" = "remoteapp1/0" ]) && echo '{"host": "remoteapp1-0"}' || exit 2""")
+
         random_unit = self.model._cache.get(op.model.Unit, 'randomunit/0')
         with self.assertRaises(KeyError):
             self.model.get_relation('db1').data[random_unit]
         remoteapp1_0 = next(filter(lambda u: u.name == 'remoteapp1/0', self.model.get_relation('db1').units))
         self.assertEqual(self.model.get_relation('db1').data[remoteapp1_0], {'host': 'remoteapp1-0'})
 
+        self.assertEqual(fake_script_calls(self), [
+            ['relation-ids', 'db1', '--format=json'],
+            ['relation-list', '-r', '4', '--format=json'],
+            ['relation-get', '-r', '4', '-', 'remoteapp1/0', '--app=False', '--format=json']
+        ])
+
     def test_remote_app_relation_data(self):
         self.backend = op.model.ModelBackend()
         meta = op.charm.CharmMeta()
@@ -153,23 +139,43 @@ class TestModel(unittest.TestCase):
         ])
 
     def test_relation_data_modify_remote(self):
+        fake_script(self, 'relation-ids', """[ "$1" = db1 ] && echo '["db1:4"]' || echo '[]'""")
+        fake_script(self, 'relation-list', """[ "$2" = 4 ] && echo '["remoteapp1/0"]' || exit 2""")
+        fake_script(self, 'relation-get', """([ "$2" = 4 ] && [ "$4" = "remoteapp1/0" ]) && echo '{"host": "remoteapp1-0"}' || exit 2""")
+
         rel_db1 = self.model.get_relation('db1')
         remoteapp1_0 = next(filter(lambda u: u.name == 'remoteapp1/0', self.model.get_relation('db1').units))
         # Force memory cache to be loaded.
         self.assertIn('host', rel_db1.data[remoteapp1_0])
         with self.assertRaises(op.model.RelationDataError):
             rel_db1.data[remoteapp1_0]['foo'] = 'bar'
-        self.assertEqual(self.backend.relation_set_calls, [])
         self.assertNotIn('foo', rel_db1.data[remoteapp1_0])
 
+        self.assertEqual(fake_script_calls(self), [
+            ['relation-ids', 'db1', '--format=json'],
+            ['relation-list', '-r', '4', '--format=json'],
+            ['relation-get', '-r', '4', '-', 'remoteapp1/0', '--app=False', '--format=json']
+        ])
+
     def test_relation_data_modify_our(self):
+        fake_script(self, 'relation-ids', """[ "$1" = db1 ] && echo '["db1:4"]' || echo '[]'""")
+        fake_script(self, 'relation-list', """[ "$2" = 4 ] && echo '["remoteapp1/0"]' || exit 2""")
+        fake_script(self, 'relation-set', '''[ "$2" = 4 ] && exit 0 || exit 2''')
+        fake_script(self, 'relation-get', """([ "$2" = 4 ] && [ "$4" = "myapp/0" ]) && echo '{"host": "bar"}' || exit 2""")
+
         rel_db1 = self.model.get_relation('db1')
         # Force memory cache to be loaded.
         self.assertIn('host', rel_db1.data[self.model.unit])
         rel_db1.data[self.model.unit]['host'] = 'bar'
-        self.assertEqual(self.backend.relation_set_calls, [(4, 'host', 'bar', False)])
         self.assertEqual(rel_db1.data[self.model.unit]['host'], 'bar')
 
+        self.assertEqual(fake_script_calls(self), [
+            ['relation-ids', 'db1', '--format=json'],
+            ['relation-list', '-r', '4', '--format=json'],
+            ['relation-get', '-r', '4', '-', 'myapp/0', '--app=False', '--format=json'],
+            ['relation-set', '-r', '4', 'host=bar', '--app=False']
+        ])
+
     def test_app_relation_data_modify_local_as_leader(self):
         self.backend = op.model.ModelBackend()
         meta = op.charm.CharmMeta()
@@ -226,24 +232,50 @@ class TestModel(unittest.TestCase):
         ])
 
     def test_relation_data_del_key(self):
+        fake_script(self, 'relation-ids', """[ "$1" = db1 ] && echo '["db1:4"]' || echo '[]'""")
+        fake_script(self, 'relation-list', """[ "$2" = 4 ] && echo '["remoteapp1/0"]' || exit 2""")
+        fake_script(self, 'relation-set', '''[ "$2" = 4 ] && exit 0 || exit 2''')
+        fake_script(self, 'relation-get', """([ "$2" = 4 ] && [ "$4" = "myapp/0" ]) && echo '{"host": "bar"}' || exit 2""")
+
         rel_db1 = self.model.get_relation('db1')
         # Force memory cache to be loaded.
         self.assertIn('host', rel_db1.data[self.model.unit])
         del rel_db1.data[self.model.unit]['host']
-        self.assertEqual(self.backend.relation_set_calls, [(4, 'host', '', False)])
+        fake_script(self, 'relation-get', """([ "$2" = 4 ] && [ "$4" = "myapp/0" ]) && echo '{}' || exit 2""")
         self.assertNotIn('host', rel_db1.data[self.model.unit])
 
+        self.assertEqual(fake_script_calls(self), [
+            ['relation-ids', 'db1', '--format=json'],
+            ['relation-list', '-r', '4', '--format=json'],
+            ['relation-get', '-r', '4', '-', 'myapp/0', '--app=False', '--format=json'],
+            ['relation-set', '-r', '4', 'host=', '--app=False']
+        ])
+
     def test_relation_set_fail(self):
+        fake_script(self, 'relation-ids', """[ "$1" = db2 ] && echo '["db2:5"]' || echo '[]'""")
+        fake_script(self, 'relation-list',
+                    """[ "$2" = 5 ] && echo '["remoteapp1/0"]' || exit 2""")
+        fake_script(self, 'relation-get', """([ "$2" = 5 ] && [ "$4" = "myapp/0" ]) && echo '{"host": "myapp-0"}' || exit 2""")
+        fake_script(self, 'relation-set', 'exit 2')
+
         rel_db2 = self.model.relations['db2'][0]
         # Force memory cache to be loaded.
         self.assertIn('host', rel_db2.data[self.model.unit])
-        with self.assertRaises(ValueError):
+        with self.assertRaises(op.model.ModelError):
             rel_db2.data[self.model.unit]['host'] = 'bar'
         self.assertEqual(rel_db2.data[self.model.unit]['host'], 'myapp-0')
-        with self.assertRaises(ValueError):
+        with self.assertRaises(op.model.ModelError):
             del rel_db2.data[self.model.unit]['host']
         self.assertIn('host', rel_db2.data[self.model.unit])
 
+        self.assertEqual(fake_script_calls(self), [
+            ['relation-ids', 'db2', '--format=json'],
+            ['relation-list', '-r', '5', '--format=json'],
+            ['relation-get', '-r', '5', '-', 'myapp/0', '--app=False', '--format=json'],
+            ['relation-set', '-r', '5', 'host=bar', '--app=False'],
+            ['relation-set', '-r', '5', 'host=', '--app=False']
+        ])
+
     def test_relation_get_set_is_app_arg(self):
         self.backend = op.model.ModelBackend()
 
@@ -263,6 +295,11 @@ class TestModel(unittest.TestCase):
                 self.backend.relation_get(1, 'fooentity', is_app=is_app_v)
 
     def test_relation_data_type_check(self):
+        fake_script(self, 'relation-ids', """[ "$1" = db1 ] && echo '["db1:4"]' || echo '[]'""")
+        fake_script(self, 'relation-list',
+                    """[ "$2" = 4 ] && echo '["remoteapp1/0"]' || exit 2""")
+        fake_script(self, 'relation-get', """([ "$2" = 4 ] && [ "$4" = "myapp/0" ]) && echo '{"host": "myapp-0"}' || exit 2""")
+
         rel_db1 = self.model.get_relation('db1')
         with self.assertRaises(op.model.RelationDataError):
             rel_db1.data[self.model.unit]['foo'] = 1
@@ -270,9 +307,14 @@ class TestModel(unittest.TestCase):
             rel_db1.data[self.model.unit]['foo'] = {'foo': 'bar'}
         with self.assertRaises(op.model.RelationDataError):
             rel_db1.data[self.model.unit]['foo'] = None
-        self.assertEqual(self.backend.relation_set_calls, [])
+
+        self.assertEqual(fake_script_calls(self), [
+            ['relation-ids', 'db1', '--format=json'],
+            ['relation-list', '-r', '4', '--format=json']
+        ])
 
     def test_config(self):
+        fake_script(self, 'config-get', """echo '{"foo":"foo","bar":1,"qux":true}'""")
         self.assertEqual(self.model.config, {
             'foo': 'foo',
             'bar': 1,
@@ -282,12 +324,9 @@ class TestModel(unittest.TestCase):
             # Confirm that we cannot modify config values.
             self.model.config['foo'] = 'bar'
 
-    def test_is_leader(self):
-        self.backend = op.model.ModelBackend()
-        meta = op.charm.CharmMeta()
-        meta.relations = {'db0': None, 'db1': None, 'db2': None}
-        self.model = op.model.Model('myapp/0', meta, self.backend)
+        self.assertEqual(fake_script_calls(self), [['config-get', '--format=json']])
 
+    def test_is_leader(self):
         def check_remote_units():
             fake_script(self, 'relation-ids',
                         """[ "$1" = db1 ] && echo '["db1:4"]' || echo '[]'""")
@@ -305,7 +344,10 @@ class TestModel(unittest.TestCase):
 
         check_remote_units()
 
+        # Create a new model and backend to drop a cached is-leader output.
         self.backend = op.model.ModelBackend()
+        meta = op.charm.CharmMeta()
+        meta.relations = {'db0': None, 'db1': None, 'db2': None}
         self.model = op.model.Model('myapp/0', meta, self.backend)
 
         fake_script(self, 'is-leader', 'echo false')
@@ -323,11 +365,6 @@ class TestModel(unittest.TestCase):
         ])
 
     def test_is_leader_refresh(self):
-        self.backend = op.model.ModelBackend()
-        meta = op.charm.CharmMeta()
-        meta.relations = {'db0': None, 'db1': None, 'db2': None}
-        self.model = op.model.Model('myapp/0', meta, self.backend)
-
         # A sanity check.
         self.assertGreater(time.monotonic(), op.model.ModelBackend.LEASE_RENEWAL_PERIOD.total_seconds())
 
@@ -345,10 +382,9 @@ class TestModel(unittest.TestCase):
         self.assertTrue(self.model.unit.is_leader())
 
     def test_resources(self):
-        backend = op.model.ModelBackend()
         meta = op.charm.CharmMeta()
         meta.resources = {'foo': None, 'bar': None}
-        model = op.model.Model('myapp/0', meta, backend)
+        model = op.model.Model('myapp/0', meta, self.backend)
 
         with self.assertRaises(RuntimeError):
             model.resources.fetch('qux')
@@ -362,10 +398,6 @@ class TestModel(unittest.TestCase):
         self.assertEqual(model.resources.fetch('bar').name, 'bar.tgz')
 
     def test_pod_spec(self):
-        meta = op.charm.CharmMeta()
-        meta.relations = {'db0': None, 'db1': None, 'db2': None}
-        model = op.model.Model('myapp/0', meta, op.model.ModelBackend())
-
         fake_script(self, 'pod-spec-set', """
                     cat $2 > $(dirname $0)/spec.json
                     [[ -n $4 ]] && cat $4 > $(dirname $0)/k8s_res.json || true
@@ -380,14 +412,14 @@ class TestModel(unittest.TestCase):
             # Other characters are from POSIX 3.282 (Portable Filename Character Set) a subset of which Python's mkdtemp uses.
             self.assertTrue(re.match('/tmp/tmp[A-Za-z0-9._-]{8}-pod-spec-set', fake_calls[0][2]))
 
-        model.pod.set_spec({'foo': 'bar'})
+        self.model.pod.set_spec({'foo': 'bar'})
         self.assertEqual(spec_path.read_text(), '{"foo": "bar"}')
         self.assertFalse(k8s_res_path.exists())
 
         fake_calls = fake_script_calls(self, clear=True)
         check_calls(fake_calls)
 
-        model.pod.set_spec({'bar': 'foo'}, {'qux': 'baz'})
+        self.model.pod.set_spec({'bar': 'foo'}, {'qux': 'baz'})
         self.assertEqual(spec_path.read_text(), '{"bar": "foo"}')
         self.assertEqual(k8s_res_path.read_text(), '{"qux": "baz"}')
 
@@ -540,6 +572,9 @@ class TestModel(unittest.TestCase):
                 remote_unit.status = target_status
 
     def test_remote_app_status(self):
+        fake_script(self, 'relation-ids', """[ "$1" = db1 ] && echo '["db1:4"]' || echo '[]'""")
+        fake_script(self, 'relation-list', """[ "$2" = 4 ] && echo '["remoteapp1/0", "remoteapp1/1"]' || exit 2""")
+
         remoteapp1 = self.model.get_relation('db1').app
 
         # Remote application status is always unknown.
@@ -556,6 +591,11 @@ class TestModel(unittest.TestCase):
             with self.assertRaises(RuntimeError):
                 remoteapp1.status = target_status
 
+        self.assertEqual(fake_script_calls(self, clear=True), [
+            ['relation-ids', 'db1', '--format=json'],
+            ['relation-list', '-r', '4', '--format=json'],
+        ])
+
 
 class TestModelBackend(unittest.TestCase):
 
@@ -566,7 +606,7 @@ class TestModelBackend(unittest.TestCase):
         self.backend = op.model.ModelBackend()
 
     def test_relation_tool_errors(self):
-        RELATION_ERROR_MESSAGE = "ERROR invalid value \"$1\" for option -r: relation not found"
+        err_msg = "ERROR invalid value \"$2\" for option -r: relation not found"
 
         test_cases = [(
             lambda: fake_script(self, 'relation-list', f'echo fooerror >&2 ; exit 1'),
@@ -574,7 +614,7 @@ class TestModelBackend(unittest.TestCase):
             op.model.ModelError,
             [['relation-list', '-r', '3', '--format=json']],
         ), (
-            lambda: fake_script(self, 'relation-list', f'echo {RELATION_ERROR_MESSAGE} >&2 ; exit 2'),
+            lambda: fake_script(self, 'relation-list', f'echo {err_msg} >&2 ; exit 2'),
             lambda: self.backend.relation_list(3),
             op.model.RelationNotFoundError,
             [['relation-list', '-r', '3', '--format=json']],
@@ -584,7 +624,7 @@ class TestModelBackend(unittest.TestCase):
             op.model.ModelError,
             [['relation-set', '-r', '3', 'foo=bar', '--app=False']],
         ), (
-            lambda: fake_script(self, 'relation-set', f'echo {RELATION_ERROR_MESSAGE} >&2 ; exit 2'),
+            lambda: fake_script(self, 'relation-set', f'echo {err_msg} >&2 ; exit 2'),
             lambda: self.backend.relation_set(3, 'foo', 'bar', is_app=False),
             op.model.RelationNotFoundError,
             [['relation-set', '-r', '3', 'foo=bar', '--app=False']],
@@ -594,7 +634,7 @@ class TestModelBackend(unittest.TestCase):
             op.model.ModelError,
             [['relation-get', '-r', '3', '-', 'remote/0', '--app=False', '--format=json']],
         ), (
-            lambda: fake_script(self, 'relation-get', f'echo {RELATION_ERROR_MESSAGE} >&2 ; exit 2'),
+            lambda: fake_script(self, 'relation-get', f'echo {err_msg} >&2 ; exit 2'),
             lambda: self.backend.relation_get(3, 'remote/0', is_app=False),
             op.model.RelationNotFoundError,
             [['relation-get', '-r', '3', '-', 'remote/0', '--app=False', '--format=json']],