EventsAssertionsTrait refactor and Symfony 6.3 Support

[TavoNiievez]

This PR will be merged with the Rebase and Merge strategy.

Calls to $eventCollector->getCalledListeners(...) and $eventCollector->getOrphanedEvents(...) need to specify the default dispatcher since Symfony 6.3

The goals of the refactoring to EventsAssertionsTrait were mainly to reduce the number of protected methods needed to perform the assertions, as well as to eliminate duplicate code and reduce cyclomatic complexity.
I was able to reduce the file by 68 lines.

[TavoNiievez]

Hi @xEdelweiss ,

Could you please take a look at the refactored file EventsAssertionsTrait.php ?

Thank you!

[xEdelweiss]

Hi @TavoNiievez, yes, of course, I'll take a look.

[xEdelweiss]

Hi @TavoNiievez, I like the idea of removing $eventCollector stuff from each method.

However, assertEventTriggered without helper methods became a bit more complex, and it seems there is an issue - I will add a comment.

[TavoNiievez]

@xEdelweiss I agree that the methods were still complex, so I returned the eventWasTriggered and listenerWasCalled functions in the second foreach of the functions they belong to.

I am still working on the issue with seeEvent.

[TavoNiievez]

Hi @xEdelweiss I've added a test to verify that seeEvent doesn't work on non-existent events and then fixed the bug you identified that only called listeners were evaluated.
Here is the fixed version of EventsAssertionsTrait.php
Is everything ok this time?

[xEdelweiss]

Hi @TavoNiievez

Great, it's become much simpler without nested loops. Also, I like how assertListenersCalled is getting listeners for itself, missed this change previously.

I got a bit confused with how $actualEvents is working and while I tried to come up with some suggestions, I found that there is a possibility to make code even easier and unified.

Every event assertion requires orphan events data, and called listeners are optional. So assertEventTriggered can accept bool $orphansOnly param instead of $actual, and collect all that it needs by itself. Then there is no need to split $actual back to events/listeners:

-    protected function assertEventTriggered(bool $assertTrue, array|object|string|null $expected, array|Data $actual): void
+    protected function assertEventTriggered(bool $assertTrue, array|object|string|null $expected, bool $orphansOnly = true): void
     {
         $expected = is_array($expected) ? $expected : [$expected];
-        $actual = is_array($actual) ? $actual : [$actual];
+        $actualEvents = [
+            ...$this->getOrphanedEvents()->getValue(true),
+            ...($orphansOnly ? [] : array_column($this->getCalledListeners()->getValue(true), 'event')),
+        ];

-        $noEventsTriggered = array_sum(array_map('count', $actual)) === 0;
-        if ($assertTrue && $noEventsTriggered) {
+        if ($assertTrue && empty($actualEvents)) {
             $this->fail('No event was triggered');
         }

-        $actualEvents = array_merge(...array_map(static fn (Data $data) => $data->getValue(true), $actual));
-        $calledListeners = array_column($actualEvents, 'event');
-
         foreach ($expected as $expectedEvent) {
             $expectedEvent = is_object($expectedEvent) ? $expectedEvent::class : $expectedEvent;
-            $eventTriggered = in_array($expectedEvent, $calledListeners) || in_array($expectedEvent, $actualEvents);
+            $eventTriggered = in_array($expectedEvent, $actualEvents);
...

But, that's up to you. And I apologize, I couldn't resist coding - knowing when to stop a review is always a challenge for me. 🫣

Overall, I think the code already looks good and easy to read.

[TavoNiievez]

Hi @xEdelweiss
It was a super fast response.

I have partially implemented the code from your comment. I liked the ...(array_column($this->getCalledListeners()->getValue(true), 'event')) stuff.
Although I didn't write it in the proposed place since:

I was already thinking about an approach with something very similar $orphansOnly, but I undid it since a function with more than one parameter flag brings in the long run more challenges than benefits.
That's why I preferred to let the protected functions work with the data assuming it was sent correctly and not to add an additional responsibility by making them decide which data to work with since they already did so much.

Also, while working on this I realized that the fact that these functions can receive an instance of the event or listener does not make any kind of sense. That probably happened because the 2020 me was very happy with the fact that these assertions would work with whatever you passed them as a parameter.

Now I can't think of a use case for that specific behavior to be allowed. Especially because if you create an instance of your event in the tests it is very easy to add ::class at the end of the instance and the assertion will work just as well.

That's why I removed object from the examples of the new assertions.
In the next major version of this module object will also be removed from the method signatures and can only be used with strings or arrays of strings.
This way I save having to write $expectedEvent = is_object($expectedEvent) ? $expectedEvent::class : $expectedEvent; in multiple places.

If the changes from the last commit seem OK to you I will merge this and tag a new minor version.

Thank you!

[xEdelweiss]

It looks very good!

I'm really glad to have been able to help and will be looking forward to seeing the new version in action.
Also, if you ever need a fresh pair of eyes for code review, feel free to reach out.

Thank you for the insightful discussion!

[TavoNiievez]

Released as version 3.2.0 🎉.

thank you very much for your contributions, time and support!