Factor out the Harness changes for test_model.py #196

jameinel · 2020-03-31T09:16:14Z

If you want to call something like Model.relations['db'] when there
isn't a relation established, we need to have CharmMeta in the
ModelBackend so we give the right errors.

Also, track all the calls to the model backend, so when doing Model tests, we can validate that we interact with the backend in the way we expect.

If you want to call something like Model.relations['db'] when there isn't a relation established, we need to have CharmMeta in the ModelBackend so we give the right errors. Also, track all the calls to the model backend, so we can assert them.

facundobatista

Nice work! Added some comments inline, thanks!

ops/testing.py

This PR needs to go in canonical/operator#196 to avoid canonical/operator#202 when running tests.

jameinel · 2020-04-06T08:28:20Z

ops/testing.py

@@ -319,16 +319,33 @@ def set_leader(self, is_leader=True):
        if is_leader and not was_leader and self._charm is not None and self._hooks_enabled:
            self._charm.on.leader_elected.emit()

+    def get_backend_calls(self, reset=False):


While here, I went ahead and changed the default to "reset=True'. Having written a few tests, it is usually quite a bit clearer to just list the calls since the last time you checked, rather than having a growing list that says [('a',)] then [('a',),('b',)], then ([('a',), ('b',), ('c'),)]

best to change it before anyone depends on it.

Use a record_calls decorator that creates the same output that we've been using, but does so without having to change each method, or ever forget about other args, etc.

niemeyer

Here is a first pass:

niemeyer · 2020-04-08T09:19:43Z

ops/testing.py

@@ -173,7 +173,7 @@ def add_relation(self, relation_name, remote_app, remote_app_data=None):
        #       remote_app_data isn't empty.
        return rel_id

-    def add_relation_unit(self, relation_id, remote_unit_name, remote_unit_data=None):
+    def add_relation_unit(self, relation_id, remote_unit_name, *, remote_unit_data=None):


We need to discuss the idea of these extra parameters, per discussion on that other PR, sooner rather than later, otherwise people will start trusting the parameters and we will drive people mad by breaking their tests.

niemeyer · 2020-04-08T09:22:15Z

ops/testing.py

@@ -319,16 +319,60 @@ def set_leader(self, is_leader=True):
        if is_leader and not was_leader and self._charm is not None and self._hooks_enabled:
            self._charm.on.leader_elected.emit()

+    def get_backend_calls(self, reset=True):
+        """Return the calls that we have made to the TestingModelBackend.


It seems a bit premature to stick that in our exposed API. We should discourage the idea of testing by checking what exact methods the implementation has called, and should instead encourage testing by checking that the expected behavior did take place. This is strongly encouraging the former.

Yeah, it was because I wanted to use Harness for internal testing, but I agree it shouldn't be part of the exposed-to-charmers interface.

niemeyer · 2020-04-08T09:22:33Z

ops/testing.py

+
+
+def record_calls(cls):
+    """Replace methods on cls with methods that record that they have been called.


niemeyer · 2020-04-08T09:23:25Z

