[4.0] support for custom events

[PietroPasotti]

Fixes #38

Added custom event API:

    class FooEvent(EventBase):
        pass

    class MyObjEvents(CharmEvents):
        foo = EventSource(FooEvent)

    class MyObject(Object):
        my_on = MyObjEvents()

    class MyCharm(CharmBase):
        def __init__(self, *args, **kwargs):
            super().__init__(*args, **kwargs)
            self.obj = MyObject(self, "child")
            self.framework.observe(self.obj.my_on.foo, self._on_foo)

    trigger(
        State(),
        "foo",
        MyCharm,
        owner_path=('obj', 'my_on')   # <<<--- this will tell Scenario where to find the event we're trying to emit
    )

[gruyaume]

From a user / tester point of view it would be great not to have to tell scenario about those paths. We are already telling scenario about the charm that we want to use and this charm observes some custom events. It feels like scenario already has all the info it needs.

[PietroPasotti]

From a user / tester point of view it would be great not to have to tell scenario about those paths. We are already telling scenario about the charm that we want to use and this charm observes some custom events. It feels like scenario already has all the info it needs.

That however does not generalize: it might be the charm lib the one that observes the custom event, not the charm. So the charm might be totally unaware of the custom event. What do we do then? Recursively search all child Objects for an event with that name? What if there are conflicts?

We could make some cautious assumptions, of course, so that this situation is still nicely surfaced but we handle the common case automagically.

Another, completely different approach to implementing this could be to skip altogether the Framework.emit() machinery by pointing out manually the listener you want to exercise. This is somewhat complementary perhaps, because it could be that you're interested in testing all listeners (however many or dynamically defined), instead of just a predetermined one.

ctx = Context(MyCharm)
ctx.call_method(MyCharm._on_my_custom_event, args=(MyCustomEventType(MyMockedArg1, MyMockedArg2)))

WDYT?

[gruyaume]

From a user / tester point of view it would be great not to have to tell scenario about those paths. We are already telling scenario about the charm that we want to use and this charm observes some custom events. It feels like scenario already has all the info it needs.

That however does not generalize: it might be the charm lib the one that observes the custom event, not the charm. So the charm might be totally unaware of the custom event. What do we do then? Recursively search all child Objects for an event with that name? What if there are conflicts?

We could make some cautious assumptions, of course, so that this situation is still nicely surfaced but we handle the common case automagically.

Another, completely different approach to implementing this could be to skip altogether the Framework.emit() machinery by pointing out manually the listener you want to exercise. This is somewhat complementary perhaps, because it could be that you're interested in testing all listeners (however many or dynamically defined), instead of just a predetermined one.

ctx = Context(MyCharm)
ctx.call_method(MyCharm._on_my_custom_event, args=(MyCustomEventType(MyMockedArg1, MyMockedArg2)))

WDYT?

I agree that it's possible and often the case that the listening of the event is done in the charm library file but my point is that in the end, it's still the charm that observes it regardless in which "file" / "part of the code" it's done.

Listening to event is always done in the charm's init, either directly or by instantiating other "Object" objects right? I wouldn't necessarily search all file recursively from the get go, since there can be a bunch of events that the charm is not listening to in the libs if those classes aren't instantiated. I think I'd start by whatever the charm is instantiating (?).

And yes calling the charm's _on_my_custom_event can be an option, it's what we do with harness. My only concern with this approach is that it bypasses the event trigger and it's possible for the handler to work perfectly well but never be called (if it's not instantiated in the charm init) and have the illusion that all works well.

[PietroPasotti]

I agree that it's possible and often the case that the listening of the event is done in the charm library file but my point is that in the end, it's still the charm that observes it regardless in which "file" / "part of the code" it's done.

No my point is that we can't assume that this is the case.
Suppose you have this setup:

• MyCharm instantiates MyLib and listens to MyLib.on.ready
• MyLib instantiates MyOtherObj and listens to MyOtherObj.on.something, using it to trigger MyLib.on.ready

How do we test MyOtherObj.on.something? It's not in MyCharm._observers, but in MyLib._observers: we have no way to know who owns that event.

