canonical / ops-scenario

State-transition testing SDK for Operator Framework Juju charms.
Apache License 2.0
10 stars 7 forks source link

feat!: add Scenario classes that match the ops status classes #142

Closed tonyandrewmeyer closed 2 months ago

tonyandrewmeyer commented 3 months ago

Adds classes that match the ops status classes:

These are comparable with (both == and isinstance) the ops classes, so this is a backwards compatible change (except for static checking), but the expectation is that instead of this code:

import ops, scenario

state_in = scenario.State(unit_status=ops.BlockedStatus("waiting for the DB"))
...
assert state_out.unit_status == ops.ActiveStatus()

you would write:

state_in = scenario.State(unit_status=scenario.BlockedStatus("waiting for the DB"))
...
assert state_out.unit_status == scenario.ActiveStatus()

Or, in a future time when the Scenario objects live in ops.testing:

import ops
import ops.testing as testing

state_in = testing.State(unit_status=testing.BlockedStatus("waiting for the DB"))
...
assert state_out.unit_status == testing.ActiveStatus()

In the same way that __post_init__ converts ops status classes to Scenario ones, it will also convert ops Port and Storage objects, and the Scenario classes gain an __eq__ so they can be used more interchangeably with the ops objects. They don't inherit from the ops classes, because isinstance seems unlikely to be useful in these cases, and that would bring in additional baggage.

(I originally planned to remove _EntityStatus in favour of using ops.StatusBase by getting the ops status classes converted to dataclasses, but the @canonical/charm-tech team decided that it would provide a more consistent experience to have the Scenario state only contain Scenario objects (and plain Python ones), and no ops ones.)

benhoyt commented 3 months ago

Thanks, @tonyandrewmeyer. Side note: I think one would usually write import ops.testing as testing as from ops import testing, right?

@PietroPasotti I realize you might disagree with this direction. Happy to hear your feedback and come to consensus (open to a voice discussion about this -- or anything else -- if needed).

tonyandrewmeyer commented 3 months ago

Side note: I think one would usually write import ops.testing as testing as from ops import testing, right?

Yes one would. It's actually slightly slower (looking with dis, I assume because the latter builds a tuple), but I just generally don't like the aesthetics of mixing "import x" and "from x import y", although it's certainly valid in this case. I can adjust the PR description if you prefer (and I would almost certainly write "from ops import testing" in docs, going for convention over personal preference).

@PietroPasotti I realize you might disagree with this direction. Happy to hear your feedback and come to consensus (open to a voice discussion about this -- or anything else -- if needed).

+1 (but "realise" :wink: ).

benhoyt commented 3 months ago

Haha, you have to reali(z|e) I'm an American Kiwi... :-)