Repository: canonical/operator
Pull request #1602 (2025-03-03 01:15:14)

test: sort pytest params to guarantee identical ordering across workers
This PR updates the `scenario` tests to sort any `set` parameters so that the ordering is guaranteed to be the same every time. This is necessary because `pytest-xdist` will error if workers have different (or differently ordered) tests, because it [refers to tests by index](https://github.com/pytest-dev/pytest-xdist/issues/432#issuecomment-506535968).

> each worker performs a standard collection, and sends the collected test ids (in order) back to the master node. The master node ensures every worker collected the same number of tests and in the same order, because the scheduler will from that point on send just the test indexes (not the entire node id) to each worker to tell them which test to execute. That's why the collection must be the same across all workers.

This resolves failures like [this one](https://github.com/james-garner-canonical/operator/actions/runs/13620751544/job/38069841078).

```
_____________________________ ERROR collecting gw1 _____________________________
Different tests were collected between gw0 and gw1. The difference is:
--- gw0

+++ gw1

@@ -1246,13 +1246,13 @@

 testing/tests/test_consistency_checker.py::test_container_in_state_but_no_container_in_meta
 testing/tests/test_consistency_checker.py::test_container_not_in_state
 testing/tests/test_consistency_checker.py::test_evt_bad_container_name
+testing/tests/test_consistency_checker.py::test_evt_bad_relation_name[_relation_joined]
 testing/tests/test_consistency_checker.py::test_evt_bad_relation_name[_relation_changed]
-testing/tests/test_consistency_checker.py::test_evt_bad_relation_name[_relation_joined]
 testing/tests/test_consistency_checker.py::test_evt_bad_relation_name[_relation_created]
 testing/tests/test_consistency_checker.py::test_evt_bad_relation_name[_relation_departed]
 testing/tests/test_consistency_checker.py::test_evt_bad_relation_name[_relation_broken]
+testing/tests/test_consistency_checker.py::test_evt_no_relation[_relation_joined]
 testing/tests/test_consistency_checker.py::test_evt_no_relation[_relation_changed]
-testing/tests/test_consistency_checker.py::test_evt_no_relation[_relation_joined]
 testing/tests/test_consistency_checker.py::test_evt_no_relation[_relation_created]
 testing/tests/test_consistency_checker.py::test_evt_no_relation[_relation_departed]
 testing/tests/test_consistency_checker.py::test_evt_no_relation[_relation_broken]
To see why this happens see Known limitations in documentation
```


Patch:
diff --git a/ops/_private/harness.py b/ops/_private/harness.py
index 6e18bac..7814deb 100644
--- a/ops/_private/harness.py
+++ b/ops/_private/harness.py
@@ -3213,75 +3213,102 @@ class _TestingPebbleClient:
                 self._new_perform_check(info)
         return self.autostart_services(timeout, delay)
 
+    def _new_service_change(self, kind: pebble.ChangeKind, services: List[str]) -> pebble.Change:
+        now = datetime.datetime.now()
+        if kind == pebble.ChangeKind.START:
+            action = 'Start'
+        elif kind == pebble.ChangeKind.STOP:
+            action = 'Stop'
+        elif kind == pebble.ChangeKind.RESTART:
+            action = 'Restart'
+        else:
+            raise ValueError(f'unknown kind {kind}')
+        summary = f'{action} service {services[0]}'
+        if len(services) > 1:
+            summary += f' and {len(services) - 1} more'
+        change = pebble.Change(
+            pebble.ChangeID(str(uuid.uuid4())),
+            kind.value,
+            summary=summary,
+            status=pebble.ChangeStatus.DONE.value,
+            tasks=[],
+            ready=True,
+            err=None,
+            spawn_time=now,
+            ready_time=now,
+        )
+        self._changes[change.id] = change
+        return change
+
     def start_services(
         self,
         services: List[str],
         timeout: float = 30.0,
         delay: float = 0.1,
-    ):
+    ) -> pebble.ChangeID:
         # A common mistake is to pass just the name of a service, rather than a list of services,
         # so trap that so it is caught quickly.
         if isinstance(services, str):
             raise TypeError(f'start_services should take a list of names, not just "{services}"')
-
+        if not services:
+            raise self._api_error(400, 'must specify services for start action')
         self._check_connection()
 
-        # Note: jam 2021-04-20 We don't implement ChangeID, but the default caller of this is
-        # Container.start() which currently ignores the return value
         known_services = self._render_services()
         # Names appear to be validated before any are activated, so do two passes
         for name in services:
             if name not in known_services:
-                # TODO: jam 2021-04-20 This needs a better error type
-                raise RuntimeError(f'400 Bad Request: service "{name}" does not exist')
+                raise self._api_error(400, f'cannot start services: service {name} does not exist')
         for name in services:
             self._service_status[name] = pebble.ServiceStatus.ACTIVE
+        return self._new_service_change(pebble.ChangeKind.START, services).id
 
     def stop_services(
         self,
         services: List[str],
         timeout: float = 30.0,
         delay: float = 0.1,
-    ):
-        # handle a common mistake of passing just a name rather than a list of names
+    ) -> pebble.ChangeID:
+        # Handle a common mistake of passing just a name rather than a list of names.
         if isinstance(services, str):
             raise TypeError(f'stop_services should take a list of names, not just "{services}"')
+        if not services:
+            raise self._api_error(400, 'must specify services for start action')
 
         self._check_connection()
 
-        # Note: jam 2021-04-20 We don't implement ChangeID, but the default caller of this is
-        # Container.stop() which currently ignores the return value
         known_services = self._render_services()
         for name in services:
             if name not in known_services:
-                # TODO: jam 2021-04-20 This needs a better error type
-                #  400 Bad Request: service "bal" does not exist
-                raise RuntimeError(f'400 Bad Request: service "{name}" does not exist')
+                raise self._api_error(400, f'cannot stop services: service {name} does not exist')
         for name in services:
             self._service_status[name] = pebble.ServiceStatus.INACTIVE
+        return self._new_service_change(pebble.ChangeKind.STOP, services).id
 
     def restart_services(
         self,
         services: List[str],
         timeout: float = 30.0,
         delay: float = 0.1,
-    ):
-        # handle a common mistake of passing just a name rather than a list of names
+    ) -> pebble.ChangeID:
+        # Handle a common mistake of passing just a name rather than a list of names.
         if isinstance(services, str):
             raise TypeError(f'restart_services should take a list of names, not just "{services}"')
+        if not services:
+            raise self._api_error(400, 'must specify services for start action')
 
         self._check_connection()
 
-        # Note: jam 2021-04-20 We don't implement ChangeID, but the default caller of this is
-        # Container.restart() which currently ignores the return value
         known_services = self._render_services()
         for name in services:
             if name not in known_services:
-                # TODO: jam 2021-04-20 This needs a better error type
-                #  400 Bad Request: service "bal" does not exist
-                raise RuntimeError(f'400 Bad Request: service "{name}" does not exist')
+                raise self._api_error(
+                    400,
+                    f'cannot restart services: service {name} does not exist',
+                )
         for name in services:
             self._service_status[name] = pebble.ServiceStatus.ACTIVE
+        return self._new_service_change(pebble.ChangeKind.RESTART, services).id
 
     def wait_change(
         self,
diff --git a/ops/charm.py b/ops/charm.py
index 1318435..77fb0a8 100644
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -1451,6 +1451,17 @@ class CharmMeta:
     assumes: 'JujuAssumes'
     """Juju features this charm requires."""
 
+    charm_user: Literal['root', 'sudoer', 'non-root']
+    """Type of user used to run the charm hook code.
+
+    The value of ``root`` ensures the charm runs as root. The value of
+    ``sudoer`` runs the charm as a user other than root with access to sudo to
+    elevate its privileges. ``non-root`` ensures the charm does not run as root
+    and also does not have ``sudo`` privileges.
+
+    .. jujuadded 3.6.0
+    """
+
     containers: Dict[str, 'ContainerMeta']
     """Container metadata for each defined container."""
 
@@ -1524,6 +1535,7 @@ class CharmMeta:
         # Note that metadata v2 does not define min-juju-version ('assumes'
         # should be used instead).
         self.min_juju_version = raw_.get('min-juju-version')
+        self.charm_user = raw_.get('charm-user', 'root')
         self.requires = {
             name: RelationMeta(RelationRole.requires, name, rel)
             for name, rel in raw_.get('requires', {}).items()


Test patch:
diff --git a/test/test_charm.py b/test/test_charm.py
index 934f5c3..b2c9461 100644
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -1129,3 +1129,19 @@ assumes:
             ops.JujuAssumesCondition.ANY,
         ),
     ]
