Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Mailer] AddassertEmailAddressNotContains#60740
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
[Mailer] AddassertEmailAddressNotContains#60740
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| 7.4 | ||
| --- | ||
| * added`assertEmailAddressNotContains()` to`MailerAssertionsTrait`. |
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.
| * added`assertEmailAddressNotContains()` to`MailerAssertionsTrait`. | |
| * Add`assertEmailAddressNotContains()` to the`MailerAssertionsTrait` |
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 agree with your suggestion, and I've made that change.
Thanks a lot for your help!
| $this->assertEmailAddressNotContains($email,'To','fabien@fake.symfony.com'); | ||
| $this->assertEmailAddressNotContains($email,'To','thomas@fake.symfony.com'); | ||
| $this->assertEmailAddressNotContains($email,'Reply-To','me@fake.symfony.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 seems that these assertions are redundant, given that the addresses specified have never been used before.
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 don’t think the assertions are an issue, but I’m happy to update the emails if you have any suggestions for alternatives.
Thanks again! 😄❤️
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've updated the emails tohelene@symfony.com. I believe this still tests the same behavior, but this format is more in line with the emails currently used in Symfony tests 😊
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.
A test that cannot fail is not useful. I think you should to add assertions that would actually fail if something breaks.
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'd like to clarify a couple of points:
- The method is simply applying aLogicalNot to an existingConstraint that is already implemented and tested.
- If you look at several of the current assertions in the sametest file, you'll notice that they cover similar cases to the ones I'm adding now. In fact, the sameemail is already being tested, just in a different way.
As always, if you have any suggestions, they're more than welcome! 😄
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.
Hi@Spomky@bkosun 👋, I also added this assertion
$this->assertEmailAddressNotContains($email,'To','thomas@symfony.com');
This checks that the first email does not includethomas@symfony.com in the "To" field.
Just to clarify: this address is included in the second email, but not in the first one, which is the behavior this assertion is verifying.
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.
Now it makes sense. Thank you!
fd6bc22 to7d47611Compare7d47611 tob64a567CompareUh oh!
There was an error while loading.Please reload this page.
b64a567 to559d45bCompareSpomky commentedJun 10, 2025
Hi@santysisi, Indeed this could be a nice addition. |
santysisi commentedJun 10, 2025
Hi@Spomky 👋, thanks a lot for your comment! 😊 I see where you're coming from. The main concern with that approach is that it could lead to a large number of helper methods, like That said, if you and others feel that having separate, explicit methods is more readable or convenient, I'm happy to add them! 😄 Thanks again for your feedback ❤️ |
Spomky commentedJun 11, 2025
OK so let consider the suggested change from@bkosun and I'll approve this PR. |
10566de toce99b79Comparece99b79 to9cc6a63Comparenicolas-grekas commentedJun 16, 2025
Thank you@santysisi. |
c9500d8 intosymfony:7.4Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This PR introduces a new assertion method
assertEmailAddressNotContains()to theMailerAssertionsTrait.The new method complements the existing
assertEmailAddressContains()by allowing developers to assert that a specific email addressdoes not appear in a given header. This addition brings theassertEmailAddressContainsin line with other similar assertion pairs already available in the trait, such as:assertEmailTextBodyContains/assertEmailTextBodyNotContainsassertEmailHtmlBodyContains/assertEmailHtmlBodyNotContainsassertEmailSubjectContains/assertEmailSubjectNotContainsExample usage