ops/testing.py

 class _TestingModelBackend:
    """This conforms to the interface for ModelBackend but provides canned data.

-    You should not use this class directly, it is used by `TestingHarness`_ to drive the model.
+    You should not use this class directly, it is used by `Harness`_ to drive the model.


s/You should not/Do NOT/

niemeyer · 2020-04-08T09:24:40Z

ops/testing.py

+            return self._relation_ids_map[relation_name]
+        except KeyError as e:
+            if relation_name not in self._meta.relations:
+                raise model.ModelError('{} is not a known relation'.format(relation_name)) from e


niemeyer · 2020-04-08T09:25:12Z

ops/testing.py

+        try:
+            return self._relation_list_map[relation_id]
+        except KeyError as e:
+            raise model.RelationNotFoundError from e


Again, nice to see the model being tuned to reflect the real implementation. I suspect we'll have a lot of that in the near future.

niemeyer · 2020-04-08T09:26:54Z

test/test_testing.py

@@ -49,6 +51,25 @@ def test_add_relation(self):
        self.assertEqual([rel_id], backend.relation_ids('db'))
        self.assertEqual([], backend.relation_list(rel_id))

+    def test_add_relation_must_name_app_data(self):
+        # language=YAML


Strange.. what are those about? They don't feel so great here.

They cause my editor to highlight the string text as YAML rather than just a string, but we can get rid of them. I'll clean them up, as they aren't docs for the tests, which is what we're currently doing.

niemeyer · 2020-04-08T09:28:24Z

test/test_testing.py

+        harness.set_leader(False)
+        rel_id = harness.add_relation('db', 'postgresql')
+        harness.update_relation_data(rel_id, 'test-charm/0', {'foo': 'bar'})
+        harness.add_relation_unit(rel_id, 'postgresql/0')


Those interactions via the harness are great! So readable and so clear.

I agree that Harness probably shouldn't use get_backend_calls for Charm testing. For test_model.py something like this is very useful, because we're explicitly testing how our Model object interacts with our ModelBackend. (currently we do so via a fake_script indirection).

I'd happily make get_backend_calls private for our internal use, and not leave it as part of the public API.

niemeyer · 2020-04-08T09:29:27Z

test/test_testing.py

+        self.assertEqual([
+            ('relation_ids', 'db'),
+            ('relation_list', rel_id),
+        ], harness.get_backend_calls(reset=True))


This is the awkward bit.. what just happened there? How and why do we constrain the above logic to that list? Why does it matter?

Make get_backend_calls a private method, as it is useful for the TestModel tests, but should not be used by charmers interacting with Harness.

facundobatista

I'm all green now. Thanks!!!

* moved the code that mutates the system state to a separate type (DbInstanceManager); * moved events previously emitted by the charm to DbInstanceManager * this created a need for the charm code to subscribe the CockroachDbCluster type to the ClusterInitialized event from CockroachDbCharm and a subsequent change to how this event is unit tested in test_cluster.py; * dependency injection of TestDbInstanceManager is done via a class attribute which is far from ideal but given the charm type is initialized by the framework there is no direct way to control its arguments; * TestDbInstanceManager overrides the methods that modify the system state leaving the rest of the logic interesting for testing intact; 3 test cases need uncommenting either partially or fully when PR 196 gets merged: canonical/operator#196

* some unit tests use mocking because the interface code calls out to charm-helpers which uses hookenv. In order to avoid that we need to solve a larger problem of depending on charm-helpers and bypassing of the model and test harness provided by the framework. * there are 2 expected failures which are waiting on canonical/operator#196 to be merged.

Cleanup the API for the new expectations (using update_relation_data instead of a remote_app_data parameter).

gnuoy · 2020-04-24T13:20:25Z

I have tested this PR as a potential fix for issue #241 and it does indeed fix it.

This is #196 but without the _get_backend_calls. I'll probably bring that back later on, but for now this fixes a few key aspects of Harness. Namely: * fixes: #241 * We should not be raising KeyError for a relation that isn't established yet. It is normal for a charm to want to see if something is ready. * Allow relation_set to actually remove values (when set to '') which conforms to Juju's behavior remove language=YAML comments (they were considered noisy)

One 'advantage' of squash-and-merge is that it doesn't automatically roll back when you override a change. (Sequential branches are going to be a pain.) This leaves the _record_calls and _get_backend_calls around, prepared for when we use them in test_model.py

jameinel · 2020-05-06T07:39:32Z

Closing this for now, as it isn't making use of the _get_backend_calls (which are intended for test_model.py). I'll bring it back once that makes it clear how this helps.

jameinel added 2 commits March 31, 2020 13:12

Update comment of what is going on.

c1795c6

jameinel mentioned this pull request Mar 31, 2020

Interface pgsql #197

Closed

facundobatista suggested changes Mar 31, 2020

View reviewed changes

ops/testing.py Outdated Show resolved Hide resolved

ops/testing.py Show resolved Hide resolved

ops/testing.py Outdated Show resolved Hide resolved

ops/testing.py Outdated Show resolved Hide resolved

jameinel mentioned this pull request Apr 1, 2020

Testing hardness: _TestingModelBackend.relation_ids raises KeyError if a relation present in the charm meta has not been added via Harness.add_relation #202

Closed

dshcherb mentioned this pull request Apr 1, 2020

Return an empty list if not relation-ids exist yet #204

Closed

dshcherb reviewed Apr 1, 2020

View reviewed changes

ops/testing.py Outdated Show resolved Hide resolved

dshcherb pushed a commit to dshcherb/cockroachdb-operator-1 that referenced this pull request Apr 1, 2020

Add unit tests for the peer relation

95c61eb

This PR needs to go in canonical/operator#196 to avoid canonical/operator#202 when running tests.

jameinel commented Apr 6, 2020

View reviewed changes

ops/testing.py: instead of updating each method decorate the class.

b03918b

Use a record_calls decorator that creates the same output that we've been using, but does so without having to change each method, or ever forget about other args, etc.

niemeyer requested changes Apr 8, 2020

View reviewed changes

jameinel added 2 commits April 8, 2020 15:52

ops/testing.py:A Cleanup language=YAML lines.

d45aa94

Make get_backend_calls a private method, as it is useful for the TestModel tests, but should not be used by charmers interacting with Harness.

Stronger language for _TestingModelBackend.

fa74a53

facundobatista approved these changes Apr 8, 2020

View reviewed changes

dshcherb approved these changes Apr 13, 2020

View reviewed changes

dshcherb mentioned this pull request Apr 13, 2020

Add unit tests gnuoy/ops-interface-ceph-client#2

Merged

Merge upstream/master into test-harness-backend-calls

3d06514

Cleanup the API for the new expectations (using update_relation_data instead of a remote_app_data parameter).

This was referenced Apr 24, 2020

Stop test harness relation_ids raising KeyError #242

Closed

_TestingModelBackend incorrectly raises KeyError on missing relation #241

Closed

jameinel mentioned this pull request May 5, 2020

Test harness no backend #253

Merged

jameinel closed this May 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor out the Harness changes for test_model.py #196

Factor out the Harness changes for test_model.py #196

jameinel commented Mar 31, 2020

facundobatista left a comment

jameinel Apr 6, 2020

niemeyer left a comment

niemeyer Apr 8, 2020

niemeyer Apr 8, 2020

jameinel Apr 8, 2020

niemeyer Apr 8, 2020

niemeyer Apr 8, 2020

niemeyer Apr 8, 2020

niemeyer Apr 8, 2020

niemeyer Apr 8, 2020

jameinel Apr 8, 2020

niemeyer Apr 8, 2020

jameinel Apr 8, 2020

niemeyer Apr 8, 2020

facundobatista left a comment •

edited

gnuoy commented Apr 24, 2020

jameinel commented May 6, 2020



		def record_calls(cls):
		"""Replace methods on cls with methods that record that they have been called.

Factor out the Harness changes for test_model.py #196

Factor out the Harness changes for test_model.py #196

Conversation

jameinel commented Mar 31, 2020

facundobatista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facundobatista left a comment • edited

Choose a reason for hiding this comment

gnuoy commented Apr 24, 2020

jameinel commented May 6, 2020

facundobatista left a comment •

edited