-
-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
Added docs about testing HTML email content #18273
Added docs about testing HTML email content #18273
Conversation
Hi @sarahboyce! I tried my best, working in plain text files with zero syntax support feels... uncomfortable 😅 No idea if I screwed up somewhere. I'd be happy for some feedback and guidance if I'm on the right track 🙂 Best regards |
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.
Looks good, I have minor comments 👍
@sarahboyce Thx so much for the review 💪 Applied all your suggestions. |
docs/topics/testing/tools.txt
Outdated
|
||
# Verify that the HTML content for the first message is correct. | ||
self.assertEqual( | ||
mail.outbox[0].alternatives[0][0], "<strong>Here is the message.</strong>" |
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.
Worth flagging this conflicts with #18261 slightly
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.
Thx @RealOrangeOne! What would be the default process in this case? see which PR makes it in faster and adjust the other afterwards?
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 it conflicts too much, I think they roughly compliment each other 👍
#18261 needs an accepted ticket and then docs, release note etc
@RealOrangeOne I'd invite you to add your thoughts on https://forum.djangoproject.com/t/improve-email-unit-testing/32044/2 as this is a topic along the lines of "can we make testing emails a bit nicer" and your PR looks related to this as a goal
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.
Thx @sarahboyce! What's the next action? Let it sit here for a while so others can add their opinion?
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 squashed and tweaked the test (I reduced some of the asserts as they exist above but can add them in if wanted).
Would like an approval from someone other than me before merging 👍 (maybe from @nessita when she's back)
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.
Thank you for summoning me! I agree with the resolution of this PR.
2c01406
to
2af05ae
Compare
…ves message. It is common that emails have an alternative HTML part attached. This test shows how you can assert this content.
2af05ae
to
9721f9d
Compare
Hi @sarahboyce do you think this PR has still value after the other, releated PRs? Best |
I think because the new attributes have examples around testing, probably not 🤔 |
Ok, was at least not too much work. |
Trac ticket number
ticket-XXXXX
https://forum.djangoproject.com/t/improve-email-unit-testing/32044/2
Branch description
Added docs about testing HTML email content.
Checklist
main
branch.