-
Notifications
You must be signed in to change notification settings - Fork 25
Add notifier assertions trait #220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add notifier assertions trait #220
Conversation
TavoNiievez
left a comment
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.
@d-mitrofanov-v Thank you for taking the time to add these new assertions!
All of these assertions were included in Symfony 6.2, and are not available in Symfony 5.4.
This module maintains the same support as the supported versions of Symfony, so we must ensure compatibility with Symfony 5.4 while it is still supported.
For a long time, I didn't think creating compatibility code was the most elegant option for this, which is why I kept putting off adding these assertions, but waiting until 2029 for SF 5.4 to become unsupported seems worse tbh.
So, Please add conditionals to each assertion to check that if the current version of Symfony is 5.4, you throw an exception explaining that these assertions are only available from SF 6.2 onwards (although in reality we only support 6.4+).
|
You can use |
|
@TavoNiievez Thanks for review! I've added check for a symfony version in |
TavoNiievez
left a comment
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.
LGTM!
|
@TavoNiievez Hi! Should we create a new release, so that these assertions would be available for everyone or is there something that is blocking the release? |
|
It is my job to verify that the code works in the three supported versions of Symfony in the test project before creating a release. I had green tests with 7.3 and 6.4, but they failed in 5.4, which is when I wrote to you. I still needed to cherry-pick your changes in the 5.4 branch and update dependencies to bring these changes over and verify that the tests ran well there. Sorry for the delay. |
|
@d-mitrofanov-v I just released version 3.8.0. However, I would ask you to please send a PR targeting branch The tests fail, and that is the expectation, but our CI should not report failures for this behavior. |
|
@TavoNiievez Great, thanks! Here is the PR for 5.4 |
Added assertions for notifier based on symfony NotificationAssertionsTrait