charm state initial prototype

[PietroPasotti]

this PR adds to scenario charm-state mocking utilities.

Charm State

Suppose that your charm code makes an http call to a server somewhere to get some data, say, the current temperature reading from a sensor on top of the Nieuwe Kerk in Delft, The Netherlands.

If you follow the best practices of how to structure your charm code, then you are aware that this piece of data, at runtime, is categorised as 'charm state'.
Scenario offers a way to plug into this system natively, and integrate this charm state data structure into its own State tree.

If your charm code looks like this:

from dataclasses import dataclass
from ops import CharmBase, Framework

from scenario.charm_state import CharmStateBackend
from scenario.state import CharmState

# in state.py
@dataclass(frozen=True)
class MyState(CharmState):
    temperature: float = 4.5  # brr

# in state.py
class MyCharmStateBackend(CharmStateBackend):
    @property
    def temperature(self) -> int:
        import requests
        return requests.get('http://nieuwekerk.delft.nl/temp...').json()['celsius']
    
    # no setter: you can't change the weather. 
    # ... Can you?
    
# in charm.py
class MyCharm(CharmBase):
    state = MyCharmStateBackend()

    def __init__(self, framework: Framework):
        super().__init__(framework)
        self.temperature = self.state.temperature

Then you can write scenario tests like that:

import pytest
from scenario import Context, State
from charm import MyCharm
from state import MyState

@pytest.fixture
def ctx():
    return Context(MyCharm, meta={"name": "foo"})


@pytest.mark.parametrize("temp", (1.1, 10.2, 20.3))
def test_get(ctx, temp):
    state = State(charm_state=MyState("state", temperature=temp))

    # the charm code will get the value from State.charm_state.temperature instead of making http calls at test-time.
    def post_event(charm: MyCharm):
        assert charm.temperature == temp

    ctx.run("start", state=state, post_event=post_event)

[jdkandersson]

This looks good, I suppose what it is doing is somehow patching the .state?

Minor side note, I remember we discussed a preference not to put a HTTP call on a @property? Perhaps we can pick a different example, like a value that would have been read from a workload?

[PietroPasotti]

This looks good, I suppose what it is doing is somehow patching the .state?

Minor side note, I remember we discussed a preference not to put a HTTP call on a @property? Perhaps we can pick a different example, like a value that would have been read from a workload?

Yeah I agree it's not a good pattern, but the design of the CharmStateBackend object relies on every piece of state being property-like, because it needs to map to a CharmState (which is a dataclass nested in State).
The CharmStateBackend uses the properties defined on it to determine what attributes can be mapped to CharmState values (and the presence of a @prop.setter to determine whether it's writeable or read-only).
It's quite lean I think, and it fits well with the current State monolith, so I'd avoid introducing a totally different mechanism for handling CharmState.

If making requests in a property hurts our eyes, we can write the example like:

import requests
def get_temperature():
     return requests.get('http://nieuwekerk.delft.nl/temp...').json()['celsius']

class MyCharmStateBackend(CharmStateBackend):
    @property
    def temperature(self) -> int:
          return get_temperature()

which if you squint still hurts your eyes, but it's a bit less in your face.

Do you see better alternatives?

[jdkandersson]

Yeah I agree it's not a good pattern, but the design of the CharmStateBackend object relies on every piece of state being property-like, because it needs to map to a CharmState (which is a dataclass nested in State). The CharmStateBackend uses the properties defined on it to determine what attributes can be mapped to CharmState values (and the presence of a @prop.setter to determine whether it's writeable or read-only). It's quite lean I think, and it fits well with the current State monolith, so I'd avoid introducing a totally different mechanism for handling CharmState.

If making requests in a property hurts our eyes, we can write the example like:

import requests
def get_temperature():
     return requests.get('http://nieuwekerk.delft.nl/temp...').json()['celsius']

class MyCharmStateBackend(CharmStateBackend):
    @property
    def temperature(self) -> int:
          return get_temperature()

which if you squint still hurts your eyes, but it's a bit less in your face.

Do you see better alternatives?

I'm ok with either one, just thought I would ask the question

[benhoyt]

If I'm understanding this correctly, this would require you to write your charm in a specific way, with properties for all charm-state attributes (would it also require charm.state attribute?). I think it's too opinionated / constraining, and the issue that @jdkandersson pointed out of having HTTP requests and things behind properties is another issue.

So I'm not a fan -- I think it's better to have ops-scenario only try to mock ops stuff, and let the charmer mock their own stuff and structure their charm state the way they want. The charmer can either mock/patch things like this, or use dependency injection to make things more explicit (e.g., in this example, passing in a requests session or passing in a get_temperature callable).

[PietroPasotti]

If I'm understanding this correctly, this would require you to write your charm in a specific way, with properties for all charm-state attributes (would it also require charm.state attribute?). I think it's too opinionated / constraining,

This prototype is based on the ISD014 - Managing Charm Complexity spec that was introduced/discussed in Prague. The spec proposes to have a separate 'state' class for charms which mediates all interactions between the charm and 'the world' outside of juju and pebble. This implementation indeed assumes that the charm will be written according to that spec (or that the author is willing to write an ad-hoc facade to present to Scenario as-if it did actually implement the spec).

If the spec is accepted, in its present form, I think having the ability in scenario to 'natively' integrate with it in this way would be very valuable.

and the issue that @jdkandersson pointed out of having HTTP requests and things behind properties is another issue.

So I'm not a fan -- I think it's better to have ops-scenario only try to mock ops stuff, and let the charmer mock their own stuff and structure their charm state the way they want. The charmer can either mock/patch things like this, or use dependency injection to make things more explicit (e.g., in this example, passing in a requests session or passing in a get_temperature callable).

Like the idea to pass in callbacks, but I'm not convinced that ops-scenario should only worry about ops stuff.
I think the power of the concept of State is that it's a single data structure containing ALL inputs and outputs a charm can be expected to read/write at runtime. My goal is to get to a point where charm testing code needs to do (near) zero custom mock/patching, and all goes through scenario.State. I think that would make for a much easier way to conceptualize what's going on in your charm code and test it.

[PietroPasotti]

(would it also require charm.state attribute?).

The present implementation allows you to customize that attribute (it's the first argument to CharmState), but that's all implementation details.

[benhoyt]

Thanks, that's a fair point of view. I'm going to take another look at ISD014 - Managing Charm Complexity. I'm a bit wary of this approach, though -- my understanding was that that spec would be a recommendation of how to structure complex charms, not something we'd make opinionated in the tooling.

[PietroPasotti]

I like opinionated tooling :)
The reasoning was: if you follow that spec and structure your charm according to that structure, then there is a testing plugin ready for you to use. It's a way to encourage the pattern, as well as make life easier for those who would use it either way.
And personally I'm not convinced only 'complex' charms would benefit from having an externalized CharmState interface in the way the spec proposes; all charms are sufficiently complicated even in the simple cases, and having a uniform structure like this facilitates testing, maintenance, and understandability.

But I agree that given the drafty status of the spec, it may be premature to have tooling whose opinions are stronger than those of the spec itself.