From 15a92b24702fa88604f3d9cb842a800986e3c0e2 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 24 Jul 2024 17:20:51 +1200 Subject: [PATCH 1/2] Make action events more like other events. (#161) --- .gitignore | 2 +- README.md | 11 +++++---- docs/custom_conf.py | 1 + scenario/__init__.py | 2 -- scenario/consistency_checker.py | 4 ++-- scenario/context.py | 40 ++++++++++++++++++------------- scenario/state.py | 15 +++++------- tests/test_consistency_checker.py | 26 ++++++++++---------- tests/test_context.py | 23 ++++++++---------- tests/test_context_on.py | 11 ++++----- tests/test_e2e/test_actions.py | 34 ++++++++------------------ tests/test_e2e/test_manager.py | 4 ++-- 12 files changed, 80 insertions(+), 93 deletions(-) diff --git a/.gitignore b/.gitignore index c20b5a45..a2f1492c 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,6 @@ venv/ build/ -docs/build/ +docs/_build/ *.charm .tox/ .coverage diff --git a/README.md b/README.md index 8983a3ec..87524339 100644 --- a/README.md +++ b/README.md @@ -957,7 +957,7 @@ def test_backup_action(): # If you didn't declare do_backup in the charm's metadata, # the `ConsistencyChecker` will slap you on the wrist and refuse to proceed. - out: scenario.ActionOutput = ctx.run_action("do_backup_action", scenario.State()) + out: scenario.ActionOutput = ctx.run_action(ctx.on.action("do_backup"), scenario.State()) # You can assert action results, logs, failure using the ActionOutput interface: assert out.logs == ['baz', 'qux'] @@ -973,17 +973,18 @@ def test_backup_action(): ## Parametrized Actions -If the action takes parameters, you'll need to instantiate an `Action`. +If the action takes parameters, you can pass those in the call. ```python def test_backup_action(): - # Define an action: - action = scenario.Action('do_backup', params={'a': 'b'}) ctx = scenario.Context(MyCharm) # If the parameters (or their type) don't match what is declared in the metadata, # the `ConsistencyChecker` will slap you on the other wrist. - out: scenario.ActionOutput = ctx.run_action(action, scenario.State()) + out: scenario.ActionOutput = ctx.run_action( + ctx.on.action("do_backup", params={'a': 'b'}), + scenario.State() + ) # ... ``` diff --git a/docs/custom_conf.py b/docs/custom_conf.py index 37ad32ef..c7f2a943 100644 --- a/docs/custom_conf.py +++ b/docs/custom_conf.py @@ -308,6 +308,7 @@ def _compute_navigation_tree(context): # Please keep this list sorted alphabetically. ('py:class', 'AnyJson'), ('py:class', '_CharmSpec'), + ('py:class', '_Event'), ('py:class', 'scenario.state._DCBase'), ('py:class', 'scenario.state._EntityStatus'), ] diff --git a/scenario/__init__.py b/scenario/__init__.py index aa70017c..2f6471e7 100644 --- a/scenario/__init__.py +++ b/scenario/__init__.py @@ -3,7 +3,6 @@ # See LICENSE file for licensing details. from scenario.context import ActionOutput, Context from scenario.state import ( - Action, ActiveStatus, Address, BindAddress, @@ -37,7 +36,6 @@ ) __all__ = [ - "Action", "ActionOutput", "CloudCredential", "CloudSpec", diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index c4281ece..b227bdcc 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -12,9 +12,9 @@ from scenario.runtime import InconsistentScenarioError from scenario.runtime import logger as scenario_logger from scenario.state import ( - Action, PeerRelation, SubordinateRelation, + _Action, _CharmSpec, normalize_name, ) @@ -274,7 +274,7 @@ def _check_storage_event( def _check_action_param_types( charm_spec: _CharmSpec, - action: Action, + action: _Action, errors: List[str], warnings: List[str], ): diff --git a/scenario/context.py b/scenario/context.py index 1248afe3..69943326 100644 --- a/scenario/context.py +++ b/scenario/context.py @@ -12,11 +12,11 @@ from scenario.logger import logger as scenario_logger from scenario.runtime import Runtime from scenario.state import ( - Action, Container, MetadataNotFoundError, Secret, Storage, + _Action, _CharmSpec, _Event, _max_posargs, @@ -26,7 +26,7 @@ from ops.testing import CharmType from scenario.ops_main_mock import Ops - from scenario.state import AnyRelation, JujuLogLine, State, _EntityStatus + from scenario.state import AnyJson, AnyRelation, JujuLogLine, State, _EntityStatus PathLike = Union[str, Path] @@ -81,7 +81,7 @@ class _Manager: def __init__( self, ctx: "Context", - arg: Union[str, Action, _Event], + arg: Union[str, _Action, _Event], state_in: "State", ): self._ctx = ctx @@ -160,7 +160,7 @@ def run(self) -> "ActionOutput": @property def _runner(self): - return self._ctx._run_action # noqa + return self._ctx._run # noqa def _get_output(self): return self._ctx._finalize_action(self._ctx.output_state) # noqa @@ -312,6 +312,19 @@ def storage_detaching(storage: Storage): def pebble_ready(container: Container): return _Event(f"{container.name}_pebble_ready", container=container) + @staticmethod + def action( + name: str, + params: Optional[Dict[str, "AnyJson"]] = None, + id: Optional[str] = None, + ): + kwargs = {} + if params: + kwargs["params"] = params + if id: + kwargs["id"] = id + return _Event(f"{name}_action", action=_Action(name, **kwargs)) + class Context: """Represents a simulated charm's execution context. @@ -578,7 +591,7 @@ def manager(self, event: "_Event", state: "State"): """ return _EventManager(self, event, state) - def action_manager(self, action: "Action", state: "State"): + def action_manager(self, action: "_Action", state: "State"): """Context manager to introspect live charm object before and after the event is emitted. Usage: @@ -608,23 +621,23 @@ def run(self, event: "_Event", state: "State") -> "State": :arg state: the State instance to use as data source for the hook tool calls that the charm will invoke when handling the Event. """ - if isinstance(event, Action) or event.action: + if isinstance(event, _Action) or event.action: raise InvalidEventError("Use run_action() to run an action event.") with self._run_event(event=event, state=state) as ops: ops.emit() return self.output_state - def run_action(self, action: "Action", state: "State") -> ActionOutput: - """Trigger a charm execution with an Action and a State. + def run_action(self, event: "_Event", state: "State") -> ActionOutput: + """Trigger a charm execution with an action event and a State. Calling this function will call ``ops.main`` and set up the context according to the specified ``State``, then emit the event on the charm. - :arg action: the Action that the charm will execute. + :arg event: the action event that the charm will execute. :arg state: the State instance to use as data source for the hook tool calls that the - charm will invoke when handling the Action (event). + charm will invoke when handling the action event. """ - with self._run_action(action=action, state=state) as ops: + with self._run(event=event, state=state) as ops: ops.emit() return self._finalize_action(self.output_state) @@ -643,11 +656,6 @@ def _finalize_action(self, state_out: "State"): return ao - @contextmanager - def _run_action(self, action: "Action", state: "State"): - with self._run(event=action.event, state=state) as ops: - yield ops - @contextmanager def _run(self, event: "_Event", state: "State"): runtime = Runtime( diff --git a/scenario/state.py b/scenario/state.py index 3b574e02..30354953 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -1658,7 +1658,7 @@ class _Event: notice: Optional[Notice] = None """If this is a Pebble notice event, the notice it refers to.""" - action: Optional["Action"] = None + action: Optional["_Action"] = None """If this is an action event, the :class:`Action` it refers to.""" _owner_path: List[str] = dataclasses.field(default_factory=list) @@ -1815,15 +1815,17 @@ def next_action_id(*, update=True): @dataclasses.dataclass(frozen=True) -class Action(_max_posargs(1)): +class _Action(_max_posargs(1)): """A ``juju run`` command. Used to simulate ``juju run``, passing in any parameters. For example:: def test_backup_action(): - action = scenario.Action('do_backup', params={'filename': 'foo'}) ctx = scenario.Context(MyCharm) - out: scenario.ActionOutput = ctx.run_action(action, scenario.State()) + out: scenario.ActionOutput = ctx.run_action( + ctx.on.action('do_backup', params={'filename': 'foo'}), + scenario.State() + ) """ name: str @@ -1838,11 +1840,6 @@ def test_backup_action(): Every action invocation is automatically assigned a new one. Override in the rare cases where a specific ID is required.""" - @property - def event(self) -> _Event: - """Helper to generate an action event from this action.""" - return _Event(self.name + ACTION_EVENT_SUFFIX, action=self) - def deferred( event: Union[str, _Event], diff --git a/tests/test_consistency_checker.py b/tests/test_consistency_checker.py index 82d9c76a..2e2efb9e 100644 --- a/tests/test_consistency_checker.py +++ b/tests/test_consistency_checker.py @@ -4,10 +4,10 @@ from ops.charm import CharmBase from scenario.consistency_checker import check_consistency +from scenario.context import Context from scenario.runtime import InconsistentScenarioError from scenario.state import ( RELATION_EVENTS_SUFFIX, - Action, CloudCredential, CloudSpec, Container, @@ -22,6 +22,7 @@ Storage, StoredState, SubordinateRelation, + _Action, _CharmSpec, _Event, ) @@ -377,19 +378,19 @@ def test_relation_not_in_state(): def test_action_not_in_meta_inconsistent(): - action = Action("foo", params={"bar": "baz"}) + ctx = Context(MyCharm, meta={"name": "foo"}, actions={"foo": {}}) assert_inconsistent( State(), - action.event, + ctx.on.action("foo", params={"bar": "baz"}), _CharmSpec(MyCharm, meta={}, actions={}), ) def test_action_meta_type_inconsistent(): - action = Action("foo", params={"bar": "baz"}) + ctx = Context(MyCharm, meta={"name": "foo"}, actions={"foo": {}}) assert_inconsistent( State(), - action.event, + ctx.on.action("foo", params={"bar": "baz"}), _CharmSpec( MyCharm, meta={}, actions={"foo": {"params": {"bar": {"type": "zabazaba"}}}} ), @@ -397,24 +398,24 @@ def test_action_meta_type_inconsistent(): assert_inconsistent( State(), - action.event, + ctx.on.action("foo", params={"bar": "baz"}), _CharmSpec(MyCharm, meta={}, actions={"foo": {"params": {"bar": {}}}}), ) def test_action_name(): - action = Action("foo", params={"bar": "baz"}) + ctx = Context(MyCharm, meta={"name": "foo"}, actions={"foo": {}}) assert_consistent( State(), - action.event, + ctx.on.action("foo", params={"bar": "baz"}), _CharmSpec( MyCharm, meta={}, actions={"foo": {"params": {"bar": {"type": "string"}}}} ), ) assert_inconsistent( State(), - _Event("box_action", action=action), + _Event("box_action", action=ctx.on.action("foo", params={"bar": "baz"})), _CharmSpec(MyCharm, meta={}, actions={"foo": {}}), ) @@ -431,19 +432,18 @@ def test_action_name(): @pytest.mark.parametrize("ptype,good,bad", _ACTION_TYPE_CHECKS) def test_action_params_type(ptype, good, bad): - action = Action("foo", params={"bar": good}) + ctx = Context(MyCharm, meta={"name": "foo"}, actions={"foo": {}}) assert_consistent( State(), - action.event, + ctx.on.action("foo", params={"bar": good}), _CharmSpec( MyCharm, meta={}, actions={"foo": {"params": {"bar": {"type": ptype}}}} ), ) if bad is not None: - action = Action("foo", params={"bar": bad}) assert_inconsistent( State(), - action.event, + ctx.on.action("foo", params={"bar": bad}), _CharmSpec( MyCharm, meta={}, actions={"foo": {"params": {"bar": {"type": ptype}}}} ), diff --git a/tests/test_context.py b/tests/test_context.py index aed14159..2ca8b93a 100644 --- a/tests/test_context.py +++ b/tests/test_context.py @@ -3,8 +3,8 @@ import pytest from ops import CharmBase -from scenario import Action, ActionOutput, Context, State -from scenario.state import _Event, next_action_id +from scenario import ActionOutput, Context, State +from scenario.state import _Action, _Event, next_action_id class MyCharm(CharmBase): @@ -34,22 +34,19 @@ def test_run_action(): state = State() expected_id = next_action_id(update=False) - with patch.object(ctx, "_run_action") as p: - ctx._output_state = ( - "foo" # would normally be set within the _run_action call scope - ) - action = Action("do-foo") - output = ctx.run_action(action, state) + with patch.object(ctx, "_run") as p: + ctx._output_state = "foo" # would normally be set within the _run call scope + output = ctx.run_action(ctx.on.action("do-foo"), state) assert output.state == "foo" assert p.called - a = p.call_args.kwargs["action"] + e = p.call_args.kwargs["event"] s = p.call_args.kwargs["state"] - assert isinstance(a, Action) - assert a.event.name == "do_foo_action" + assert isinstance(e, _Event) + assert e.name == "do_foo_action" assert s is state - assert a.id == expected_id + assert e.action.id == expected_id @pytest.mark.parametrize("app_name", ("foo", "bar", "george")) @@ -76,6 +73,6 @@ def _on_act_action(self, _): pass ctx = Context(MyCharm, meta={"name": "foo"}, actions={"act": {}}) - out = ctx.run_action(Action("act"), State()) + out = ctx.run_action(ctx.on.action("act"), State()) assert out.results is None assert out.failure is None diff --git a/tests/test_context_on.py b/tests/test_context_on.py index 1c98b4ea..151bc303 100644 --- a/tests/test_context_on.py +++ b/tests/test_context_on.py @@ -173,8 +173,7 @@ def test_action_event_no_params(): ctx = scenario.Context(ContextCharm, meta=META, actions=ACTIONS) # These look like: # ctx.run_action(ctx.on.action(action), state) - action = scenario.Action("act") - with ctx.action_manager(action, scenario.State()) as mgr: + with ctx.action_manager(ctx.on.action("act"), scenario.State()) as mgr: mgr.run() assert len(mgr.charm.observed) == 2 assert isinstance(mgr.charm.observed[1], ops.CollectStatusEvent) @@ -184,18 +183,18 @@ def test_action_event_no_params(): def test_action_event_with_params(): ctx = scenario.Context(ContextCharm, meta=META, actions=ACTIONS) - action = scenario.Action("act", params={"param": "hello"}) # These look like: # ctx.run_action(ctx.on.action(action=action), state) # So that any parameters can be included and the ID can be customised. - with ctx.action_manager(action, scenario.State()) as mgr: + call_event = ctx.on.action("act", params={"param": "hello"}) + with ctx.action_manager(call_event, scenario.State()) as mgr: mgr.run() assert len(mgr.charm.observed) == 2 assert isinstance(mgr.charm.observed[1], ops.CollectStatusEvent) event = mgr.charm.observed[0] assert isinstance(event, ops.ActionEvent) - assert event.id == action.id - assert event.params["param"] == action.params["param"] + assert event.id == call_event.action.id + assert event.params["param"] == call_event.action.params["param"] def test_pebble_ready_event(): diff --git a/tests/test_e2e/test_actions.py b/tests/test_e2e/test_actions.py index 34c9cd94..b0668355 100644 --- a/tests/test_e2e/test_actions.py +++ b/tests/test_e2e/test_actions.py @@ -5,7 +5,7 @@ from scenario import Context from scenario.context import InvalidEventError -from scenario.state import Action, State, next_action_id +from scenario.state import State, _Action, next_action_id @pytest.fixture(scope="function") @@ -34,8 +34,7 @@ def test_action_event(mycharm, baz_value): "foo": {"params": {"bar": {"type": "number"}, "baz": {"type": "boolean"}}} }, ) - action = Action("foo", params={"baz": baz_value, "bar": 10}) - ctx.run_action(action, State()) + ctx.run_action(ctx.on.action("foo", params={"baz": baz_value, "bar": 10}), State()) evt = ctx.emitted_events[0] @@ -51,24 +50,15 @@ def handle_evt(charm: CharmBase, evt: ActionEvent): mycharm._evt_handler = handle_evt - action = Action("foo") ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - ctx.run_action(action, State()) + ctx.run_action(ctx.on.action("foo"), State()) def test_cannot_run_action(mycharm): ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - action = Action("foo") with pytest.raises(InvalidEventError): - ctx.run(action, state=State()) - - -def test_cannot_run_action_event(mycharm): - ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - action = Action("foo") - with pytest.raises(InvalidEventError): - ctx.run(action.event, state=State()) + ctx.run(ctx.on.action("foo"), state=State()) @pytest.mark.parametrize("res_value", ({"a": {"b": {"c"}}}, {"d": "e"})) @@ -82,10 +72,9 @@ def handle_evt(charm: CharmBase, evt): mycharm._evt_handler = handle_evt - action = Action("foo") ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - out = ctx.run_action(action, State()) + out = ctx.run_action(ctx.on.action("foo"), State()) assert out.results == res_value assert out.success is True @@ -104,9 +93,8 @@ def handle_evt(charm: CharmBase, evt: ActionEvent): mycharm._evt_handler = handle_evt - action = Action("foo") ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - out = ctx.run_action(action, State()) + out = ctx.run_action(ctx.on.action("foo"), State()) assert out.failure == "failed becozz" assert out.logs == ["log1", "log2"] @@ -133,9 +121,8 @@ def handle_evt(charm: CharmBase, evt: ActionEvent): mycharm._evt_handler = handle_evt - action = Action("foo") ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - ctx.run_action(action, State()) + ctx.run_action(ctx.on.action("foo"), State()) @pytest.mark.skipif( @@ -151,20 +138,19 @@ def handle_evt(charm: CharmBase, evt: ActionEvent): mycharm._evt_handler = handle_evt - action = Action("foo", id=uuid) ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - ctx.run_action(action, State()) + ctx.run_action(ctx.on.action("foo", id=uuid), State()) def test_positional_arguments(): with pytest.raises(TypeError): - Action("foo", {}) + _Action("foo", {}) def test_default_arguments(): expected_id = next_action_id(update=False) name = "foo" - action = Action(name) + action = _Action(name) assert action.name == name assert action.params == {} assert action.id == expected_id diff --git a/tests/test_e2e/test_manager.py b/tests/test_e2e/test_manager.py index 66d39f82..3f99ffd0 100644 --- a/tests/test_e2e/test_manager.py +++ b/tests/test_e2e/test_manager.py @@ -4,7 +4,7 @@ from ops import ActiveStatus from ops.charm import CharmBase, CollectStatusEvent -from scenario import Action, Context, State +from scenario import Context, State from scenario.context import ActionOutput, AlreadyEmittedError, _EventManager @@ -73,7 +73,7 @@ def test_context_manager(mycharm): def test_context_action_manager(mycharm): ctx = Context(mycharm, meta=mycharm.META, actions=mycharm.ACTIONS) - with ctx.action_manager(Action("do-x"), State()) as manager: + with ctx.action_manager(ctx.on.action("do-x"), State()) as manager: ao = manager.run() assert isinstance(ao, ActionOutput) assert ctx.emitted_events[0].handle.kind == "do_x_action" From 54c9b7a2a148ce7cea01263af6538de42f640c69 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 24 Jul 2024 17:21:30 +1200 Subject: [PATCH 2/2] Use Iterable as the container component type, so that any iterable of hashable objects can be passed, but still convert to frozenset underneath. (#160) --- docs/custom_conf.py | 1 + scenario/state.py | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/custom_conf.py b/docs/custom_conf.py index c7f2a943..3e035474 100644 --- a/docs/custom_conf.py +++ b/docs/custom_conf.py @@ -311,4 +311,5 @@ def _compute_navigation_tree(context): ('py:class', '_Event'), ('py:class', 'scenario.state._DCBase'), ('py:class', 'scenario.state._EntityStatus'), + ('py:class', 'scenario.state._max_posargs.._MaxPositionalArgs'), ] diff --git a/scenario/state.py b/scenario/state.py index 30354953..6adf3289 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -19,6 +19,7 @@ Final, FrozenSet, Generic, + Iterable, List, Literal, Optional, @@ -1210,9 +1211,9 @@ class State(_max_posargs(0)): default_factory=dict, ) """The present configuration of this charm.""" - relations: FrozenSet["AnyRelation"] = dataclasses.field(default_factory=frozenset) + relations: Iterable["AnyRelation"] = dataclasses.field(default_factory=frozenset) """All relations that currently exist for this charm.""" - networks: FrozenSet[Network] = dataclasses.field(default_factory=frozenset) + networks: Iterable[Network] = dataclasses.field(default_factory=frozenset) """Manual overrides for any relation and extra bindings currently provisioned for this charm. If a metadata-defined relation endpoint is not explicitly mapped to a Network in this field, it will be defaulted. @@ -1220,24 +1221,24 @@ class State(_max_posargs(0)): support it, but use at your own risk.] If a metadata-defined extra-binding is left empty, it will be defaulted. """ - containers: FrozenSet[Container] = dataclasses.field(default_factory=frozenset) + containers: Iterable[Container] = dataclasses.field(default_factory=frozenset) """All containers (whether they can connect or not) that this charm is aware of.""" - storages: FrozenSet[Storage] = dataclasses.field(default_factory=frozenset) + storages: Iterable[Storage] = dataclasses.field(default_factory=frozenset) """All ATTACHED storage instances for this charm. If a storage is not attached, omit it from this listing.""" # we don't use sets to make json serialization easier - opened_ports: FrozenSet[_Port] = dataclasses.field(default_factory=frozenset) + opened_ports: Iterable[_Port] = dataclasses.field(default_factory=frozenset) """Ports opened by juju on this charm.""" leader: bool = False """Whether this charm has leadership.""" model: Model = Model() """The model this charm lives in.""" - secrets: FrozenSet[Secret] = dataclasses.field(default_factory=frozenset) + secrets: Iterable[Secret] = dataclasses.field(default_factory=frozenset) """The secrets this charm has access to (as an owner, or as a grantee). The presence of a secret in this list entails that the charm can read it. Whether it can manage it or not depends on the individual secret's `owner` flag.""" - resources: FrozenSet[Resource] = dataclasses.field(default_factory=frozenset) + resources: Iterable[Resource] = dataclasses.field(default_factory=frozenset) """All resources that this charm can access.""" planned_units: int = 1 """Number of non-dying planned units that are expected to be running this application. @@ -1249,7 +1250,7 @@ class State(_max_posargs(0)): # to this list. deferred: List["DeferredEvent"] = dataclasses.field(default_factory=list) """Events that have been deferred on this charm by some previous execution.""" - stored_states: FrozenSet["StoredState"] = dataclasses.field( + stored_states: Iterable["StoredState"] = dataclasses.field( default_factory=frozenset, ) """Contents of a charm's stored state.""" @@ -1306,9 +1307,8 @@ def __post_init__(self): "stored_states", ]: val = getattr(self, name) - # We check for "not frozenset" rather than "is set" so that you can - # actually pass a tuple or list or really any iterable of hashable - # objects, and it will end up as a frozenset. + # It's ok to pass any iterable (of hashable objects), but you'll get + # a frozenset as the actual attribute. if not isinstance(val, frozenset): object.__setattr__(self, name, frozenset(val))