Skip to content
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

Add assert_email_delivered_matches/1 to test functions #534

Conversation

devonestes
Copy link
Contributor

Right now if one wanted to make pretty comprehensive assertions on a
delivered email, you need to either match the exact email with
assert_email_delivered/1 or use values or regexes with
assert_email_delivered_with/1. The problem with those is that it
doesn't allow you to bind a variable to the email that was delivered,
meaning you need to do all your assertions on that one email in that one
function.

This becomes especially problematic with things like testing the body of
an HTML email, as your option is either an exact match (which is
brittle) or a single regex (which is exceptionally complicated). By
allowing the user to use the match operator to bind variables and then
test those variables after the assert_email_delivered_matches/1 call,
it can make for much simpler and less brittle tests.

It's a very small implementation - basically all it does is hide the
implementation detail of how the TestAdapter works, which I think is
enough to warrant its existence.

I thought about changing something like assert_email_delivered/1 to
allow for binding variables in this way, but that would change the
semantics of that function too significantly, so I went with introducing
a new function.

Right now if one wanted to make pretty comprehensive assertions on a
delivered email, you need to either match the exact email with
`assert_email_delivered/1` or use values or regexes with
`assert_email_delivered_with/1`. The problem with those is that it
doesn't allow you to bind a variable to the email that was delivered,
meaning you need to do all your assertions on that one email in that one
function.

This becomes especially problematic with things like testing the body of
an HTML email, as your option is either an exact match (which is
brittle) or a single regex (which is exceptionally complicated). By
allowing the user to use the match operator to bind variables and then
test those variables after the `assert_email_delivered_matches/1` call,
it can make for much simpler and less brittle tests.

It's a very small implementation - basically all it does is hide the
implementation detail of how the TestAdapter works, which I think is
enough to warrant its existence.

I thought about changing something like `assert_email_delivered/1` to
allow for binding variables in this way, but that would change the
semantics of that function too significantly, so I went with introducing
a new function.
@maymillerricci
Copy link
Contributor

Thanks for this @devonestes! You raise a good point. I think there are a couple of different options to achieve something similar with the existing API that I wanted to bring up and get your thoughts.

  1. Bind to the returned results from assert_delivered_email:
{:delivered_email, email} = assert_delivered_email(sent_email)
assert email.text_body =~ "Welcome to MyApp, #{user_name}"
assert email.text_body =~ "You can sign up at https://my_app.com/users/#{user_name}"

or

{_, %{text_body: text_body}} = assert_delivered_email(sent_email)
assert text_body =~ "Welcome to MyApp, #{user_name}"
assert text_body =~ "You can sign up at https://my_app.com/users/#{user_name}"
  1. Use assert_received directly and bind the variable to that:
assert_received {:delivered_email, email}
assert email.text_body =~ "Welcome to MyApp, #{user_name}"
assert email.text_body =~ "You can sign up at https://my_app.com/users/#{user_name}"
  1. Use assert_email_delivered_with with multiple regexes testing the email body:
assert_email_delivered_with(
  text_body: ~r/Welcome to MyApp, #{user_name}/,
  text_body: ~r/You can sign up at https:\/\/my_app.com\/users\/#{user_name}/
)

Do you think any of those is a solution for this? Though maybe each of those is a little bit strange in its own way. I'm not saying your addition should not be included, but I want to make sure there aren't existing reasonable ways to accomplish things before we add new code. Let me know your thoughts!

@devonestes
Copy link
Contributor Author

Howdy!

So right now I'm using option 2, but the problem there (and with option 1) is that I'm depending on a private implementation detail of the TestAdapter, and it would be better for everybody if that weren't the case. Option 1 is also kind of superfluous since assert_delivered_email expects an exact match, so if that assertion passes there's no reason to make any further assertions.

I guess option 3 might help in some ways, but if I had to choose between that and option 2, I'd choose option 2 as it's more flexible and lets me have better failure messages since I can turn 1 assertion into many assertions, each with a specific failure message.

The other thing I thought of that would help is if there was something like a get_delivered_emails/0 function that returned a list of all delivered emails (and maybe that accepted a timeout to wait in the event that there are no emails?) - then the implementation details of the TestAdapter can remain hidden from callers and I can still get the delivered emails to do with what I please.

This has the downside of not being selective based on a pattern, so timing and timeouts get a bit trickier there, and that might lead to some flaky tests if, for example, you send two emails but actually want to test against the third but it hadn't actually been "sent" by the time the function is called.

I'm happy to provide an implementation of that as well if y'all decide that's the better way to go, though. I'd personally prefer that over depending on the implementation details of the TestAdapter.

@maymillerricci
Copy link
Contributor

Thanks for your thoughts! I agree with what you're saying about not wanting to rely on the hidden implementation details, so I do think you're right that we should add something here to handle this. I'm intrigued by the get_delivered_emails/0 option, and then being to do whatever you want with the delivered emails. My initial thought is that that would be more intuitive and straightforward for people using it, but I suppose the downside is the potential for flakiness like you said. Do you think that would be preferable over what you have here? At this point though I'd be happy to merge this as well if we think this is preferable.

Comment on lines +288 to +294
test "assert_delivered_email_matches/1 allows binding of variables for further testing" do
sent_email = new_email(from: "foo@bar.com", to: ["foo@bar.com"])
TestMailer.deliver_now(sent_email)

assert_delivered_email_matches(%{to: [{nil, email}]})
assert email == "foo@bar.com"
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Just one small thought here if we do go with this. What do you think about updating the test to be more like your example in the docs as an example use case for this, to make it extra clear what real-life / more complex scenarios someone might want to use this for?

@devonestes
Copy link
Contributor Author

I'd definitely prefer this to get_delivered_emails/0 because of the flakiness that it get_delivered_emails/0 can cause. There's no reason that both can't exist, but I don't think there's any scenario where I could see myself using get_delivered_emails/0 if assert_email_delivered_matches/1 exists.

@maymillerricci
Copy link
Contributor

Sounds great -- thanks so much for your work on this!

@maymillerricci maymillerricci merged commit 5e88ed3 into beam-community:master Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants