From c70af326841039c0ff281169c3fa9f83c134e837 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 8 Aug 2024 14:38:33 +1200 Subject: [PATCH] Flatten the action results and logs, per review. --- README.md | 23 ++++++------ scenario/__init__.py | 3 +- scenario/context.py | 69 ++++++++-------------------------- scenario/mocking.py | 13 +++---- scenario/state.py | 2 +- tests/test_context.py | 24 +----------- tests/test_e2e/test_actions.py | 58 ++++++++++++++++++++++------ 7 files changed, 83 insertions(+), 109 deletions(-) diff --git a/README.md b/README.md index c6aac364..227dd0fa 100644 --- a/README.md +++ b/README.md @@ -705,9 +705,9 @@ check doesn't have to match the event being generated: by the time that Juju sends a pebble-check-failed event the check might have started passing again. ```python -ctx = scenario.Context(MyCharm, meta={"name": "foo", "containers": {"my-container": {}}}) +ctx = scenario.Context(MyCharm, meta={"name": "foo", "containers": {"my_container": {}}}) check_info = scenario.CheckInfo("http-check", failures=7, status=ops.pebble.CheckStatus.DOWN) -container = scenario.Container("my-container", check_infos={check_info}) +container = scenario.Container("my_container", check_infos={check_info}) state = scenario.State(containers={container}) ctx.run(ctx.on.pebble_check_failed(info=check_info, container=container), state=state) ``` @@ -976,17 +976,16 @@ def test_backup_action(): # the `ConsistencyChecker` will slap you on the wrist and refuse to proceed. state = ctx.run(ctx.on.action("do_backup"), scenario.State()) - # You can assert action results and logs using the action history: - assert ctx.action_output.logs == ['baz', 'qux'] - assert ctx.action_output.results == {'foo': 'bar'} + # You can assert on action results and logs using the context: + assert ctx.action_logs == ['baz', 'qux'] + assert ctx.action_results == {'foo': 'bar'} ``` ## Failing Actions If the charm code calls `event.fail()` to indicate that the action has failed, an `ActionFailed` exception will be raised. This avoids having to include -`assert ctx.action_output.status == "completed"` code in every test where -the action is successful. +success checks in every test where the action is successful. ```python def test_backup_action_failed(): @@ -995,10 +994,12 @@ def test_backup_action_failed(): with pytest.raises(ActionFailed) as exc_info: ctx.run(ctx.on.action("do_backup"), scenario.State()) assert exc_info.value.message == "sorry, couldn't do the backup" + # The state is also available if that's required: + assert exc_info.value.state.get_container(...) - # You can still assert action results and logs that occured before the failure: - assert ctx.action_output.logs == ['baz', 'qux'] - assert ctx.action_output.results == {'foo': 'bar'} + # You can still assert action results and logs that occured as well as the failure: + assert ctx.action_logs == ['baz', 'qux'] + assert ctx.action_results == {'foo': 'bar'} ``` ## Parametrized Actions @@ -1011,7 +1012,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( + state = ctx.run( ctx.on.action("do_backup", params={'a': 'b'}), scenario.State() ) diff --git a/scenario/__init__.py b/scenario/__init__.py index 6023ed57..24c1cac0 100644 --- a/scenario/__init__.py +++ b/scenario/__init__.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 # Copyright 2023 Canonical Ltd. # See LICENSE file for licensing details. -from scenario.context import ActionOutput, Context, Manager +from scenario.context import Context, Manager from scenario.state import ( ActionFailed, ActiveStatus, @@ -39,7 +39,6 @@ ) __all__ = [ - "ActionOutput", "ActionFailed", "CheckInfo", "CloudCredential", diff --git a/scenario/context.py b/scenario/context.py index 998274fd..918e7213 100644 --- a/scenario/context.py +++ b/scenario/context.py @@ -1,7 +1,6 @@ #!/usr/bin/env python3 # Copyright 2023 Canonical Ltd. # See LICENSE file for licensing details. -import dataclasses import tempfile from contextlib import contextmanager from pathlib import Path @@ -22,7 +21,6 @@ _Action, _CharmSpec, _Event, - _max_posargs, ) if TYPE_CHECKING: # pragma: no cover @@ -38,47 +36,6 @@ DEFAULT_JUJU_VERSION = "3.4" -@dataclasses.dataclass(frozen=True) -class ActionOutput(_max_posargs(0)): - """Wraps the results of running an action event on a unit. - - Tests should generally not create instances of this class directly, but - rather use the :attr:`Context.action_output` attribute to inspect the - results of running actions. - """ - - 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`.""" - - 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 update_results(self, results: Dict[str, Any]): - """Update the results of the action.""" - if self.results is None: - # bypass frozen dataclass - object.__setattr__(self, "results", results) - else: - self.results.update(results) - - class InvalidEventError(RuntimeError): """raised when something is wrong with the event passed to Context.run""" @@ -370,6 +327,10 @@ class Context: - :attr:`unit_status_history`: record of the unit statuses the charm has set - :attr:`workload_version_history`: record of the workload versions the charm has set - :attr:`emitted_events`: record of the events (including custom) that the charm has processed + - :attr:`action_logs`: logs associated with the action output, set by the charm with + :meth:`ops.ActionEvent.log` + - :attr:`action_results`: 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` This allows you to write assertions not only on the output state, but also, to some extent, on the path the charm took to get there. @@ -496,7 +457,9 @@ def __init__( self._output_state: Optional["State"] = None # operations (and embedded tasks) from running actions - self.action_output: Optional[ActionOutput] = None + self.action_logs: List[str] = [] + self.action_results: Optional[Dict[str, Any]] = None + self._action_failure_message: Optional[str] = None self.on = _CharmEvents() @@ -563,19 +526,17 @@ def run(self, event: "_Event", state: "State") -> "State": charm will invoke when handling the Event. """ if event.action: - # Create an ActionOutput object now so that there is somewhere to - # store the logs and results while the charm is processing the - # action handler(s). This is not accessible until run() finishes and - # the handlers have finished. - self.action_output = ActionOutput() + # Reset the logs, failure status, and results, in case the context + # is reused. + self.action_logs.clear() + 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() if event.action: - current_task = self.action_output - assert current_task is not None - if current_task.status == "failed": - raise ActionFailed(current_task.failure_message, self.output_state) - current_task.set_status("completed") + if self._action_failure_message is not None: + raise ActionFailed(self._action_failure_message, self.output_state) return self.output_state @contextmanager diff --git a/scenario/mocking.py b/scenario/mocking.py index 8d400c78..f33a9512 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -528,25 +528,24 @@ 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 - assert self._context.action_output is not None - self._context.action_output.update_results(results) + if self._context.action_results: + self._context.action_results.update(results) + else: + self._context.action_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", ) - assert self._context.action_output is not None - self._context.action_output.set_status("failed") - self._context.action_output.set_failure_message(message) + self._context._action_failure_message = 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", ) - assert self._context.action_output is not None - self._context.action_output.logs.append(message) + self._context.action_logs.append(message) def action_get(self): action = self._event.action diff --git a/scenario/state.py b/scenario/state.py index 5d18842d..9773b23b 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -1853,7 +1853,7 @@ def test_backup_action(): ctx.on.action('do_backup', params={'filename': 'foo'}), scenario.State() ) - assert ctx.action_output.results == ... + assert ctx.action_results == ... """ name: str diff --git a/tests/test_context.py b/tests/test_context.py index 62d3e854..361b4543 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 from scenario.state import _Event, next_action_id @@ -68,25 +68,3 @@ def test_context_manager(): with ctx(ctx.on.action("act"), state) as mgr: mgr.run() assert mgr.charm.meta.name == "foo" - - -def test_action_output_no_positional_arguments(): - with pytest.raises(TypeError): - ActionOutput(None) - - -def test_action_output_no_results(): - class MyCharm(CharmBase): - def __init__(self, framework): - super().__init__(framework) - framework.observe(self.on.act_action, self._on_act_action) - - def _on_act_action(self, _): - pass - - ctx = Context(MyCharm, meta={"name": "foo"}, actions={"act": {}}) - ctx.run(ctx.on.action("act"), State()) - action_output = ctx.action_output - assert action_output.results is None - assert action_output.status == "completed" - assert action_output.failure_message == "" diff --git a/tests/test_e2e/test_actions.py b/tests/test_e2e/test_actions.py index a6c335bc..7b6d1727 100644 --- a/tests/test_e2e/test_actions.py +++ b/tests/test_e2e/test_actions.py @@ -41,6 +41,21 @@ def test_action_event(mycharm, baz_value): assert evt.params["baz"] is baz_value +def test_action_no_results(): + class MyCharm(CharmBase): + def __init__(self, framework): + super().__init__(framework) + framework.observe(self.on.act_action, self._on_act_action) + + def _on_act_action(self, _): + pass + + ctx = Context(MyCharm, meta={"name": "foo"}, actions={"act": {}}) + ctx.run(ctx.on.action("act"), State()) + assert ctx.action_results is None + assert ctx.action_logs == [] + + @pytest.mark.parametrize("res_value", ("one", 1, [2], ["bar"], (1,), {1, 2})) def test_action_event_results_invalid(mycharm, res_value): def handle_evt(charm: CharmBase, evt: ActionEvent): @@ -68,8 +83,7 @@ def handle_evt(_: CharmBase, evt): ctx.run(ctx.on.action("foo"), State()) - assert ctx.action_output.results == res_value - assert ctx.action_output.status == "completed" + assert ctx.action_results == res_value @pytest.mark.parametrize("res_value", ({"a": {"b": {"c"}}}, {"d": "e"})) @@ -86,14 +100,11 @@ def handle_evt(_: CharmBase, evt: ActionEvent): mycharm._evt_handler = handle_evt ctx = Context(mycharm, meta={"name": "foo"}, actions={"foo": {}}) - with pytest.raises(ActionFailed) as out: + with pytest.raises(ActionFailed) as exc_info: ctx.run(ctx.on.action("foo"), State()) - assert out.value.message == "failed becozz" - task = ctx.action_output - assert task.results == {"my-res": res_value} - assert task.logs == ["log1", "log2"] - assert task.failure_message == "failed becozz" - assert task.status == "failed" + assert exc_info.value.message == "failed becozz" + assert ctx.action_results == {"my-res": res_value} + assert ctx.action_logs == ["log1", "log2"] def test_action_continues_after_fail(): @@ -112,8 +123,8 @@ def _on_foo_action(self, event): with pytest.raises(ActionFailed) as exc_info: ctx.run(ctx.on.action("foo"), State()) assert exc_info.value.message == "oh no!" - assert ctx.action_output.logs == ["starting"] - assert ctx.action_output.results == {"initial": "result", "final": "result"} + assert ctx.action_logs == ["starting"] + assert ctx.action_results == {"initial": "result", "final": "result"} def _ops_less_than(wanted_major, wanted_minor): @@ -157,6 +168,31 @@ def handle_evt(charm: CharmBase, evt: ActionEvent): ctx.run(ctx.on.action("foo", id=uuid), State()) +def test_two_actions_same_context(): + class MyCharm(CharmBase): + def __init__(self, framework): + super().__init__(framework) + framework.observe(self.on.foo_action, self._on_foo_action) + framework.observe(self.on.bar_action, self._on_bar_action) + + def _on_foo_action(self, event): + event.log("foo") + event.set_results({"foo": "result"}) + + def _on_bar_action(self, event): + event.log("bar") + event.set_results({"bar": "result"}) + + ctx = Context(MyCharm, meta={"name": "foo"}, actions={"foo": {}, "bar": {}}) + ctx.run(ctx.on.action("foo"), State()) + assert ctx.action_results == {"foo": "result"} + assert ctx.action_logs == ["foo"] + # Not recommended, but run another action in the same context. + ctx.run(ctx.on.action("bar"), State()) + assert ctx.action_results == {"bar": "result"} + assert ctx.action_logs == ["bar"] + + def test_positional_arguments(): with pytest.raises(TypeError): _Action("foo", {})