Skip to content

[Notifier] Introduce PHPUnit constraints and assertions for the Notifier #46895

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

Merged
merged 1 commit into from
Jul 24, 2022

Conversation

ismail1432
Copy link
Contributor

@ismail1432 ismail1432 commented Jul 10, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

Since 4.4 we have assertions for email , the goal of this PR is to introduce the same kind of assertions for the Notifier component.

   public function testNotifier()
      {  
         /** @var NotifierInterface $notifier */
         $firstNotification = new Notification('Hello World!', ['chat/slack']);
        $firstNotification->content("Symfony is awesome!");

        $notifier->send($firstNotification);

        $secondNotification = (new Notification('New urgent notification'))
            ->importance(Notification::IMPORTANCE_URGENT)
        ;
        $notifier->send($secondNotification);

                $this->assertNotificationCount(2);
        $first = 0;
        $second = 1;
        $this->assertNotificationIsNotQueued($this->getNotifierEvent($first));
        $this->assertNotificationIsNotQueued($this->getNotifierEvent($second));

        $notification = $this->getNotifierMessage($first);
        $this->assertNotificationSubjectContains($notification, 'Hello World!');
        $this->assertNotificationSubjectNotContains($notification, 'New urgent notification');
        $this->assertNotificationTransportIsEqual($notification,'slack');
        $this->assertNotificationTransportIsNotEqual($notification,'mercure');


        $notification = $this->getNotifierMessage($second);
        $this->assertNotificationSubjectContains($notification, 'New urgent notification');
        $this->assertNotificationSubjectNotContains($notification, 'Hello World!');
        $this->assertNotificationTransportIsEqual($notification,'mercure');
        $this->assertNotificationTransportIsNotEqual($notification,'slack');

  }

@ismail1432 ismail1432 requested a review from OskarStark as a code owner July 10, 2022 02:19
@carsonbot carsonbot added this to the 6.2 milestone Jul 10, 2022
@ismail1432 ismail1432 force-pushed the add-notifier-assertions branch 8 times, most recently from 0b2c788 to 3864f8d Compare July 10, 2022 02:37
@ismail1432 ismail1432 changed the title [Notifier] Add PHPUnit constraints and assertions for the Notifier [Notifier] Introduce PHPUnit constraints and assertions for the Notifier Jul 10, 2022
@OskarStark OskarStark requested a review from kbond July 10, 2022 06:25
@ismail1432 ismail1432 force-pushed the add-notifier-assertions branch from 3864f8d to e3a1aa0 Compare July 11, 2022 07:49
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @ismail1432!

This makes sense to me and looks to match the email assertions. I'm not that familiar with notifier but should there be assertions on:

  1. Who the notification was sent to?
  2. What was in the body?

@OskarStark, you've been wanting this type of feature for some time and use notifier in your app(s). Do these assertions cover your needs?

@ismail1432 ismail1432 force-pushed the add-notifier-assertions branch 2 times, most recently from 047a607 to 356e312 Compare July 12, 2022 18:05
@ismail1432
Copy link
Contributor Author

This makes sense to me and looks to match the email assertions. I'm not that familiar with notifier but should there be assertions on:

Who the notification was sent to?
What was in the body?

I add more assertion, my point here is to introduce basic assertions and add more later by me or community 👍

@kbond
Copy link
Member

kbond commented Jul 12, 2022

my point here is to introduce basic assertions and add more later by me or community

Sure, that's fair!

@ismail1432 ismail1432 force-pushed the add-notifier-assertions branch 2 times, most recently from 766d1ca to b6b6383 Compare July 24, 2022 11:10
@ismail1432 ismail1432 force-pushed the add-notifier-assertions branch from b6b6383 to 747e9cb Compare July 24, 2022 11:12
@fabpot
Copy link
Member

fabpot commented Jul 24, 2022

Thank you @ismail1432.

@fabpot fabpot merged commit 800deaa into symfony:6.2 Jul 24, 2022
xabbuh added a commit that referenced this pull request Aug 6, 2022
…(javiereguiluz)

This PR was merged into the 6.2 branch.

Discussion
----------

[Notifier] Rename some arguments in notifier assertions

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | if this is merged, I'll update symfony/symfony-docs#17116

While documenting #46895, I thought about renaming some arguments. So here's a PR with that proposal for your consideration. Thanks.

Commits
-------

b5178dd [Notifier] Rename some arguments in notifier assertions
@fabpot fabpot mentioned this pull request Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants