Skip to content

Commit

Permalink
Flatten the action results and logs, per review.
Browse files Browse the repository at this point in the history
  • Loading branch information
tonyandrewmeyer committed Aug 8, 2024
1 parent a156f81 commit c70af32
Show file tree
Hide file tree
Showing 7 changed files with 83 additions and 109 deletions.
23 changes: 12 additions & 11 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
```
Expand Down Expand Up @@ -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():
Expand All @@ -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
Expand All @@ -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()
)
Expand Down
3 changes: 1 addition & 2 deletions scenario/__init__.py
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -39,7 +39,6 @@
)

__all__ = [
"ActionOutput",
"ActionFailed",
"CheckInfo",
"CloudCredential",
Expand Down
69 changes: 15 additions & 54 deletions scenario/context.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -22,7 +21,6 @@
_Action,
_CharmSpec,
_Event,
_max_posargs,
)

if TYPE_CHECKING: # pragma: no cover
Expand All @@ -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"""

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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
Expand Down
13 changes: 6 additions & 7 deletions scenario/mocking.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion scenario/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 1 addition & 23 deletions tests/test_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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 == ""
58 changes: 47 additions & 11 deletions tests/test_e2e/test_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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"}))
Expand All @@ -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():
Expand All @@ -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):
Expand Down Expand Up @@ -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", {})
Expand Down

0 comments on commit c70af32

Please sign in to comment.