From 3d012cecff7c828076acb92909aeac88491c95c0 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 27 Nov 2024 14:50:17 +1300 Subject: [PATCH] chore: minor linting improvements to testing/src/scenario (#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 | 3 +- testing/src/scenario/_consistency_checker.py | 16 +++-- testing/src/scenario/context.py | 16 +++-- testing/src/scenario/logger.py | 2 + testing/src/scenario/mocking.py | 40 +++++------ testing/src/scenario/runtime.py | 21 +++--- testing/src/scenario/state.py | 71 ++++++++++---------- testing/tests/test_e2e/test_event.py | 46 ++++++------- 8 files changed, 113 insertions(+), 102 deletions(-) diff --git a/testing/src/scenario/__init__.py b/testing/src/scenario/__init__.py index b4dff01df..2cf042cc6 100644 --- a/testing/src/scenario/__init__.py +++ b/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, diff --git a/testing/src/scenario/_consistency_checker.py b/testing/src/scenario/_consistency_checker.py index 81df133d1..335c9c2ee 100644 --- a/testing/src/scenario/_consistency_checker.py +++ b/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) diff --git a/testing/src/scenario/context.py b/testing/src/scenario/context.py index ef0ffca9a..6b3ea2170 100644 --- a/testing/src/scenario/context.py +++ b/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 diff --git a/testing/src/scenario/logger.py b/testing/src/scenario/logger.py index daa272b52..a7909f397 100644 --- a/testing/src/scenario/logger.py +++ b/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 diff --git a/testing/src/scenario/mocking.py b/testing/src/scenario/mocking.py index 903886069..b95b553be 100644 --- a/testing/src/scenario/mocking.py +++ b/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. diff --git a/testing/src/scenario/runtime.py b/testing/src/scenario/runtime.py index 63a7d937a..3ad2fd0a2 100644 --- a/testing/src/scenario/runtime.py +++ b/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) diff --git a/testing/src/scenario/state.py b/testing/src/scenario/state.py index 29489ba3f..9d192e928 100644 --- a/testing/src/scenario/state.py +++ b/testing/src/scenario/state.py @@ -35,8 +35,9 @@ ) from uuid import uuid4 -import ops import yaml + +import ops from ops import pebble, CharmBase, CharmEvents, SecretRotate, StatusBase from ops import CloudCredential as CloudCredential_Ops from ops import CloudSpec as CloudSpec_Ops @@ -152,7 +153,7 @@ def __new__(cls, *args: Any, **kwargs: Any): # Also check if there are just not enough arguments at all, because # the default TypeError message will incorrectly describe some of # the arguments as positional. - elif n_posargs < len(required_args): + if n_posargs < len(required_args): required_pos = [ f"'{arg}'" for arg in required_args[n_posargs:] @@ -355,9 +356,6 @@ def _update_metadata( """Update the metadata.""" # bypass frozen dataclass object.__setattr__(self, "_latest_revision", self._latest_revision + 1) - # TODO: if this is done twice in the same hook, then Juju ignores the - # first call, it doesn't continue to update like this does. - # Fix when https://github.com/canonical/operator/issues/1288 is resolved. if content: object.__setattr__(self, "latest_content", content) if label: @@ -410,8 +408,7 @@ class BindAddress(_max_posargs(1)): """The MAC address of the interface.""" def _hook_tool_output_fmt(self): - # dumps itself to dict in the same format the hook tool would - # todo support for legacy (deprecated) `interfacename` and `macaddress` fields? + """Dumps itself to dict in the same format the hook tool would.""" dct = { "interface-name": self.interface_name, "addresses": [dataclasses.asdict(addr) for addr in self.addresses], @@ -472,6 +469,8 @@ def _next_relation_id(*, update: bool = True): @dataclasses.dataclass(frozen=True) class RelationBase(_max_posargs(2)): + """Base class for the various types of integration (relation).""" + endpoint: str """Relation endpoint name. Must match some endpoint name defined in the metadata.""" @@ -687,7 +686,6 @@ class Model(_max_posargs(1)): """A unique identifier for the model, typically generated by Juju.""" # whatever juju models --format=json | jq '.models[].type' gives back. - # TODO: make this exhaustive. type: Literal["kubernetes", "lxd"] = "kubernetes" """The type of Juju model.""" @@ -695,10 +693,6 @@ class Model(_max_posargs(1)): """Cloud specification information (metadata) including credentials.""" -# for now, proc mock allows you to map one command to one mocked output. -# todo extend: one input -> multiple outputs, at different times - - _CHANGE_IDS = 0 @@ -1072,12 +1066,14 @@ def from_status_name( name: _RawStatusLiteral, message: str = "", ) -> _EntityStatus: + """Convert the status name, such as 'active', to the class, such as ActiveStatus.""" # Note that this won't work for UnknownStatus. # All subclasses have a default 'name' attribute, but the type checker can't tell that. return cls._entity_statuses[name](message=message) # type:ignore @classmethod def from_ops(cls, obj: StatusBase) -> _EntityStatus: + """Convert from the ops.StatusBase object to the matching _EntityStatus object.""" return cls.from_status_name(obj.name, obj.message) @@ -1692,18 +1688,19 @@ class DeferredEvent: # them because they need a Handle. @property def name(self): + """A comparable name for the event.""" return self.handle_path.split("/")[-1].split("[")[0] class _EventType(str, Enum): - framework = "framework" - builtin = "builtin" - relation = "relation" - action = "action" - secret = "secret" - storage = "storage" - workload = "workload" - custom = "custom" + FRAMEWORK = "framework" + BUILTIN = "builtin" + RELATION = "relation" + ACTION = "action" + SECRET = "secret" + STORAGE = "storage" + WORKLOAD = "workload" + CUSTOM = "custom" class _EventPath(str): @@ -1737,36 +1734,36 @@ def __new__(cls, string: str): def _get_suffix_and_type(s: str) -> tuple[str, _EventType]: for suffix in _RELATION_EVENTS_SUFFIX: if s.endswith(suffix): - return suffix, _EventType.relation + return suffix, _EventType.RELATION if s.endswith(_ACTION_EVENT_SUFFIX): - return _ACTION_EVENT_SUFFIX, _EventType.action + return _ACTION_EVENT_SUFFIX, _EventType.ACTION if s in _SECRET_EVENTS: - return s, _EventType.secret + return s, _EventType.SECRET if s in _FRAMEWORK_EVENTS: - return s, _EventType.framework + return s, _EventType.FRAMEWORK # Whether the event name indicates that this is a storage event. for suffix in _STORAGE_EVENTS_SUFFIX: if s.endswith(suffix): - return suffix, _EventType.storage + return suffix, _EventType.STORAGE # Whether the event name indicates that this is a workload event. if s.endswith(_PEBBLE_READY_EVENT_SUFFIX): - return _PEBBLE_READY_EVENT_SUFFIX, _EventType.workload + return _PEBBLE_READY_EVENT_SUFFIX, _EventType.WORKLOAD if s.endswith(_PEBBLE_CUSTOM_NOTICE_EVENT_SUFFIX): - return _PEBBLE_CUSTOM_NOTICE_EVENT_SUFFIX, _EventType.workload + return _PEBBLE_CUSTOM_NOTICE_EVENT_SUFFIX, _EventType.WORKLOAD if s.endswith(_PEBBLE_CHECK_FAILED_EVENT_SUFFIX): - return _PEBBLE_CHECK_FAILED_EVENT_SUFFIX, _EventType.workload + return _PEBBLE_CHECK_FAILED_EVENT_SUFFIX, _EventType.WORKLOAD if s.endswith(_PEBBLE_CHECK_RECOVERED_EVENT_SUFFIX): - return _PEBBLE_CHECK_RECOVERED_EVENT_SUFFIX, _EventType.workload + return _PEBBLE_CHECK_RECOVERED_EVENT_SUFFIX, _EventType.WORKLOAD if s in _BUILTIN_EVENTS: - return "", _EventType.builtin + return "", _EventType.BUILTIN - return "", _EventType.custom + return "", _EventType.CUSTOM @dataclasses.dataclass(frozen=True) @@ -1843,27 +1840,27 @@ def owner_path(self) -> list[str]: @property def _is_relation_event(self) -> bool: """Whether the event name indicates that this is a relation event.""" - return self._path.type is _EventType.relation + return self._path.type is _EventType.RELATION @property def _is_action_event(self) -> bool: """Whether the event name indicates that this is a relation event.""" - return self._path.type is _EventType.action + return self._path.type is _EventType.ACTION @property def _is_secret_event(self) -> bool: """Whether the event name indicates that this is a secret event.""" - return self._path.type is _EventType.secret + return self._path.type is _EventType.SECRET @property def _is_storage_event(self) -> bool: """Whether the event name indicates that this is a storage event.""" - return self._path.type is _EventType.storage + return self._path.type is _EventType.STORAGE @property def _is_workload_event(self) -> bool: """Whether the event name indicates that this is a workload event.""" - return self._path.type is _EventType.workload + return self._path.type is _EventType.WORKLOAD # this method is private because _CharmSpec is not quite user-facing; also, # the user should know. @@ -1884,7 +1881,7 @@ def _is_builtin_event(self, charm_spec: _CharmSpec[CharmType]) -> bool: # However, our Event data structure ATM has no knowledge of which Object/Handle it is # owned by. So the only thing we can do right now is: check whether the event name, # assuming it is owned by the charm, LOOKS LIKE that of a builtin event or not. - return self._path.type is not _EventType.custom + return self._path.type is not _EventType.CUSTOM def deferred(self, handler: Callable[..., Any], event_id: int = 1) -> DeferredEvent: """Construct a DeferredEvent from this Event.""" diff --git a/testing/tests/test_e2e/test_event.py b/testing/tests/test_e2e/test_event.py index 2efc7285f..3151b4929 100644 --- a/testing/tests/test_e2e/test_event.py +++ b/testing/tests/test_e2e/test_event.py @@ -9,34 +9,34 @@ @pytest.mark.parametrize( "evt, expected_type", ( - ("foo_relation_changed", _EventType.relation), - ("foo_relation_created", _EventType.relation), - ("foo_bar_baz_relation_created", _EventType.relation), - ("foo_storage_attached", _EventType.storage), - ("foo_storage_detaching", _EventType.storage), - ("foo_bar_baz_storage_detaching", _EventType.storage), - ("foo_pebble_ready", _EventType.workload), - ("foo_bar_baz_pebble_ready", _EventType.workload), - ("foo_pebble_custom_notice", _EventType.workload), - ("foo_bar_baz_pebble_custom_notice", _EventType.workload), - ("secret_remove", _EventType.secret), - ("pre_commit", _EventType.framework), - ("commit", _EventType.framework), - ("collect_unit_status", _EventType.framework), - ("collect_app_status", _EventType.framework), - ("foo", _EventType.custom), - ("kaboozle_bar_baz", _EventType.custom), + ("foo_relation_changed", _EventType.RELATION), + ("foo_relation_created", _EventType.RELATION), + ("foo_bar_baz_relation_created", _EventType.RELATION), + ("foo_storage_attached", _EventType.STORAGE), + ("foo_storage_detaching", _EventType.STORAGE), + ("foo_bar_baz_storage_detaching", _EventType.STORAGE), + ("foo_pebble_ready", _EventType.WORKLOAD), + ("foo_bar_baz_pebble_ready", _EventType.WORKLOAD), + ("foo_pebble_custom_notice", _EventType.WORKLOAD), + ("foo_bar_baz_pebble_custom_notice", _EventType.WORKLOAD), + ("secret_remove", _EventType.SECRET), + ("pre_commit", _EventType.FRAMEWORK), + ("commit", _EventType.FRAMEWORK), + ("collect_unit_status", _EventType.FRAMEWORK), + ("collect_app_status", _EventType.FRAMEWORK), + ("foo", _EventType.CUSTOM), + ("kaboozle_bar_baz", _EventType.CUSTOM), ), ) def test_event_type(evt, expected_type): event = _Event(evt) assert event._path.type is expected_type - assert event._is_relation_event is (expected_type is _EventType.relation) - assert event._is_storage_event is (expected_type is _EventType.storage) - assert event._is_workload_event is (expected_type is _EventType.workload) - assert event._is_secret_event is (expected_type is _EventType.secret) - assert event._is_action_event is (expected_type is _EventType.action) + assert event._is_relation_event is (expected_type is _EventType.RELATION) + assert event._is_storage_event is (expected_type is _EventType.STORAGE) + assert event._is_workload_event is (expected_type is _EventType.WORKLOAD) + assert event._is_secret_event is (expected_type is _EventType.SECRET) + assert event._is_action_event is (expected_type is _EventType.ACTION) class MyCharm(CharmBase): pass @@ -55,7 +55,7 @@ class MyCharm(CharmBase): "containers": {"foo": {}, "foo_bar_baz": {}}, }, ) - assert event._is_builtin_event(spec) is (expected_type is not _EventType.custom) + assert event._is_builtin_event(spec) is (expected_type is not _EventType.CUSTOM) def test_emitted_framework():