+
+
+@pytest.mark.parametrize('user', ['root', 'sudoer', 'non-root'])
+def test_meta_charm_user(user: str):
+    meta = ops.CharmMeta.from_yaml(f"""
+name: my-charm
+charm-user: {user}
+""")
+    assert meta.charm_user == user
+
+
+def test_meta_charm_user_default():
+    meta = ops.CharmMeta.from_yaml("""
+name: my-charm
+""")
+    assert meta.charm_user == 'root'
diff --git a/test/test_testing.py b/test/test_testing.py
index 4aa0f0b..9f370a2 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -4688,7 +4688,10 @@ class TestTestingPebbleClient:
                 command: '/bin/echo bar'
             """,
         )
-        client.start_services(['bar'])
+        change_id = client.start_services(['bar'])
+        change = client.get_change(change_id)
+        assert change.kind == pebble.ChangeKind.START.value
+        assert 'start' in change.summary.lower()
         infos = client.get_services()
         assert len(infos) == 2
         bar_info = infos[0]
@@ -4701,7 +4704,10 @@ class TestTestingPebbleClient:
         assert foo_info.name == 'foo'
         assert foo_info.startup == pebble.ServiceStartup.ENABLED
         assert foo_info.current == pebble.ServiceStatus.INACTIVE
-        client.stop_services(['bar'])
+        change_id = client.stop_services(['bar'])
+        change = client.get_change(change_id)
+        assert change.kind == pebble.ChangeKind.STOP.value
+        assert 'stop' in change.summary.lower()
         infos = client.get_services()
         bar_info = infos[0]
         assert bar_info.name == 'bar'
@@ -4772,8 +4778,7 @@ class TestTestingPebbleClient:
         assert infos == []
 
     def test_invalid_start_service(self, client: _TestingPebbleClient):
-        # TODO: jam 2021-04-20 This should become a better error
-        with pytest.raises(RuntimeError):
+        with pytest.raises(pebble.APIError):
             client.start_services(['unknown'])
 
     def test_start_service_str(self, client: _TestingPebbleClient):
@@ -4782,12 +4787,20 @@ class TestTestingPebbleClient:
         with pytest.raises(TypeError):
             client.start_services('unknown')
 
+    def test_start_service_no_services(self, client: _TestingPebbleClient):
+        with pytest.raises(pebble.APIError):
+            client.start_services([])
+
     def test_stop_service_str(self, client: _TestingPebbleClient):
         # Start service takes a list of names, but it is really easy to accidentally pass just a
         # name
         with pytest.raises(TypeError):
             client.stop_services('unknown')
 
+    def test_stop_service_no_services(self, client: _TestingPebbleClient):
+        with pytest.raises(pebble.APIError):
+            client.stop_services([])
+
     def test_mixed_start_service(self, client: _TestingPebbleClient):
         client.add_layer(
             'foo',
@@ -4800,8 +4813,7 @@ class TestTestingPebbleClient:
                 command: '/bin/echo foo'
             """,
         )
-        # TODO: jam 2021-04-20 better error type
-        with pytest.raises(RuntimeError):
+        with pytest.raises(pebble.APIError):
             client.start_services(['foo', 'unknown'])
         # foo should not be started
         infos = client.get_services()
@@ -4824,8 +4836,7 @@ class TestTestingPebbleClient:
             """,
         )
         client.autostart_services()
-        # TODO: jam 2021-04-20 better error type
-        with pytest.raises(RuntimeError):
+        with pytest.raises(pebble.APIError):
             client.stop_services(['foo', 'unknown'])
         # foo should still be running
         infos = client.get_services()
@@ -4901,6 +4912,20 @@ class TestTestingPebbleClient:
         assert foo_info.startup == pebble.ServiceStartup.ENABLED
         assert foo_info.current == pebble.ServiceStatus.INACTIVE
 
+    def test_invalid_restart_service(self, client: _TestingPebbleClient):
+        with pytest.raises(pebble.APIError):
+            client.restart_services(['unknown'])
+
+    def test_restart_service_str(self, client: _TestingPebbleClient):
+        # Restart service takes a list of names, but it is really easy to
+        # accidentally pass just a name.
+        with pytest.raises(TypeError):
+            client.restart_services('unknown')
+
+    def test_restart_service_no_services(self, client: _TestingPebbleClient):
+        with pytest.raises(pebble.APIError):
+            client.restart_services([])
+
     @unittest.skipUnless(is_linux, 'Pebble runs on Linux')
     def test_send_signal(self, client: _TestingPebbleClient):
         client.add_layer(
diff --git a/testing/tests/test_consistency_checker.py b/testing/tests/test_consistency_checker.py
index 8a73872..57837a8 100644
--- a/testing/tests/test_consistency_checker.py
+++ b/testing/tests/test_consistency_checker.py
@@ -180,8 +180,8 @@ def test_evt_bad_container_name():
     )
 
 
-@pytest.mark.parametrize("suffix", _RELATION_EVENTS_SUFFIX)
-def test_evt_bad_relation_name(suffix):
+@pytest.mark.parametrize("suffix", sorted(_RELATION_EVENTS_SUFFIX))
+def test_evt_bad_relation_name(suffix: str):
     assert_inconsistent(
         State(),
         _Event(f"foo{suffix}", relation=Relation("bar")),
@@ -195,8 +195,8 @@ def test_evt_bad_relation_name(suffix):
     )
 
 
-@pytest.mark.parametrize("suffix", _RELATION_EVENTS_SUFFIX)
-def test_evt_no_relation(suffix):
+@pytest.mark.parametrize("suffix", sorted(_RELATION_EVENTS_SUFFIX))
+def test_evt_no_relation(suffix: str):
     assert_inconsistent(State(), _Event(f"foo{suffix}"), _CharmSpec(MyCharm, {}))
     relation = Relation("bar")
     assert_consistent(