-
Notifications
You must be signed in to change notification settings - Fork 62
Created EventListenerIntrospectionTrait #19
Created EventListenerIntrospectionTrait #19
Conversation
This trait is intended to provide a forwards-compatibility shim for use when testing event listener registration. Methods mimic some of the functionality already in v2 (via `getEvents()` and `getListeners()`), but in such a way as to allow common behavior when testing the two versions. In particular, it provides the assertion method `assertListenerAtPriority()`, allowing developers to test that a listener was registered for the specified event at the specified priority; this is useful when testing attachment by aggregates or of default listeners.
|
||
namespace Zend\EventManager\Test; | ||
|
||
use PHPUnit_Framework_TestCase as TestCase; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be PHPUnit_Framework_Assert
This patch ports the changes from zendframework#19 to the develop branch, and adapts them for the v3 API.
EventManager $events, | ||
$message = '' | ||
) { | ||
$message = $message ?: sprintf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much logic in this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How so? Do you have any suggestions to improve it?
Currently, it does exactly the following:
- Sets a default message if none was provided
- Retrieves the listeners, segregated by priority
- Loops through the listeners to test if any match the specified conditions
- Asserts that it found a match
The total number of lines is 15. I'm not seeing how this is "too much logic", personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think this is fine, especially considering it is a private method it would easily allow things to be separated out if it gets more complex in the future. Right now this method has low cyclomatic complexity, I think the larger issue is that there is a ton of parameters but again, it's a private method so I feel there is no issue to really raise here.
I should point out that the point of this is to simplify testing in components that are registering listeners, such as zend-modulemanager. In many of these, when refactoring for version 3, we introduced similar traits so that we could introspect the state of the event manager after attaching listeners, as the methods for doing so ( In many cases, such tests did not exist prior to the refactors, which means that now, when we can attempt to see if the code works against both v2 and v3, we need traits such as this that can be re-used based on the event manager version in use. I'll be using this code going forward to refactor tests in the few components that use the event manager so that we can do v2 releases that will be forwards-compatible with zend-eventmanager v3. |
* Zend Framework (http://framework.zend.com/) | ||
* | ||
* @link http://github.com/zendframework/zend-eventmanager for the canonical source repository | ||
* @copyright Copyright (c) 2005-2015 Zend Technologies USA Inc. (http://www.zend.com) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's 2016 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can update that on merge; thanks for the heads-up! (copy-paste fail!)
This trait is intended to provide a forwards-compatibility shim for use when testing event listener registration. Methods mimic some of the functionality already in v2 (via
getEvents()
andgetListeners()
), but in such a way as to allow common behavior when testing the two versions.In particular, it provides the assertion method
assertListenerAtPriority()
, allowing developers to test that a listener was registered for the specified event at the specified priority; this is useful when testing attachment by aggregates or of default listeners.A related pull request will also be submitted against the develop branch.