Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:6.2fromismail1432:add-notifier-assertions
Jul 24, 2022

Conversation

@ismail1432
Copy link
Contributor

@ismail1432ismail1432 commentedJul 10, 2022•
edited
Loading

QA
Branch?6.2
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix #...
LicenseMIT
Doc PRsymfony/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.

publicfunctiontestNotifier()      {/** @var NotifierInterface $notifier */$firstNotification =newNotification('Hello World!', ['chat/slack']);$firstNotification->content("Symfony is awesome!");$notifier->send($firstNotification);$secondNotification = (newNotification('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');  }

alexislefebvre and chalasr reacted with thumbs up emoji
@carsonbotcarsonbot added this to the6.2 milestoneJul 10, 2022
@ismail1432ismail1432force-pushed theadd-notifier-assertions branch 8 times, most recently from0b2c788 to3864f8dCompareJuly 10, 2022 02:37
@ismail1432ismail1432 changed the title[Notifier] Add PHPUnit constraints and assertions for the Notifier[Notifier] Introduce PHPUnit constraints and assertions for the NotifierJul 10, 2022
@OskarStarkOskarStark requested a review fromkbondJuly 10, 2022 06:25
@ismail1432ismail1432force-pushed theadd-notifier-assertions branch from3864f8d toe3a1aa0CompareJuly 11, 2022 07:49
Copy link
Member

@kbondkbond 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?

@ismail1432ismail1432force-pushed theadd-notifier-assertions branch 2 times, most recently from047a607 to356e312CompareJuly 12, 2022 18:05
@ismail1432
Copy link
ContributorAuthor

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

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

Sure, that's fair!

chalasr, ismail1432, and OskarStark reacted with thumbs up emoji

@ismail1432ismail1432force-pushed theadd-notifier-assertions branch 2 times, most recently from766d1ca tob6b6383CompareJuly 24, 2022 11:10
@ismail1432ismail1432force-pushed theadd-notifier-assertions branch fromb6b6383 to747e9cbCompareJuly 24, 2022 11:12
@fabpot
Copy link
Member

Thank you@ismail1432.

@fabpotfabpot merged commit800deaa intosymfony:6.2Jul 24, 2022
xabbuh added a commit that referenced this pull requestAug 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 updatesymfony/symfony-docs#17116While 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
@fabpotfabpot mentioned this pull requestOct 24, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@kbondkbondkbond left review comments

@fabpotfabpotfabpot approved these changes

@OskarStarkOskarStarkAwaiting requested review from OskarStarkOskarStark is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

4 participants

@ismail1432@kbond@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp