Repository: canonical/operator (language: python)

Issue:
Simplify adding relation units and data in test harness
When using `testing.Harness`, it's *very* common to see these two or three calls in a row: add relation, (often) add relation unit, and (often) update relation data. It [came up recently](https://github.com/tim-hm/ratings/blob/38e379881e998c97932ee4260e70ec0a7deedf1c/operator/tests/unit/test_charm.py#L34C1-L36) in a simple charm @jnsgruk was writing, and there are many more examples. Here are just a few from a quick grep of the charms I have checked out locally:

```python
# alertmanager-k8s-operator/tests/unit/test_charm.py
self.relation_id = self.harness.add_relation("alerting", "otherapp")
self.harness.add_relation_unit(self.relation_id, "otherapp/0")

# grafana-k8s-operator/tests/unit/test_auth_proxy_provider.py (slightly modified)
relation_id = self.harness.add_relation("grafana-auth", "requirer")
urls = {"urls": json.dumps(EXAMPLE_URLS)}
self.harness.update_relation_data(relation_id, "requirer", urls)

# prometheus-k8s-operator/tests/unit/test_endpoint_aggregator.py
alert_rules_rel_id = self.harness.add_relation(ALERT_RULES_RELATION, "rules-app")
self.harness.add_relation_unit(alert_rules_rel_id, "rules-app/0")
self.harness.update_relation_data(
    alert_rules_rel_id, "rules-app/0", {"groups": ALERT_RULE_1}
)
```

Even our API reference docs have examples with this pattern in it, for example the [`begin_with_initial_hooks` docstring](https://ops.readthedocs.io/en/latest/#ops.testing.Harness.begin_with_initial_hooks).

This is a bit annoying and certainly verbose for authors of charm unit tests.

I propose we add a simplified API that rolls these two or three calls into one. This is somewhat similar to how in Python 3.5 they added [`subprocess.run`](https://docs.python.org/3/library/subprocess.html#subprocess.run) to simplify a number of common patterns and reduce verbosity.

There are various ways we could approach this, but I think the simplest and most discoverable would be to augment `add_relation` with two new optional keyword arguments: `unit` and `data`. If you specified a `unit`, it would call `add_relation_unit` after adding the relation, and if you specified `data` it would also call `update_relation_data`. The new signature of `add_relation` and the (trivial) implementation would be:

```python
def add_relation(self, relation_name: str, remote_app: str,
                 unit: Optional[str] = None,
                 data: Optional[Mapping[str, str]] = None) -> int:
    # ... existing add_relation implementation ...
    if unit is not None:
        self.add_relation_unit(relation_id, unit)
    if data is not None:
        self.update_relation_data(relation_id, unit or remote_app, data)
    return relation_id
```

The charm test examples from the top would be simplified to:

```python
# alertmanager-k8s-operator/tests/unit/test_charm.py
self.relation_id = self.harness.add_relation("alerting", "otherapp", unit="otherapp/0")

# grafana-k8s-operator/tests/unit/test_auth_proxy_provider.py
urls = {"urls": json.dumps(EXAMPLE_URLS)}
self.harness.add_relation("grafana-auth", "requirer", data=urls)

# prometheus-k8s-operator/tests/unit/test_endpoint_aggregator.py
self.harness.add_relation(ALERT_RULES_RELATION, "rules-app", unit="rules-app/0",
                          data={"groups": ALERT_RULE_1})
```


Discussion / hints:
Yes please
Firstly - love this!

I wonder about the necessity of making the `unit` argument so explicit - perhaps it would make more sense to have the `unit` argument implied, but only if `data` is also supplied?

So for example:

```python
self.harness.add_relation("grafana-auth", "requirer", data={"foo": "bar"})
```

Is actually equivalent to:

```python
self.harness.add_relation("grafana-auth", "requirer", data={"foo": "bar"}, unit="requirer/0")
```

Unless the author decides to override the `unit` arg themselves?
@jnsgruk Those two are actually different: the first is adding relation data to the *application* data bag, the second (with the `unit`) parameter is adding to the *unit* data bag. So the `unit` parameter in my proposal both sets the unit name and selects to set unit data rather the app data.

@jameinel and I were discussing this this morning. He's not opposed to this, but noted previous discussion, which I'll try to summarize here: adding a `data` argument to `Harness.add_relation` has been considered before, but was seen as "yet another way of doing things", and keeping them separate allows fine-grained control over when the various events are fired (eg: `relation_joined` is fired when you call `add_relation_unit`). Read a bit more in [this comment](https://github.com/canonical/operator/issues/794#issuecomment-1177336832).

Just because it's been considered before doesn't mean it's out. As I've shown above, we're making the very common patterns (used in almost all Harness tests) difficult for people, which pushes them to 1) be annoyed with Harness, 2) create their own helper functions, or 3) use other tools like ops-scenario. There's nothing wrong with using other tools if they're better! But we also want to make the official tools something people like to use!

John also noted that it's possible in the Juju model to have application data without a unit, but not normal: it only occurs if you add a relation and unit(s) and application data, and then scale down to zero units. But in charm tests we often ignore the fact that's it's not typical and just take a shortcut: call `add_relation` followed directly by `update_relation_data` (with no `add_relation_unit`).

We could also consider a slightly more explicit API that would always call `add_relation_unit` to fire `relation_joined`, which is more how a real Juju model works. For example:

```python
# if app_data provided, call add_relation, generate a unit name and call add_relation_unit,
#    and call update_relation_data to set app data
# if unit_data provided, call add_relation, generate a unit name and call add_relation_unit,
#    and call update_relation_data to set unit data
# it would be an error to provide both app_data and unit_data
def add_relation(self, relation_name: str, remote_app: str, *, app_data=None, unit_data=None) -> int:
    ...
```

Oh I do like that last suggestions with `app_data` and `unit_data` both explicitly in the signature :)
John and I discussed the above today -- indeed the proposed API looks good but we both agree that it doesn't necessarily need to  be an error to provide both unit and app data?
I like where we ended up. As Jon noted, we can allow them to set both, because there isn't ambiguity. The only thing that I hesitate at all is that creating the unit is based on whether you supply app_data, but that is a compromise I can live with for backward compatibility. (if we just made (`add_relation(relation, app)` always create a unit it would break any test suite that was then calling add_unit with the same name, and probably some other ones that check how many units there are.)
> only thing that I hesitate at all is that creating the unit is based on whether you supply app_data

@jameinel Do you mean "whether you supply app_data or unit_data", as surely we should call `add_relation_unit` if they supply unit_data too?

So this is the updated proposal:

```python
    def add_relation(self, relation_name: str, remote_app: str, *,
                     app_data: Optional[Mapping[str, str]] = None,
                     unit_data: Optional[Mapping[str, str]] = None) -> int:
        """Declare that there is a new relation between this app and `remote_app`.

        ... existing docstring ...

        If app_data or unit_data are provided, also add a new unit to the
        relation and trigger ``relation_joined``. Then update the application
        data if ``app_data`` is set and unit data if ``unit_data`` is set, triggering
        ``relation_changed`` after each update.
        """
        # ... existing add_relation implementation ...

        if app_data is not None or unit_data is not None:
            remote_unit = remote_app + '/0'  # or possibly a small random number?
            self.add_relation_unit(relation_id, remote_unit)
            if app_data is not None:
                self.update_relation_data(relation_id, remote_app, app_data)
            if unit_data is not None:
                self.update_relation_data(relation_id, remote_unit, unit_data)

        return relation_id
```

Any further comments?
Correct. We change behavior if you supply the new arguments. My point was that I don't really prefer adding a unit because you supplied app_data, (unit_data is rather obvious that you need a unit), but I also don't like just having app data without a unit.


Fix patch:
diff --git a/README.md b/README.md
index 8fa10ef1d..b8cd27a96 100644
--- a/README.md
+++ b/README.md
@@ -56,15 +56,21 @@ a full deployment. Our [API documentation](https://ops.readthedocs.io/en/latest/
 has the details, including this example:
 
 ```python
-harness = Harness(MyCharm)
-# Do initial setup here
-relation_id = harness.add_relation('db', 'postgresql')
-# Now instantiate the charm to see events as the model changes
-harness.begin()
-harness.add_relation_unit(relation_id, 'postgresql/0')
-harness.update_relation_data(relation_id, 'postgresql/0', {'key': 'val'})
-# Check that charm has properly handled the relation_joined event for postgresql/0
-self.assertEqual(harness.charm. ...)
+class TestCharm(unittest.TestCase):
+    def test_foo(self):
+        harness = Harness(MyCharm)
+        self.addCleanup(harness.cleanup)  # always clean up after ourselves
+
+        # Instantiate the charm and trigger events that Juju would on startup
+        harness.begin_with_initial_hooks()
+
+        # Update charm config and trigger config-changed
+        harness.update_config({'log_level': 'warn'})
+
+        # Check that charm properly handled config-changed, for example,
+        # the charm added the correct Pebble layer
+        plan = harness.get_container_pebble_plan('prometheus')
+        self.assertIn('--log.level=warn', plan.services['prometheus'].command)
 ```
 
 


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index 4247d053e..970d86099 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -149,23 +149,49 @@ class ExecResult:
 class Harness(Generic[CharmType]):
     """This class represents a way to build up the model that will drive a test suite.
 
-    The model that is created is from the viewpoint of the charm that you are testing.
+    The model created is from the viewpoint of the charm that you are testing.
 
-    Example::
+    Below is an example test using :meth:`begin_with_initial_hooks` that ensures
+    the charm responds correctly to config changes::
 
-        harness = Harness(MyCharm)
-        self.addCleanup(harness.cleanup)  # always clean up after ourselves
+        class TestCharm(unittest.TestCase):
+            def test_foo(self):
+                harness = Harness(MyCharm)
+                self.addCleanup(harness.cleanup)  # always clean up after ourselves
 
-        # Do initial setup here
-        relation_id = harness.add_relation('db', 'postgresql')
+                # Instantiate the charm and trigger events that Juju would on startup
+                harness.begin_with_initial_hooks()
 
-        # Now instantiate the charm to see events as the model changes
-        harness.begin()
-        harness.add_relation_unit(relation_id, 'postgresql/0')
-        harness.update_relation_data(relation_id, 'postgresql/0', {'key': 'val'})
+                # Update charm config and trigger config-changed
+                harness.update_config({'log_level': 'warn'})
 
-        # Check that charm has properly handled the relation_joined event for postgresql/0
-        self.assertEqual(harness.charm. ...)
+                # Check that charm properly handled config-changed, for example,
+                # the charm added the correct Pebble layer
+                plan = harness.get_container_pebble_plan('prometheus')
+                self.assertIn('--log.level=warn', plan.services['prometheus'].command)
+
+    To set up the model without triggering events (or calling charm code), perform the
+    harness actions before calling :meth:`begin`. Below is an example that adds a
+    relation before calling ``begin``, and then updates config to trigger the
+    ``config-changed`` event in the charm::
+
+        class TestCharm(unittest.TestCase):
+            def test_bar(self):
+                harness = Harness(MyCharm)
+                self.addCleanup(harness.cleanup)  # always clean up after ourselves
+
+                # Set up model before "begin" (no events triggered)
+                harness.set_leader(True)
+                harness.add_relation('db', 'postgresql', unit_data={'key': 'val'})
+
+                # Now instantiate the charm to start triggering events as the model changes
+                harness.begin()
+                harness.update_config({'some': 'config'})
+
+                # Check that charm has properly handled config-changed, for example,
+                # has written the app's config file
+                root = harness.get_filesystem_root('container')
+                assert (root / 'etc' / 'app.conf').exists()
 
     Args:
         charm_cls: The Charm class that you'll be testing.
@@ -336,9 +362,7 @@ def begin_with_initial_hooks(self) -> None:
             # Add storage if needed before begin_with_initial_hooks() is called
             storage_ids = harness.add_storage('data', count=1)[0]
             storage_id = storage_id[0] # we only added one storage instance
-            relation_id = harness.add_relation('db', 'postgresql')
-            harness.add_relation_unit(relation_id, 'postgresql/0')
-            harness.update_relation_data(relation_id, 'postgresql/0', {'key': 'val'})
+            harness.add_relation('db', 'postgresql', unit_data={'key': 'val'})
             harness.set_leader(True)
             harness.update_config({'initial': 'config'})
             harness.begin_with_initial_hooks()
@@ -737,20 +761,45 @@ def remove_storage(self, storage_id: str) -> None:
                 model.Storage(storage_name, storage_index, self._backend))
         self._backend._storage_remove(storage_id)
 
-    def add_relation(self, relation_name: str, remote_app: str) -> int:
-        """Declare that there is a new relation between this app and `remote_app`.
+    def add_relation(self, relation_name: str, remote_app: str, *,
+                     app_data: Optional[Mapping[str, str]] = None,
+                     unit_data: Optional[Mapping[str, str]] = None) -> int:
+        """Declare that there is a new relation between this application and `remote_app`.
 
-        In the case of adding peer relations, `remote_app` is *this* app.  This function creates a
-        relation with an application and will trigger a relation-created hook. To relate units (and
-        trigger relation-joined and relation-changed hooks), you should also call
-        :meth:`.add_relation_unit`.
+        This function creates a relation with an application and triggers a
+        :class:`RelationCreatedEvent <ops.RelationCreatedEvent>`.
+
+        If `app_data` or `unit_data` are provided, also add a new unit
+        (``<remote_app>/0``) to the relation and trigger
+        :class:`RelationJoinedEvent <ops.RelationJoinedEvent>`. Then update
+        the application data if `app_data` is provided and the unit data if
+        `unit_data` is provided, triggering
+        :class:`RelationChangedEvent <ops.RelationChangedEvent>` after each update.
+        Alternatively, charm tests can call :meth:`add_relation_unit` and
+        :meth:`update_relation_data` explicitly.
+
+        Example usage::
+
+            secret_id = harness.add_model_secret('mysql', {'password': 'SECRET'})
+            harness.add_relation('db', 'mysql', unit_data={
+                'host': 'mysql.localhost,
+                'username': 'appuser',
+                'secret-id': secret_id,
+            })
 
         Args:
-            relation_name: The relation on Charm that is being related to
-            remote_app: The name of the application that is being related to
+            relation_name: The relation on the charm that is being related to.
+            remote_app: The name of the application that is being related to.
+                To add a peer relation, set to the name of *this* application.
+            app_data: If provided, also add a new unit to the relation
+                (triggering relation-joined) and set the *application* relation data
+                (triggering relation-changed).
+            unit_data: If provided, also add a new unit to the relation
+                (triggering relation-joined) and set the *unit* relation data
+                (triggering relation-changed).
 
         Return:
-            The relation_id created by this add_relation.
+            The ID of the relation created.
         """
         relation_id = self._next_relation_id()
         self._backend._relation_ids_map.setdefault(
@@ -770,6 +819,15 @@ def add_relation(self, relation_name: str, remote_app: str) -> int:
         if self._model is not None:
             self._model.relations._invalidate(relation_name)
         self._emit_relation_created(relation_name, relation_id, remote_app)
+
+        if app_data is not None or unit_data is not None:
+            remote_unit = remote_app + '/0'
+            self.add_relation_unit(relation_id, remote_unit)
+            if app_data is not None:
+                self.update_relation_data(relation_id, remote_app, app_data)
+            if unit_data is not None:
+                self.update_relation_data(relation_id, remote_unit, unit_data)
+
         return relation_id
 
     def remove_relation(self, relation_id: int) -> None:
@@ -830,17 +888,21 @@ def _emit_relation_broken(self, relation_name: str, relation_id: int,
     def add_relation_unit(self, relation_id: int, remote_unit_name: str) -> None:
         """Add a new unit to a relation.
 
-        Example::
-
-          rel_id = harness.add_relation('db', 'postgresql')
-          harness.add_relation_unit(rel_id, 'postgresql/0')
-
         This will trigger a `relation_joined` event. This would naturally be
         followed by a `relation_changed` event, which you can trigger with
         :meth:`.update_relation_data`. This separation is artificial in the
         sense that Juju will always fire the two, but is intended to make
         testing relations and their data bags slightly more natural.
 
+        Unless finer-grained control is needed, most charm tests can call
+        :meth:`add_relation` with the `app_data` or `unit_data` argument
+        instead of using this function.
+
+        Example::
+
+          rel_id = harness.add_relation('db', 'postgresql')
+          harness.add_relation_unit(rel_id, 'postgresql/0')
+
         Args:
             relation_id: The integer relation identifier (as returned by :meth:`add_relation`).
             remote_unit_name: A string representing the remote unit that is being added.
@@ -1058,6 +1120,10 @@ def update_relation_data(
 
         This also triggers the `relation_changed` event for the given ``relation_id``.
 
+        Unless finer-grained control is needed, most charm tests can call
+        :meth:`add_relation` with the `app_data` or `unit_data` argument
+        instead of using this function.
+
         Args:
             relation_id: The integer relation ID representing this relation.
             app_or_unit: The unit or application name that is being updated.
diff --git a/test/test_testing.py b/test/test_testing.py
index b37d8590d..df24292e0 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -108,6 +108,38 @@ def test_add_relation(self):
         self.assertEqual(backend.relation_get(rel_id, 'test-app', is_app=True), {})
         self.assertEqual(backend.relation_get(rel_id, 'test-app/0', is_app=False), {})
 
+    def test_add_relation_with_app_data(self):
+        harness = ops.testing.Harness(ops.CharmBase, meta='''
+            name: test-app
+            requires:
+                db:
+                    interface: pgsql
+            ''')
+        self.addCleanup(harness.cleanup)
+        rel_id = harness.add_relation('db', 'postgresql', app_data={'x': '1', 'y': '2'})
+        self.assertIsInstance(rel_id, int)
+        backend = harness._backend
+        self.assertEqual(backend.relation_ids('db'), [rel_id])
+        self.assertEqual(backend.relation_list(rel_id), ['postgresql/0'])
+        self.assertEqual(harness.get_relation_data(rel_id, 'postgresql'), {'x': '1', 'y': '2'})
+        self.assertEqual(harness.get_relation_data(rel_id, 'postgresql/0'), {})
+
+    def test_add_relation_with_unit_data(self):
+        harness = ops.testing.Harness(ops.CharmBase, meta='''
+            name: test-app
+            requires:
+                db:
+                    interface: pgsql
+            ''')
+        self.addCleanup(harness.cleanup)
+        rel_id = harness.add_relation('db', 'postgresql', unit_data={'a': '1', 'b': '2'})
+        self.assertIsInstance(rel_id, int)
+        backend = harness._backend
+        self.assertEqual(backend.relation_ids('db'), [rel_id])
+        self.assertEqual(backend.relation_list(rel_id), ['postgresql/0'])
+        self.assertEqual(harness.get_relation_data(rel_id, 'postgresql'), {})
+        self.assertEqual(harness.get_relation_data(rel_id, 'postgresql/0'), {'a': '1', 'b': '2'})
+
     def test_can_connect_default(self):
         harness = ops.testing.Harness(ops.CharmBase, meta='''
             name: test-app