Update event assertions

[xEdelweiss]

Hi! Currently, seeEventTriggered and dontSeeEventTriggered are actually checking if Event Listener was called.

So I've marked these methods as deprecated and created new ones for the Listeners with more intuitive names:

• seeEventListenerCalled
• dontSeeEventListenerCalled

Additionally, I've added methods to check if Events were dispatched (named them in a way like seeOrphanEvent):

• seeEvent
• dontSeeEvent

Tests can be updated if you approve this PR.

[xEdelweiss]

@TavoNiievez, I just realized (stumbled on this in my tests) that it would be nice to have a method that checks if an event was dispatched regardless of whether it has listeners or not (if it is orphaned).

What do you think about this? Should I update seeEvent/dontSeeEvent to check both types of events?
Or add something like seeSubscribedEvent/dontSeeSubscribedEvent?

[TavoNiievez]

Hi @xEdelweiss,

I totally agree, in #147 it was already established that the names were not correct, and I'm glad to see that someone has been able to come up with a much better approach :).

In general I see quite well what you did in your PR, my only observations are the following:

1. Since in Codeception it is important that the method names (assertions) can be written just like in natural language, I think you could name these methods like
   seeEventListenerWasCalled and dontSeeEventListenerWasCalled, I think adding the 'Was' here would be fine.

2. I agree with adding the new methods you indicate: seeEvent and dontSeeEvent and it would be great if you could add tests for them, but I would ask you to add them in a different PR (I will merge both, don't worry) but this way it will be easier to review the changes in the future. (i.e. in one PR you modify the names, and in another you add the new methods.)
   Are you ok with doing it this way?

3. One last thing. I see that the way you deprecated the old methods was with the @deprecated annotation, while it is an option, it seems to me that when someone runs the tests it won't pop up any warning that those methods are deprecated and will be removed in the future (usually the next major version of the module), I'm not sure if @Naktibalda can tell us if in Codeception there is a standard way to deprecate things like this that we can use, or if using the annotation would be the right thing to do.

Thanks!

[ThomasLandauer]

Just some thoughts from me. I don't use events that much, so I can't come up with a real suggestion.

• Answer to Fix event name assertion #147 (comment)

  When I added these functions I was more concerned with making the name of the functions match what you can find in the other Framework modules of Codeception.

  Consistency across framework modules is not so important IMO, since most users will probably just use one of them ;-)

• Whenever I see "listener", I ask myself: "Are subscribers included or not?"

• Talking about subscribers: Such a class can contain many events, right? See https://symfony.com/doc/current/event_dispatcher.html#creating-an-event-subscriber Is this covered here? Maybe add an optional second argument?

• About the orphaned events: Maybe a unified method for everything: event name as first argument, and listener/subscriber class as optional second argument? Does this make sense?

• seeEventListenerWasCalled If you want to add a verb, it should be is, since everything else is present tense too:
  seeResponseCodeIs, seeEmailIsSent,...

[Naktibalda]

3. I'm not sure if @Naktibalda can tell us if in Codeception there is a standard way to deprecate things like this that we can use, or if using the annotation would be the right thing to do.

trigger_error('message', E_USER_DEPRECATED);

Deprecation messages are counted and printed below test execution results unless:
a) error_level setting excludes E_USER_DEPRECATED,
b) convert_deprecations_to_exceptions setting makes the test fail.

[xEdelweiss]

Hi! Thank you all for your feedback. I'll start with the deprecations.

I think adding the 'Was' here would be fine

@TavoNiievez good point. Also, Given @ThomasLandauer it seems a good idea to name them using "is" for consistency.

Whenever I see "listener", I ask myself: "Are subscribers included or not?"

@ThomasLandauer, that's the question I was asking myself. I have got the impression that in Symfony code, listeners are more often called with the Listener suffix (although they do implement EventSubscriberInterface), so I think this name will be more understandable.

To continue the current discussion about new methods I've added another PR that includes only methods deprecation and adds new names for them: #169.

[xEdelweiss]

