chore: minor linting improvements to testing/src/scenario (canonical#…

…1456)

We're not aiming to get 100% in the TIOBE scoring, but it's nice to get
a higher score when it aligns with our existing style and doesn't
involve significant changes. The `testing` folder already scores 95.99
but there's still some low-hanging fruit, which this PR addresses:

* Some basic import reordering, which is mostly because "ops" is
first-party now.
* Clearing out outdated TODO/FIXME style comments (I've considered each
of these, and the ones removed either are out of date, are not something
we want to do now, or already have tickets).
* TIOBE's linter prefers multiple `if` statements rather than
if/elif/else when the block unconditionally returns/raises. This is a
debatable style choice, but where it seemed reasonable I've applied that
here. I don't think we have a team style decision on this yet.
* Added module, class, method docstrings.

testing/src/scenario/__init__.py

@@ -60,9 +60,10 @@ def test_base():
         assert out.unit_status == UnknownStatus()
 """
 
+from ops._private.harness import ActionFailed  # For backwards compatibility.
+
 from .context import CharmEvents, Context, Manager
 from .errors import StateValidationError  # For backwards compatibility.
-from ops._private.harness import ActionFailed  # For backwards compatibility.
 from .state import (
     ActiveStatus,
     Address,

testing/src/scenario/_consistency_checker.py

@@ -170,14 +170,16 @@ def check_event_consistency(
     errors: List[str] = []
     warnings: List[str] = []
 
-    # custom event: can't make assumptions about its name and its semantics
-    # todo: should we then just skip the other checks?
     if not event._is_builtin_event(charm_spec):
-        warnings.append(
-            "this is a custom event; if its name makes it look like a builtin one "
-            "(e.g. a relation event, or a workload event), you might get some false-negative "
-            "consistency checks.",
-        )
+        # This is a custom event - we can't make assumptions about its name and
+        # semantics. It doesn't really make sense to do checks that are designed
+        # for relations, workloads, and so on - most likely those will end up
+        # with false positives. Realistically, we can't know about what the
+        # requirements for the custom event are (in terms of the state), so we
+        # skip everything here. Perhaps in the future, custom events could
+        # optionally include some sort of state metadata that made testing
+        # consistency possible?
+        return Results(errors, warnings)
 
     if event._is_relation_event:
         _check_relation_event(charm_spec, event, state, errors, warnings)

testing/src/scenario/context.py

@@ -2,6 +2,12 @@
 # Copyright 2023 Canonical Ltd.
 # See LICENSE file for licensing details.
 
+"""Test Context
+
+The test `Context` object provides the context of the wider Juju system that the
+specific `State` exists in, and the events that can be executed on that `State`.
+"""
+
 from __future__ import annotations
 
 import functools
@@ -43,7 +49,6 @@
     from .ops_main_mock import Ops
     from .state import (
         AnyJson,
-        CharmType,
         JujuLogLine,
         RelationBase,
         State,
@@ -78,6 +83,7 @@ def __init__(
         self._state_in = state_in
 
         self._emitted: bool = False
+        self._wrapped_ctx = None
 
         self.ops: Ops | None = None
 
@@ -99,8 +105,7 @@ def _runner(self):
 
     def __enter__(self):
         self._wrapped_ctx = wrapped_ctx = self._runner(self._arg, self._state_in)
-        ops = wrapped_ctx.__enter__()
-        self.ops = ops
+        self.ops = wrapped_ctx.__enter__()
         return self
 
     def run(self) -> State:
@@ -113,6 +118,7 @@ def run(self) -> State:
         self._emitted = True
 
         # wrap up Runtime.exec() so that we can gather the output state
+        assert self._wrapped_ctx is not None
         self._wrapped_ctx.__exit__(None, None, None)
 
         assert self._ctx._output_state is not None
@@ -656,8 +662,8 @@ def run(self, event: _Event, state: State) -> State:
             if self.action_results is not None:
                 self.action_results.clear()
             self._action_failure_message = None
-        with self._run(event=event, state=state) as ops:
-            ops.emit()
+        with self._run(event=event, state=state) as manager:
+            manager.emit()
         # We know that the output state will have been set by this point,
         # so let the type checkers know that too.
         assert self._output_state is not None

testing/src/scenario/logger.py

@@ -2,6 +2,8 @@
 # Copyright 2023 Canonical Ltd.
 # See LICENSE file for licensing details.
 
+"""Test framework logger"""
+
 import logging
 import os
 

testing/src/scenario/mocking.py

@@ -2,6 +2,12 @@
 # Copyright 2023 Canonical Ltd.
 # See LICENSE file for licensing details.
 
+"""Juju and Pebble mocking
+
+This module contains mocks for the Juju and Pebble APIs that are used by ops
+to interact with the Juju controller and the Pebble service manager.
+"""
+
 import datetime
 import io
 import shutil
@@ -97,12 +103,14 @@ def _close_stdin(self):
             self._args.stdin = self.stdin.read()
 
     def wait(self):
+        """Wait for the (mock) process to finish."""
         self._close_stdin()
         self._waited = True
         if self._return_code != 0:
             raise ExecError(list(self._args.command), self._return_code, None, None)
 
     def wait_output(self):
+        """Wait for the (mock) process to finish and return tuple of (stdout, stderr)."""
         self._close_stdin()
         self._waited = True
         stdout = self.stdout.read() if self.stdout is not None else None
@@ -117,6 +125,7 @@ def wait_output(self):
         return stdout, stderr
 
     def send_signal(self, sig: Union[int, str]) -> NoReturn:  # noqa: U100
+        """Send the given signal to the (mock) process."""
         raise NotImplementedError()
 
 
@@ -150,8 +159,6 @@ def open_port(
         protocol: "_RawPortProtocolLiteral",
         port: Optional[int] = None,
     ):
-        # fixme: the charm will get hit with a StateValidationError
-        #  here, not the expected ModelError...
         port_ = _port_cls_by_protocol[protocol](port=port)  # type: ignore
         ports = set(self._state.opened_ports)
         if port_ not in ports:
@@ -203,10 +210,8 @@ def _get_relation_by_id(self, rel_id: int) -> "RelationBase":
             raise RelationNotFoundError() from None
 
     def _get_secret(self, id: Optional[str] = None, label: Optional[str] = None):
-        # FIXME: what error would a charm get IRL?
-        # ops 2.0 supports secrets, but juju only supports it from 3.0.2
         if self._context.juju_version < "3.0.2":
-            raise RuntimeError(
+            raise ModelError(
                 "secrets are only available in juju >= 3.0.2."
                 "Set ``Context.juju_version`` to 3.0.2+ to use them.",
             )
@@ -227,16 +232,15 @@ def _get_secret(self, id: Optional[str] = None, label: Optional[str] = None):
                 raise SecretNotFoundError(id)
             return secrets[0]
 
-        elif label:
+        if label:
             try:
                 return self._state.get_secret(label=label)
             except KeyError:
                 raise SecretNotFoundError(label) from None
 
-        else:
-            # if all goes well, this should never be reached. ops.model.Secret will check upon
-            # instantiation that either an id or a label are set, and raise a TypeError if not.
-            raise RuntimeError("need id or label.")
+        # if all goes well, this should never be reached. ops.model.Secret will check upon
+        # instantiation that either an id or a label are set, and raise a TypeError if not.
+        raise RuntimeError("need id or label.")
 
     def _check_app_data_access(self, is_app: bool):
         if not isinstance(is_app, bool):
@@ -256,14 +260,13 @@ def relation_get(self, relation_id: int, member_name: str, is_app: bool):
         relation = self._get_relation_by_id(relation_id)
         if is_app and member_name == self.app_name:
             return relation.local_app_data
-        elif is_app:
+        if is_app:
             if isinstance(relation, PeerRelation):
                 return relation.local_app_data
-            elif isinstance(relation, (Relation, SubordinateRelation)):
+            if isinstance(relation, (Relation, SubordinateRelation)):
                 return relation.remote_app_data
-            else:
-                raise TypeError("relation_get: unknown relation type")
-        elif member_name == self.unit_name:
+            raise TypeError("relation_get: unknown relation type")
+        if member_name == self.unit_name:
             return relation.local_unit_data
 
         unit_id = int(member_name.split("/")[-1])
@@ -386,7 +389,6 @@ def relation_set(self, relation_id: int, key: str, value: str, is_app: bool):
         else:
             tgt = relation.local_unit_data
         tgt[key] = value
-        return None
 
     def secret_add(
         self,
@@ -583,10 +585,9 @@ def relation_remote_app_name(
 
         if isinstance(relation, PeerRelation):
             return self.app_name
-        elif isinstance(relation, (Relation, SubordinateRelation)):
+        if isinstance(relation, (Relation, SubordinateRelation)):
             return relation.remote_app_name
-        else:
-            raise TypeError("relation_remote_app_name: unknown relation type")
+        raise TypeError("relation_remote_app_name: unknown relation type")
 
     def action_set(self, results: Dict[str, Any]):
         if not self._event.action:
@@ -705,7 +706,6 @@ def add_metrics(
             "it's deprecated API)",
         )
 
-    # TODO: It seems like this method has no tests.
     def resource_get(self, resource_name: str) -> str:
         # We assume that there are few enough resources that a linear search
         # will perform well enough.

testing/src/scenario/runtime.py

@@ -2,6 +2,8 @@
 # Copyright 2023 Canonical Ltd.
 # See LICENSE file for licensing details.
 
+"""Test framework runtime."""
+
 import copy
 import dataclasses
 import marshal
@@ -151,6 +153,10 @@ def __enter__(self):
         pass
 
     def emit(self):
+        """Emit the event.
+
+        Within the test framework, this only requires recording that it was emitted.
+        """
         self._has_emitted = True
 
     def __exit__(self, exc_type: Any, exc_val: Any, exc_tb: Any):  # noqa: U100
@@ -193,13 +199,11 @@ def _get_event_env(self, state: "State", event: "_Event", charm_root: Path):
             "JUJU_MODEL_NAME": state.model.name,
             "JUJU_MODEL_UUID": state.model.uuid,
             "JUJU_CHARM_DIR": str(charm_root.absolute()),
-            # todo consider setting pwd, (python)path
         }
 
         if event._is_action_event and (action := event.action):
             env.update(
                 {
-                    # TODO: we should check we're doing the right thing here.
                     "JUJU_ACTION_NAME": action.name.replace("_", "-"),
                     "JUJU_ACTION_UUID": action.id,
                 },
@@ -245,7 +249,7 @@ def _get_event_env(self, state: "State", event: "_Event", charm_root: Path):
                 else:
                     logger.warning(
                         "remote unit ID unset; no remote unit data present. "
-                        "Is this a realistic scenario?",  # TODO: is it?
+                        "Is this a realistic scenario?",
                     )
 
             if remote_unit_id is not None:
@@ -297,14 +301,17 @@ def _get_event_env(self, state: "State", event: "_Event", charm_root: Path):
     @staticmethod
     def _wrap(charm_type: Type["CharmType"]) -> Type["CharmType"]:
         # dark sorcery to work around framework using class attrs to hold on to event sources
-        # todo this should only be needed if we call play multiple times on the same runtime.
-        #  can we avoid it?
+        # this should only be needed if we call play multiple times on the same runtime.
         class WrappedEvents(charm_type.on.__class__):
+            """The charm's event sources, but wrapped."""
+
             pass
 
         WrappedEvents.__name__ = charm_type.on.__class__.__name__
 
         class WrappedCharm(charm_type):
+            """The test charm's type, but with events wrapped."""
+
             on = WrappedEvents()
 
         WrappedCharm.__name__ = charm_type.__name__
@@ -402,7 +409,6 @@ def _close_storage(self, state: "State", temporary_charm_root: Path):
     def _exec_ctx(self, ctx: "Context"):
         """python 3.8 compatibility shim"""
         with self._virtual_charm_root() as temporary_charm_root:
-            # TODO: allow customising capture_events
             with _capture_events(
                 include_deferred=ctx.capture_deferred_events,
                 include_framework=ctx.capture_framework_events,
@@ -424,9 +430,6 @@ def exec(
         This will set the environment up and call ops.main().
         After that it's up to ops.
         """
-        # todo consider forking out a real subprocess and do the mocking by
-        #  mocking hook tool executables
-
         from ._consistency_checker import check_consistency  # avoid cycles
 
         check_consistency(state, event, self._charm_spec, self._juju_version)