7.0 rough edges - Githubissues

PietroPasotti commented 1 week ago

ease learning curve by providing helpful message on ctx.run(event_name, state). Proposal: if we get a string, raise TypeError and suggest passing "ctx.on.{event.replace(-, _)}()" instead. Complexity: events with a Container, Relation...
also better error message for ctx.run(ctx.on.start) (or could we consider auto-calling any callable passed to run?)
type hinting on state.unit_status appears to be bugged
just a thought: does the .on need to live on context? it feels a little bit awkward to do context.run(context.on...
- proposal 1: context.on.update_status(params).run(state)
- proposal 2: context.run(on.update_status(params), state)
autocompletion for most objects in State is broken since they're typed as Iterable but are in fact frozensets. I also expect this to have consequences for type checkers as they'll probably mark state.relations.union as an error.
manager.output should be removed

PietroPasotti commented 1 week ago

will keep updating this issue throughout my initial testing run

tonyandrewmeyer commented 1 week ago

Maybe ctx.run_action should raise AttributeError but with an explanation to use ctx.run(ctx.on.action(action_name), ...)? Maybe also pointing out where the action results are?
I thought Python would do this but it doesn't seem to: maybe Relation.relation_id should raise AttributeError suggesting .id?
~~Maybe if a dict is passed for networks it could suggest the presumed fix?~~ - not useful because to create the Network you already need to pass in the name, so you probably have figured this out already.

PietroPasotti commented 1 week ago

https://github.com/canonical/tempo-coordinator-k8s-operator/pull/30

tonyandrewmeyer commented 1 week ago

The telco ones I've done (these are PRs against my fork for now to avoid noise on their side until it's actually useful):

And one data-platform one:

https://github.com/tonyandrewmeyer/kafka-k8s-operator/pull/1

tonyandrewmeyer commented 1 week ago

type hinting on state.unit_status appears to be bugged

The issue here is that it's wanting you do do unit_status=scenario.ActiveStatus(). If you pass in an ops.ActiveStatus() then __post_init__ will convert it for you but the type checker doesn't know that. I don't think we want to make the type _EntityStatus | StatusBase.

I'm still not 100% convinced about these status classes. The rest of the charm-tech team were very strongly in favour of having them so that there's a distinction between Scenario objects and ops objects - e.g. you don't put an ops.Relation or ops.Container into State, so why are you putting in the ops status objects? I wasn't able to come up with a convincing argument against that, so conceded and tried to make it as easy as possible to use the ops classes anyway.

I think you were ok with the PR that added them at the time. Are you still? Otherwise, we can go back and argue against having these, although there does still need to be something that can be serialised to JSON so that the entire state can be (my original proposal was for that to happen in ops).

just a thought: does the .on need to live on context? it feels a little bit awkward to do context.run(context.on...

proposal 1: context.on.update_status(params).run(state)

I don't really like this. I feel like it loses the similarity to framework.observe(self.on.start, self._on_start) that we were going for.

proposal 2: context.run(on.update_status(params), state)

I also came up with this one. I had originally thought that CharmEvents would end up more like the ops CharmEvents, and that it would need to be linked to the Context object, but that's not (yet) the case. Everything is a staticmethod - the class is just a convenient way to bundle them all together.

I do think that this is pretty reasonable, and particularly when you're getting this from ops, like:

from ops.testing import Context, State, on

def test_foo():
    ctx = Context(MyCharm)
    state_in = State()
    state_out = ctx.run(on.start(), state_in)
    assert state_out...

We're not entirely wedded to what we choose now - we could deprecate ctx.on and add on = CharmEvents() in a later 7.x version without breaking anything. But it would be nicer to get it right now, of course.

I'll think about this some more and also talk about it with Ben when he's back next week.

For what it's worth, I didn't find it too bad when converting Scenario 6 tests, although I was doing a bunch of find/replace, and I am pretty used to the 7.0 API by now :laughing:. I do think it's worse when it's context or self.ctx rather than just ctx.

autocompletion for most objects in State is broken since they're typed as Iterable but are in fact frozensets. I also expect this to have consequences for type checkers as they'll probably mark state.relations.union as an error.

This is tricky to fix. I still strongly feel that they should be frozensets to get the immutability that the State is based on. However, passing frozensets when making the State is too ugly. In order to get the type checkers to not complain when you do State(relations={...}) the __init__ needs to be Iterable (or at least set|frozenset).

I wonder how much of an issue this is in practice. For the tests I've seen/updated/written, most of the time one of the get_ methods is being used, so you have exactly a Container or Relation or whatever. When I've used/seen the frozensets it's been things like iterating through them to fuzzy match (e.g. secret label starting with x) or a contains check, and both of those are fine with Iterable. Do you expect a lot of set operations?

If set operations are rare, then it doesn't seem too bad to put a typing.cast(frozenset, state.relations) into the test.

I think this can be fixed if State has a manually crafted __init__, where the __init__ arguments are all Iterable but the class attributes that the dataclass uses are frozenset. There would be a bunch of object.__setattr__ cruft in there, but I think the State class is the only one that we would need to do this for. I'm reluctant to do this at the moment.

My suggestion for this one is to leave it and see whether people do end up using the frozensets as sets - we can fix this in a backwards compatible way, I believe.

(Thanks for the other suggestions - I'm opening a PR with those).

PietroPasotti commented 1 week ago

nah I'm still fine with the decision about the status classes, having them in scenario... but it does make life hard typing-wise, will try to hack around a bit and see if maybe we can generate stub files containing the 'right' signatures for getters, while maintaining the input signature for the constructors. I did something similar some time ago

tonyandrewmeyer commented 6 days ago

proposal 2: context.run(on.update_status(params), state) [...] I'll think about this some more and also talk about it with Ben when he's back next week.

@benhoyt and I agree that it is a bit odd that the individual start(), relation_changed() etc methods are bundled into a class namespace (but are all static methods) and then that is also put into the Context namespace (which is also not used).

A top-level name does lose appeal when you do import scenario or import ops.testing or from ops import testing:

ctx.run(scenario.on.start(), state)
ctx.run(ops.testing.on.start(), state)
ctx.run(testing.on.start(), state)

If importing all the names individually, then you probably end up with a big list because you likely need at least Context, State, Container (if k8s), Secret, Relation, PeerRelation, and on for any reasonable set of tests. Ben & I both prefer to import the namespace rather than the individual objects, so the top-level on loses its advantage at that point.

My inclination is to leave it as Context.on for now, and Ben agreed with that. My understanding is that @PietroPasotti is also ok with it as-is for now. So I believe we're ready to merge 7.0 into main :tada:.

canonical / ops-scenario

7.0 rough edges #184