Talking about subscribers: Such a class can contain many events, right? See https://symfony.com/doc/current/event_dispatcher.html#creating-an-event-subscriber Is this covered here? Maybe add an optional second argument?

@ThomasLandauer I agree, it would be nice to cover this. There is one question though: how it should be handled if the user passes multiple event listeners:

$I->seeEventListenerIsCalled([CalledListenerOne::class, CalledListenerTwo::class], 'event.name');
$I->seeEventListenerIsCalled([CalledListenerOne::class, CalledListenerTwo::class], ['event_one.name', 'event_two.name']

Should it check that all events are handled by all listeners? It may seem confusing.

Or we can disallow passing multiple listeners and require to call seeEventListenerIsCalled multiple times.

$I->seeEventListenerIsCalled(CalledListenerOne::class, ['event_one.name', 'event_two.name']
$I->seeEventListenerIsCalled(CalledListenerTwo::class, ['event_one.name', 'event_two.name']

About the orphaned events: Maybe a unified method for everything: event name as first argument, and listener/subscriber class as the optional second argument? Does this make sense?

In my code, I check that some component dispatches an event and often it can be done before the listener/subscriber is implemented.
That's why I would like to keep seeEvent and seeOrphanEvent separated.

[TavoNiievez]

Or we can disallow passing multiple listeners and require to call seeEventListenerIsCalled multiple times.

It would only occur to me to write that as an associative array:

$I->seeEventListenerIsCalled([
    CalledListenerOne::class => ['event_one.name', 'event_two.name'],
    CalledListenerTwo::class => ['event_three.name', 'event_four.name'],
]);

But it seems to me that it would be more complex than necessary, both in implementation and reading.
Not allowing multiple listeners seems like a better option to me.

That's why I would like to keep seeEvent and seeOrphanEvent separated.

I also agree.

[ThomasLandauer]

There is one question though: how it should be handled if the user passes multiple event listeners:

I also agree to disallow this. Most Codeception methods are pretty straightforward. Introducing some fancy array syntax only makes it more complicated than necessary. Even more so, as a super easy alternative is available:
If you need it twice, call it twice ;-)

[TavoNiievez]

Hi @xEdelweiss ,

I have updated your branch with the latest changes present in the main branch of this repository, plus I have added a small fix for an incorrect method call.

Currently your PR is doing several things at once:
It is adding 4 new methods and some protected methods to make them work.
It is swapping out the deprecated code warnings.

Also, the seeEvent and dontSeeEvent methods have incorrect comment block. They also need tests in the test project to verify their operation.

Please create different PRs for these tasks in order to continue.

[TavoNiievez]

@xEdelweiss remember that the agreed names for the new methods are:

• seeEventListenerIsCalled
• dontSeeEventListenerIsCalled

currently in your PR are added two additional methods named:

• seeEventListenerCalled
• dontSeeEventListenerCalled

in addition to the first two already mentioned.
Please remove these methods.

[xEdelweiss]

Hi @TavoNiievez,

I apologize for my absence and the slight mess in the commits history.

As we discussed, I have implemented the possibility to check if a listener was triggered for a specific event(s) within the seeEventListenerIsCalled/dontSeeEventListenerIsCalled methods:

$I->seeEventListenerIsCalled(CalledListenerOne::class, 'event_one.name');
$I->seeEventListenerIsCalled(CalledListenerTwo::class, ['event_one.name', 'event_two.name']);
// and the same for dontSeeEventListenerIsCalled

Additionally, I have removed the seeEvent/dontSeeEvent methods to enhance the clarity of this PR.

Please remove these methods.

These were removed as well.

I've updated the tests, and I'm ready to create a PR for it when required.

[TavoNiievez]

Hi @xEdelweiss ,

Don't worry about the git history while working on this PR. In any case it will be merged to the main branch of this repository with the 'Squash and merge' strategy so the main repo will only get a single commit.

At the moment I have fixed some tests that were failing in the test project (symfony-module-tests), I will review the changes you have added as soon as possible.

Thanks.