[RFC][DX] Introduce NotificationChecker #4622
Conversation
| @@ -30,6 +30,7 @@ | |||
| <parameter key="sylius.behat.page.admin.crud.create.class">Sylius\Behat\Page\Admin\Crud\CreatePage</parameter> | |||
| <parameter key="sylius.behat.page.admin.crud.update.class">Sylius\Behat\Page\Admin\Crud\UpdatePage</parameter> | |||
| <parameter key="sylius.behat.service.accessor.notification_accessor.class">Sylius\Behat\Service\Accessor\NotificationAccessor</parameter> | |||
| <parameter key="sylius.behat.service.notification_validator.class">Sylius\Behat\Service\NotificationValidator</parameter> | |||
There was a problem hiding this comment.
Perhaps we should be consistent and name these:
sylius.behat.service.accessor.notification
sylius.behat.service.validator.notification
?
Or maybe name this... Asserter!!! 😈
There was a problem hiding this comment.
IMO sylius.behat.service.notification_{validator,accessor} seems the best for me, as grouping them by accessor / validator doesn't really help - I need a notification validator, not a validator for notifications.
There was a problem hiding this comment.
👍 for services names. Asserter doesn't sound good for me 😄 I would leave Validator :)
There was a problem hiding this comment.
As for name - asserter / validator - the verb used in the interface suggests that it's a notification checker :)
There was a problem hiding this comment.
Asserter was a joke, but I am +1 for Checker. Validator can be confusing.
pjedrzejewski
added
RFC
| { | ||
| $this->assertPositiveMessage(); | ||
|
|
||
| Assert::true( |
There was a problem hiding this comment.
Can we use a custom exception?
lchrusciel
force-pushed
the
notification-validator
branch
2 times, most recently
from
a3ed087 to
4e98449
Compare
lchrusciel
changed the title
|
|
||
| namespace Sylius\Behat\Exception; | ||
|
|
||
| use Exception; |
lchrusciel
force-pushed
the
notification-validator
branch
4 times, most recently
from
f85739f to
ccfdcf4
Compare
| \Exception $previous = null | ||
| ) { | ||
| $message = sprintf( | ||
| "Expected *%s* notification with a \"%s\" message was not found.\n *%s* notification with a \"%s\" message has been founded.", |
There was a problem hiding this comment.
has been found instead of has been founded no ?
lchrusciel
force-pushed
the
notification-validator
branch
from
ccfdcf4 to
b730b83
Compare
| @@ -25,6 +25,7 @@ | |||
| <parameter key="sylius.behat.mocker.class">Sylius\Behat\Service\Mocker\Mocker</parameter> | |||
| <parameter key="sylius.behat.response_loader.class">Sylius\Behat\Service\ResponseLoader</parameter> | |||
| <parameter key="sylius.behat.notification_accessor.class">Sylius\Behat\Service\Accessor\NotificationAccessor</parameter> | |||
| <parameter key="sylius.behat.service.notification_checker.class">Sylius\Behat\Service\NotificationChecker</parameter> | |||
There was a problem hiding this comment.
In other PR you removed "service" prefix. I agree.
|
👍 |
Just a thought. Because of usage of NotificationAccessor we had to duplicate some line, so I just extracted it to separated service, just to keep DRY. I haven't coupled it with NotificationAccessor, because it can be useful somewhere else and writing test for it is pretty painful also.
Edit
After introducing new Exception output for developer will look like this: