From 56545a4558cae34684843a43a9d102be03ba0585 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 2 Aug 2024 14:32:14 +1200 Subject: [PATCH] Unify run() and run_action(). --- README.md | 14 ++- scenario/__init__.py | 6 +- scenario/context.py | 153 +++++++++++---------------------- scenario/mocking.py | 13 ++- scenario/runtime.py | 3 +- scenario/state.py | 2 +- tests/test_context.py | 21 +++-- tests/test_context_on.py | 8 +- tests/test_e2e/test_actions.py | 47 +++++----- tests/test_e2e/test_manager.py | 12 +-- 10 files changed, 118 insertions(+), 161 deletions(-) diff --git a/README.md b/README.md index b57edd03..36ee7615 100644 --- a/README.md +++ b/README.md @@ -963,7 +963,6 @@ class MyVMCharm(ops.CharmBase): An action is a special sort of event, even though `ops` handles them almost identically. In most cases, you'll want to inspect the 'results' of an action, or whether it has failed or logged something while executing. Many actions don't have a direct effect on the output state. -For this reason, the output state is less prominent in the return type of `Context.run_action`. How to test actions with scenario: @@ -975,18 +974,18 @@ 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(ctx.on.action("do_backup"), scenario.State()) + out: scenario.ActionOutput = ctx.run(ctx.on.action("do_backup"), scenario.State()) # You can assert action results, logs, failure using the ActionOutput interface: assert out.logs == ['baz', 'qux'] if out.success: - # If the action did not fail, we can read the results: - assert out.results == {'foo': 'bar'} + # If the action did not fail, we can read the results: + assert out.results == {'foo': 'bar'} else: - # If the action fails, we can read a failure message: - assert out.failure == 'boo-hoo' + # If the action fails, we can read a failure message: + assert out.failure == 'boo-hoo' ``` ## Parametrized Actions @@ -999,7 +998,7 @@ def test_backup_action(): # 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( + out: scenario.ActionOutput = ctx.run( ctx.on.action("do_backup", params={'a': 'b'}), scenario.State() ) @@ -1137,7 +1136,6 @@ def test_live_charm_introspection(mycharm): assert charm._stored.a == "a" # This will tell ops.main to proceed with normal execution and emit the "start" event on the charm: - # If you want to do this with actions, you should use `run_action` instead. state_out = event.run() # After that is done, we are handed back control, and we can again do some introspection: diff --git a/scenario/__init__.py b/scenario/__init__.py index bbd2c694..55017cba 100644 --- a/scenario/__init__.py +++ b/scenario/__init__.py @@ -1,7 +1,8 @@ #!/usr/bin/env python3 # Copyright 2023 Canonical Ltd. # See LICENSE file for licensing details. -from scenario.context import ActionOutput, Context +from scenario.context import Context, Task +from scenario.mocking import ActionFailed from scenario.state import ( ActiveStatus, Address, @@ -37,7 +38,8 @@ ) __all__ = [ - "ActionOutput", + "Task", + "ActionFailed", "CheckInfo", "CloudCredential", "CloudSpec", diff --git a/scenario/context.py b/scenario/context.py index d8a95751..9c6a3d68 100644 --- a/scenario/context.py +++ b/scenario/context.py @@ -38,35 +38,48 @@ @dataclasses.dataclass(frozen=True) -class ActionOutput(_max_posargs(0)): - """Wraps the results of running an action event with ``run_action``.""" +class Task(_max_posargs(0)): + """Wraps the results of running an action event on a unit with ``run``.""" - state: "State" - """The charm state after the action has been handled. - - In most cases, actions are not expected to be affecting it.""" - logs: List[str] + logs: List[str] = dataclasses.field(default_factory=list) """Any logs associated with the action output, set by the charm with :meth:`ops.ActionEvent.log`.""" + results: Optional[Dict[str, Any]] = None """Key-value mapping assigned by the charm as a result of the action. Will be None if the charm never calls :meth:`ops.ActionEvent.set_results`.""" - failure: Optional[str] = None - """None if the action was successful, otherwise the message the charm set with - :meth:`ops.ActionEvent.fail`.""" - @property - def success(self) -> bool: - """True if this action was a success, False otherwise.""" - return self.failure is None + status: str = "pending" + + # Note that in the Juju struct, this is called "fail". + failure_message: str = "" + + def set_status(self, status): + """Set the status of the task.""" + # bypass frozen dataclass + object.__setattr__(self, "status", status) + + def set_failure_message(self, message): + """Record an explanation of why this task failed.""" + # bypass frozen dataclass + object.__setattr__(self, "failure_message", message) + + def set_results(self, results: Dict[str, Any]): + """Set the results of the action.""" + if self.results is None: + # bypass frozen dataclass + object.__setattr__(self, "results", results) + else: + self.results.clear() + self.results.update(results) class InvalidEventError(RuntimeError): - """raised when something is wrong with the event passed to Context.run_*""" + """raised when something is wrong with the event passed to Context.run""" class InvalidActionError(InvalidEventError): - """raised when something is wrong with the action passed to Context.run_action""" + """raised when something is wrong with an action passed to Context.run""" class ContextSetupError(RuntimeError): @@ -77,7 +90,7 @@ class AlreadyEmittedError(RuntimeError): """Raised when ``run()`` is called more than once.""" -class _Manager: +class Manager: """Context manager to offer test code some runtime charm object introspection.""" def __init__( @@ -93,13 +106,13 @@ def __init__( self._emitted: bool = False self.ops: Optional["Ops"] = None - self.output: Optional[Union["State", ActionOutput]] = None + self.output: Optional["State"] = None @property def charm(self) -> CharmBase: if not self.ops: raise RuntimeError( - "you should __enter__ this contextmanager before accessing this", + "you should __enter__ this context manager before accessing this", ) return cast(CharmBase, self.ops.charm) @@ -107,16 +120,13 @@ def charm(self) -> CharmBase: def _runner(self): return self._ctx._run # noqa - def _get_output(self): - raise NotImplementedError("override in subclass") - def __enter__(self): self._wrapped_ctx = wrapped_ctx = self._runner(self._arg, self._state_in) ops = wrapped_ctx.__enter__() self.ops = ops return self - def _run(self) -> Union[ActionOutput, "State"]: + def run(self) -> "State": """Emit the event and proceed with charm execution. This can only be done once. @@ -128,33 +138,15 @@ def _run(self) -> Union[ActionOutput, "State"]: # wrap up Runtime.exec() so that we can gather the output state self._wrapped_ctx.__exit__(None, None, None) - return self._get_output() + assert self._ctx._output_state is not None + return self._ctx._output_state def __exit__(self, exc_type, exc_val, exc_tb): # noqa: U100 if not self._emitted: - logger.debug("user didn't emit the event within the context manager scope. Doing so implicitly upon exit...") - # The output is discarded so we can use the private method. - self._run() - - -class ManagedEvent(_Manager): - charm: CharmBase # type: ignore - - def run(self) -> "State": - return cast("State", super()._run()) - - def _get_output(self): - return self._ctx._output_state # noqa - - -class ManagedAction(_Manager): - charm: CharmBase # type: ignore - - def run_action(self) -> "ActionOutput": - return cast("ActionOutput", super()._run()) - - def _get_output(self): - return self._ctx._finalize_action(self._ctx.output_state) # noqa + logger.debug( + "user didn't emit the event within the context manager scope. Doing so implicitly upon exit...", + ) + self.run() class _CharmEvents: @@ -349,14 +341,12 @@ class Context: It contains: the charm source code being executed, the metadata files associated with it, a charm project repository root, and the Juju version to be simulated. - After you have instantiated ``Context``, typically you will call one of ``run()`` or - ``run_action()`` to execute the charm once, write any assertions you like on the output - state returned by the call, write any assertions you like on the ``Context`` attributes, - then discard the ``Context``. + After you have instantiated ``Context``, typically you will call ``run()``to execute the charm + once, write any assertions you like on the output state returned by the call, write any + assertions you like on the ``Context`` attributes, then discard the ``Context``. Each ``Context`` instance is in principle designed to be single-use: ``Context`` is not cleaned up automatically between charm runs. - You can call ``.clear()`` to do some clean up, but we don't guarantee all state will be gone. Any side effects generated by executing the charm, that are not rightful part of the ``State``, are in fact stored in the ``Context``: @@ -435,13 +425,11 @@ def __init__( It contains: the charm source code being executed, the metadata files associated with it, a charm project repository root, and the juju version to be simulated. - After you have instantiated Context, typically you will call one of `run()` or - `run_action()` to execute the charm once, write any assertions you like on the output - state returned by the call, write any assertions you like on the Context attributes, - then discard the Context. + After you have instantiated Context, typically you will call `run()` to execute the charm + once, write any assertions you like on the output state returned by the call, write any + assertions you like on the Context attributes, then discard the Context. Each Context instance is in principle designed to be single-use: Context is not cleaned up automatically between charm runs. - You can call `.clear()` to do some clean up, but we don't guarantee all state will be gone. Any side effects generated by executing the charm, that are not rightful part of the State, are in fact stored in the Context: @@ -546,11 +534,8 @@ def __init__( # set by Runtime.exec() in self._run() self._output_state: Optional["State"] = None - # ephemeral side effects from running an action - - self._action_logs: List[str] = [] - self._action_results: Optional[Dict[str, str]] = None - self._action_failure: Optional[str] = None + # operations (and embedded tasks) from running actions + self.action_history: List[Task] = [] self.on = _CharmEvents() @@ -600,18 +585,11 @@ def __call__(self, event: "_Event", state: "State"): event.run() # this will fire the event assert event.charm._some_private_attribute == "bar" # noqa - with ctx(Action("foo"), State()) as event: - event.charm._some_private_setup() - event.run_action() # this will fire the event - assert event.charm._some_private_attribute == "bar" # noqa - Args: event: the :class:`Event` or :class:`Action` that the charm will respond to. state: the :class:`State` instance to use when handling the Event. """ - if isinstance(event, _Action) or event.action: - return ManagedAction(self, event, state) - return ManagedEvent(self, event, state) + return Manager(self, event, state) def run(self, event: "_Event", state: "State") -> "State": """Trigger a charm execution with an Event and a State. @@ -624,42 +602,13 @@ def run(self, event: "_Event", state: "State") -> "State": charm will invoke when handling the Event. """ if event.action: - raise InvalidEventError("Use run_action() to run an action event.") + self.action_history.append(Task()) with self._run(event=event, state=state) as ops: ops.emit() + if event.action: + self.action_history[-1].set_status("completed") return self.output_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 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. - """ - if not event.action: - raise InvalidEventError("Use run() to run an non-action event.") - with self._run(event=event, state=state) as ops: - ops.emit() - return self._finalize_action(self.output_state) - - def _finalize_action(self, state_out: "State"): - ao = ActionOutput( - state=state_out, - logs=self._action_logs, - results=self._action_results, - failure=self._action_failure, - ) - - # reset all action-related state - self._action_logs = [] - self._action_results = None - self._action_failure = None - - return ao - @contextmanager def _run(self, event: "_Event", state: "State"): runtime = Runtime( diff --git a/scenario/mocking.py b/scenario/mocking.py index 6a65ef41..7246e431 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -72,6 +72,11 @@ class ActionMissingFromContextError(Exception): # this flow. +class ActionFailed(Exception): + def __init__(self, message: str): + self.message = message + + class _MockExecProcess: def __init__(self, command: Tuple[str, ...], change_id: int, out: "ExecOutput"): self._command = command @@ -528,21 +533,23 @@ def action_set(self, results: Dict[str, Any]): _format_action_result_dict(results) # but then we will store it in its unformatted, # original form for testing ease - self._context._action_results = results + self._context.action_history[-1].set_results(results) def action_fail(self, message: str = ""): if not self._event.action: raise ActionMissingFromContextError( "not in the context of an action event: cannot action-fail", ) - self._context._action_failure = message + self._context.action_history[-1].set_status("failed") + self._context.action_history[-1].set_failure_message(message) + raise ActionFailed(message) def action_log(self, message: str): if not self._event.action: raise ActionMissingFromContextError( "not in the context of an action event: cannot action-log", ) - self._context._action_logs.append(message) + self._context.action_history[-1].logs.append(message) def action_get(self): action = self._event.action diff --git a/scenario/runtime.py b/scenario/runtime.py index 2f739f8f..88fdeb9e 100644 --- a/scenario/runtime.py +++ b/scenario/runtime.py @@ -19,6 +19,7 @@ from scenario.capture_events import capture_events from scenario.logger import logger as scenario_logger +from scenario.mocking import ActionFailed from scenario.ops_main_mock import NoObserverError from scenario.state import DeferredEvent, PeerRelation, StoredState @@ -466,7 +467,7 @@ def exec( # if the caller did not manually emit or commit: do that. ops.finalize() - except NoObserverError: + except (NoObserverError, ActionFailed): raise # propagate along except Exception as e: raise UncaughtCharmError( diff --git a/scenario/state.py b/scenario/state.py index 9362b2c2..1014ace7 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -1837,7 +1837,7 @@ class _Action(_max_posargs(1)): def test_backup_action(): ctx = scenario.Context(MyCharm) - out: scenario.ActionOutput = ctx.run_action( + out: scenario.ActionOutput = ctx.run( ctx.on.action('do_backup', params={'filename': 'foo'}), scenario.State() ) diff --git a/tests/test_context.py b/tests/test_context.py index 851e73f4..d4f14dc4 100644 --- a/tests/test_context.py +++ b/tests/test_context.py @@ -3,7 +3,7 @@ import pytest from ops import CharmBase -from scenario import ActionOutput, Context, State +from scenario import Context, State, Task from scenario.state import _Event, next_action_id @@ -36,8 +36,8 @@ def test_run_action(): 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" + output = ctx.run(ctx.on.action("do-foo"), state) + assert output == "foo" assert p.called e = p.call_args.kwargs["event"] @@ -66,13 +66,13 @@ def test_context_manager(): assert event.charm.meta.name == "foo" with ctx(ctx.on.action("act"), state) as event: - event.run_action() + event.run() assert event.charm.meta.name == "foo" -def test_action_output_no_positional_arguments(): +def test_task_no_positional_arguments(): with pytest.raises(TypeError): - ActionOutput(None, None) + Task(None) def test_action_output_no_results(): @@ -85,6 +85,9 @@ def _on_act_action(self, _): pass ctx = Context(MyCharm, meta={"name": "foo"}, actions={"act": {}}) - out = ctx.run_action(ctx.on.action("act"), State()) - assert out.results is None - assert out.failure is None + ctx.run(ctx.on.action("act"), State()) + assert len(ctx.action_history) == 1 + task = ctx.action_history[0] + assert task.results is None + assert task.status == "completed" + assert task.failure_message == "" diff --git a/tests/test_context_on.py b/tests/test_context_on.py index f1ab20ca..83b9df76 100644 --- a/tests/test_context_on.py +++ b/tests/test_context_on.py @@ -172,9 +172,9 @@ def test_storage_events(event_name, event_kind): def test_action_event_no_params(): ctx = scenario.Context(ContextCharm, meta=META, actions=ACTIONS) # These look like: - # ctx.run_action(ctx.on.action(action_name), state) + # ctx.run(ctx.on.action(action_name), state) with ctx(ctx.on.action("act"), scenario.State()) as event: - event.run_action() + event.run() assert len(event.charm.observed) == 2 assert isinstance(event.charm.observed[1], ops.CollectStatusEvent) event = event.charm.observed[0] @@ -184,11 +184,11 @@ def test_action_event_no_params(): def test_action_event_with_params(): ctx = scenario.Context(ContextCharm, meta=META, actions=ACTIONS) # These look like: - # ctx.run_action(ctx.on.action(action=action), state) + # ctx.run(ctx.on.action(action=action), state) # So that any parameters can be included and the ID can be customised. call_event = ctx.on.action("act", params={"param": "hello"}) with ctx(call_event, scenario.State()) as event: - event.run_action() + event.run() assert len(event.charm.observed) == 2 assert isinstance(event.charm.observed[1], ops.CollectStatusEvent) event = event.charm.observed[0] diff --git a/tests/test_e2e/test_actions.py b/tests/test_e2e/test_actions.py index b0668355..a115a704 100644 --- a/tests/test_e2e/test_actions.py +++ b/tests/test_e2e/test_actions.py @@ -3,8 +3,7 @@ from ops.charm import ActionEvent, CharmBase from ops.framework import Framework -from scenario import Context -from scenario.context import InvalidEventError +from scenario import ActionFailed, Context from scenario.state import State, _Action, next_action_id @@ -34,10 +33,10 @@ def test_action_event(mycharm, baz_value): "foo": {"params": {"bar": {"type": "number"}, "baz": {"type": "boolean"}}} }, ) - ctx.run_action(ctx.on.action("foo", params={"baz": baz_value, "bar": 10}), State()) + state = ctx.run(ctx.on.action("foo", params={"baz": baz_value, "bar": 10}), State()) + assert isinstance(state, State) evt = ctx.emitted_events[0] - assert evt.params["bar"] == 10 assert evt.params["baz"] is baz_value @@ -51,19 +50,12 @@ def handle_evt(charm: CharmBase, evt: ActionEvent): mycharm._evt_handler = handle_evt ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - ctx.run_action(ctx.on.action("foo"), State()) - - -def test_cannot_run_action(mycharm): - ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - - with pytest.raises(InvalidEventError): - ctx.run(ctx.on.action("foo"), state=State()) + ctx.run(ctx.on.action("foo"), State()) @pytest.mark.parametrize("res_value", ({"a": {"b": {"c"}}}, {"d": "e"})) def test_action_event_results_valid(mycharm, res_value): - def handle_evt(charm: CharmBase, evt): + def handle_evt(_: CharmBase, evt): if not isinstance(evt, ActionEvent): return evt.set_results(res_value) @@ -74,15 +66,16 @@ def handle_evt(charm: CharmBase, evt): ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - out = ctx.run_action(ctx.on.action("foo"), State()) + ctx.run(ctx.on.action("foo"), State()) - assert out.results == res_value - assert out.success is True + assert len(ctx.action_history) == 1 + assert ctx.action_history[0].results == res_value + assert ctx.action_history[0].status == "completed" @pytest.mark.parametrize("res_value", ({"a": {"b": {"c"}}}, {"d": "e"})) def test_action_event_outputs(mycharm, res_value): - def handle_evt(charm: CharmBase, evt: ActionEvent): + def handle_evt(_: CharmBase, evt: ActionEvent): if not isinstance(evt, ActionEvent): return @@ -94,11 +87,15 @@ def handle_evt(charm: CharmBase, evt: ActionEvent): mycharm._evt_handler = handle_evt ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - out = ctx.run_action(ctx.on.action("foo"), State()) - - assert out.failure == "failed becozz" - assert out.logs == ["log1", "log2"] - assert out.success is False + with pytest.raises(ActionFailed) as out: + ctx.run(ctx.on.action("foo"), State()) + assert out.value.message == "failed becozz" + assert len(ctx.action_history) == 1 + task = ctx.action_history[0] + assert task.results == {"my-res": res_value} + assert task.logs == ["log1", "log2"] + assert task.failure_message == "failed becozz" + assert task.status == "failed" def _ops_less_than(wanted_major, wanted_minor): @@ -114,7 +111,7 @@ def _ops_less_than(wanted_major, wanted_minor): _ops_less_than(2, 11), reason="ops 2.10 and earlier don't have ActionEvent.id" ) def test_action_event_has_id(mycharm): - def handle_evt(charm: CharmBase, evt: ActionEvent): + def handle_evt(_: CharmBase, evt: ActionEvent): if not isinstance(evt, ActionEvent): return assert isinstance(evt.id, str) and evt.id != "" @@ -122,7 +119,7 @@ def handle_evt(charm: CharmBase, evt: ActionEvent): mycharm._evt_handler = handle_evt ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - ctx.run_action(ctx.on.action("foo"), State()) + ctx.run(ctx.on.action("foo"), State()) @pytest.mark.skipif( @@ -139,7 +136,7 @@ def handle_evt(charm: CharmBase, evt: ActionEvent): mycharm._evt_handler = handle_evt ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - ctx.run_action(ctx.on.action("foo", id=uuid), State()) + ctx.run(ctx.on.action("foo", id=uuid), State()) def test_positional_arguments(): diff --git a/tests/test_e2e/test_manager.py b/tests/test_e2e/test_manager.py index 12ed2fd6..1856c7ae 100644 --- a/tests/test_e2e/test_manager.py +++ b/tests/test_e2e/test_manager.py @@ -5,7 +5,7 @@ from ops.charm import CharmBase, CollectStatusEvent from scenario import Context, State -from scenario.context import ActionOutput, AlreadyEmittedError, ManagedEvent +from scenario.context import AlreadyEmittedError, Manager @pytest.fixture(scope="function") @@ -30,7 +30,7 @@ def _on_event(self, e): def test_manager(mycharm): ctx = Context(mycharm, meta=mycharm.META) - with ManagedEvent(ctx, ctx.on.start(), State()) as manager: + with Manager(ctx, ctx.on.start(), State()) as manager: assert isinstance(manager.charm, mycharm) state_out = manager.run() @@ -39,7 +39,7 @@ def test_manager(mycharm): def test_manager_implicit(mycharm): ctx = Context(mycharm, meta=mycharm.META) - with ManagedEvent(ctx, ctx.on.start(), State()) as manager: + with Manager(ctx, ctx.on.start(), State()) as manager: assert isinstance(manager.charm, mycharm) # do not call .run() @@ -49,7 +49,7 @@ def test_manager_implicit(mycharm): def test_manager_reemit_fails(mycharm): ctx = Context(mycharm, meta=mycharm.META) - with ManagedEvent(ctx, ctx.on.start(), State()) as manager: + with Manager(ctx, ctx.on.start(), State()) as manager: manager.run() with pytest.raises(AlreadyEmittedError): manager.run() @@ -66,6 +66,6 @@ def test_context_manager(mycharm): def test_context_action_manager(mycharm): ctx = Context(mycharm, meta=mycharm.META, actions=mycharm.ACTIONS) with ctx(ctx.on.action("do-x"), State()) as manager: - ao = manager.run_action() - assert isinstance(ao, ActionOutput) + state_out = manager.run() + assert isinstance(state_out, State) assert ctx.emitted_events[0].handle.kind == "do_x_action"