Somewhat simpler case:
Suppose some evil character defines a custom event called start: MyLib.on.start.
ctx.run(MyCharm, 'start') will match the 'wrong' event now. How do we tell scenario that no, we mean that 'other' start event?

[PietroPasotti]

And yes calling the charm's _on_my_custom_event can be an option, it's what we do with harness. My only concern with this approach is that it bypasses the event trigger and it's possible for the handler to work perfectly well but never be called (if it's not instantiated in the charm init) and have the illusion that all works well

The same point could be made about exercising custom events in isolation in the first place: maybe the code leading up to that event being emitted is buggy, and the event is not emitted when we think it should be.
That's why usually I recommend to construct the state in such a way that is emitted 'the normal way'

[benhoyt]

I agree with @PietroPasotti that we should be explicit here. However, I don't love the proposed API -- having a separate event_owner_path arg (with a long name) that's a tuple of strings seems awkward.

I wonder if we can do something more like unittest.mock does when you patch something. The patch target can be a dotted name that it looks up using pkgutil.resolve_name, for example @patch('time.sleep'). Example of resolve_name:

>>> pkgutil.resolve_name('ops.CharmBase.on.config_changed')
<BoundEvent ConfigChangedEvent bound to CharmEvents.config_changed at 0x7fa7e183fa60>

(The target-getting code in unittest.mock looks up the first part using pkgutil.resolve_name and the last element is the attribute.)

My thinking is we'd just use the existing event: str parameter, and if it has a . in it, use the dotted-lookup approach; if not, search the regular places (I think just charm.on right now). I'm not sure we can directly use pkgutil.resolve_name, but worth considering (for consistency with unittest.mock). If not, we should be able to fairly easily use a getattr loop like you're doing already. The example would become something like this:

trigger(State(), 'MyCharm.obj.my_on.foo', meta=...)

# or maybe
trigger(State(), 'obj.my_on.foo', meta=...)

Also in my mind is the new collect_status event that I'm working on, which will likely be an event not on CharmBase.on but on (a newly-added) Unit.on and App.on event source. So this would have to be something like:

trigger(State(), 'ops.Unit.on.collect_status')
  # or
trigger(State(), 'unit.on.collect_status')

I realize having event_owner_path as a separate parameter that's a tuple of strs is more "conceptually pure", but just an awkward API. Hence the reason they did it using dotted names in unittest.mock, I suppose.

---

Nit: I think owner_path should be event_owner_path in the PR description. (Though this parameter might go away if we do something like the above.)

[PietroPasotti]

trigger(State(), 'MyCharm.obj.my_on.foo', meta=...)

# or maybe
trigger(State(), 'obj.my_on.foo', meta=...)

Also in my mind is the new collect_status event that I'm working on, which will likely be an event not on CharmBase.on but on (a newly-added) Unit.on and App.on event source. So this would have to be something like:

trigger(State(), 'ops.Unit.on.collect_status')
  # or
trigger(State(), 'unit.on.collect_status')

I really like the idea, I'd thought about it as well but my main concern was (as with many 'string-based' approaches) the lack of type hinting and the burden of having to keep that in sync as your code changes. I don't like the idea of baking the charm name in the event emitter path for example.

I thought of a workaround, reminiscent of a piece of code I wrote long ago:

def get_event(charm_class: Type[T]) -> T:   # fake return type to leverage type hints and autocompletion
    """Helper class to construct an event path."""
    return _PathBuilder([])
    
class _PathBuilder:
    """Proxy class to construct a path of strings based on getattr."""
    path: List[str]
    def __getattr__(self, attr):
        return _PathBuilder(self.path + [attr])

This way, you'd be able to write

trigger(State(), get_event(MyCharm).foo.bar.on.baz, meta=...)

and enjoy autocompletion and type checking on the path to on.baz.

Pros:

• less validating and parsing to be done to help guard against user mistakes in building the path

Cons:

• magical, possibly counterintuitive API

[PietroPasotti]

trigger(State(), 'MyCharm.obj.my_on.foo', meta=...)

decided to go with this suggestion of Ben.