Reworked pre/post event hooks --> welcome emitter

[PietroPasotti]

Fixes #53

from ops.charm import CharmBase

from scenario import Context, State


class MyCharm(CharmBase):
  META = {"name": "mycharm"}

  def __init__(self, framework):
    super().__init__(framework)
    self.a = "a"
    framework.observe(self.on.start, self._on_start)

  def _on_start(self, event):
    self.a = "b"


def test_live_charm_introspection(mycharm):
  ctx = Context(mycharm, meta=mycharm.META)
  # If you want to do this with actions, you can use `Context.action_emitter` instead.
  with ctx.emitter("start", State()) as emitter:
    # this is your charm instance, after ops has set it up
    charm = emitter.charm
    assert isinstance(charm, MyCharm)
    assert charm.a == "a"

    # this will tell ops.main to proceed with normal execution and emit the "start" event on the charm
    state_out = emitter.emit()

    # after that is done, we are handed back control and we can again do some introspection
    assert charm.a == "b"

  # state_out is, as in regular scenario tests, a State object you can assert on:
  assert state_out.unit_status == ...

• pre/post_event hooks still work, with a deprecation warning
• the implementation involves considerable changes to the return types of many internal structures, way too many generators for my liking, but I couldn't think of a better way to implement this (while maintaining BC).

[benhoyt]

Haven't looked at the code yet, but two questions first:

1. In the example in the PR description, should the emitter.emit() call and the assert charm.a == "b" line be indented inside the with statement?
2. Why the change from runner and runner.run() to emitter / emitter.emit()? I think it's better to use run() as it's consistent with existing ops terminology. Won't it be confusing that it's context.run() but emitter.emit()?

[PietroPasotti]

Haven't looked at the code yet, but two questions first:

1. In the example in the PR description, should the emitter.emit() call and the assert charm.a == "b" line be indented inside the with statement?

yes, totally. My bad

2. Why the change from runner and runner.run() to emitter / emitter.emit()? I think it's better to use run() as it's consistent with existing ops terminology. Won't it be confusing that it's context.run() but emitter.emit()?

I thought it would be less confusing to have separate terminology for the 'Context-context-manager' api.

I'm afraid it might be hard to tell Context.run from Context.runner.run: what is a 'context runner'?

Ideally I'd like a more descriptive name for what the context manager is doing, but "context" is already confusing because of the Context object, so I think Context.context() is a bad idea.

More ideas:

• Context.runner: too abstract?
• Context.emitter: too abstract?
• Context.step: it allows the user to 'step' into the event emission
• Context.manual: as opposed to context.run which is 'auto'
• Context.introspect: it allows the user to introspect the charm object
• Context.inspect_charm: idem
• Context.pre_emit_charm_hook: it gives the user a hook onto the charm right before the event is emitted

[benhoyt]

It's a little bit cutesy, but how about Context.manager()? (It's a context manager.) And then manager.run(). Whatever the name of the context-manager function, I'm fairly convinced the method name should be run to match Context.run.

Is there a reason to disallow multiple run() calls? Can you call run() twice now on a Context? For example:

with ctx.manager(State()) as manager:
    # ... pre
    manager.run("install")
    # assert stuff
    manager.run("start")
    # ... post

Or is that disallowed because it's against the Scenario spirit of "one event per test"?

[benhoyt]

I've just skimmed the implementation, but I'm wondering why generators need to be involved? Modifying main() to yield seems a bit strange. Seeing we've got full control over main anyway, could we instead break main() up into setup, emit, and cleanup parts, and then do something like this (very rough sketch):

class Manager:
    def __enter__(self):
        self.emit_event, self.cleanup = main.setup()
        return self

    def run(self):
        self.emit_event()

    def __exit__(self, exc_type, exc_val, exc_tb):
        self.cleanup()

The main.setup function would do the initial setup, create the charm, etc -- most of what main() does now except emitting the event and the final cleanup. It could return two callables: one that emits the event, and one that does cleanup.

Seems more direct to me, and no generators involved.

[PietroPasotti]

It's a little bit cutesy, but how about Context.manager()? (It's a context manager.) And then manager.run(). Whatever the name of the context-manager function, I'm fairly convinced the method name should be run to match Context.run.

very cheeky. Love it!

Is there a reason to disallow multiple run() calls? Can you call run() twice now on a Context? For example:

with ctx.manager(State()) as manager:
    # ... pre
    manager.run("install")
    # assert stuff
    manager.run("start")
    # ... post

Or is that disallowed because it's against the Scenario spirit of "one event per test"?

It is disallowed, and because of principles and because the Context keeps some state that needs to be cleaned up between event emissions (the juju log, emitted events). It's hard to reason about what these would mean if you have to second-guess how many juju events have been emitted in the lifetime of a context

[PietroPasotti]

I've just skimmed the implementation, but I'm wondering why generators need to be involved? Modifying main() to yield seems a bit strange. Seeing we've got full control over main anyway, could we instead break main() up into setup, emit, and cleanup parts, and then do something like this (very rough sketch):

class Manager:
    def __enter__(self):
        self.emit_event, self.cleanup = main.setup()
        return self

    def run(self):
        self.emit_event()

    def __exit__(self, exc_type, exc_val, exc_tb):
        self.cleanup()

The main.setup function would do the initial setup, create the charm, etc -- most of what main() does now except emitting the event and the final cleanup. It could return two callables: one that emits the event, and one that does cleanup.

Seems more direct to me, and no generators involved.

I did consider it, but was hesitant to modify ops.main more than strictly necessary.
Because

1. I don't want to introduce subtle bugs or discrepancies with ops because main is pretty complex and I break it up in the wrong way
2. if ops.main changes, we'd have to backport

But I agree that the generator stuff introduces a whole lot more complexity than necessary when we could bite the bullet and break main up instead.

[PietroPasotti]

I've just skimmed the implementation, but I'm wondering why generators need to be involved?

It's actually more complicated than breaking ops.main up in three sections. We need to be able to interrupt the execution not only of ops.main but also of context._exec. Not sure how to do that

[benhoyt]

Looks reasonable to me. Left a few minor comments.

We need to be able to interrupt the execution not only of ops.main but also of context._exec

I'm still a bit unsure of why we need generators. What do you mean by the above -- is that referring to Context._exec_ctx / Runtime.exec?

Either way, I'm not going to push back more about a slightly tricky implementation -- the user-facing API seems reasonable.

[PietroPasotti]

Looks reasonable to me. Left a few minor comments.

We need to be able to interrupt the execution not only of ops.main but also of context._exec

I'm still a bit unsure of why we need generators. What do you mean by the above -- is that referring to Context._exec_ctx / Runtime.exec?

Pardon, I meant Runtime.exec.

Either way, I'm not going to push back more about a slightly tricky implementation -- the user-facing API seems reasonable.

To be clear, I agree that the implementation is not nice and I'm totally up for a refactoring, I just have no idea how to do it better. I already spent quite a lot of time breaking my head on it but I'm not sure how to get it done in a nicer way. Will work on a MWE of the situation so we can see if we find a better pattern.

[PietroPasotti]

@benhoyt after pouring some more hours on the problem and reorganizing a bit the mocked ops main module, I came up with a solution where instead of everything being a generator, everything is a context manager.
At least there's less obscure magic involved, and I feel there might be some better abstractions at play (the Ops object, for instance).

[benhoyt]

Yeah, I think this implementation is more direct and easier to understand -- thanks!