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.

Sign up for GitHub

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

Jump to bottom

Use refute_timeout configured for all refute_ assertions #603

Merged
merged 1 commit into from
May 21, 2021
Merged

Use refute_timeout configured for all refute_ assertions #603

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions lib/bamboo/test.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,16 +235,28 @@ defmodule Bamboo.Test do
@doc """
Check that no email was sent with the given parameters

Similarly to `assert_email_delivered_with`, the assertion waits up to 100ms before
failing. Note that you need to send the email again if you want to make other
assertions after this, as this will receive the `{:delivered_email, email}` message.
Similar to `assert_email_delivered_with/1`, but it checks that an email with
those parameters wasn't sent.

If `Bamboo.Test` is used with shared mode, you must also configure a timeout
in your test config.

# Set this in your config, typically in config/test.exs
config :bamboo, :refute_timeout, 10

The value you set is up to you. Lower values may result in faster tests, but
your tests may incorrectly pass if an email is delivered *after* the timeout.

Note that this assertion helper will grab the email out of the process
mailbox. So if you want to make other assertions about the same email after
this assertion, you need to send the email again.

## Examples

Bamboo.Email.new_email(subject: "something") |> MyApp.Mailer.deliver
refute_email_delivered_with(subject: "something else") # Will pass

email = Bamboo.Email.new_email(subject: "something") |> MyApp.Mailer.deliver
Bamboo.Email.new_email(subject: "something") |> MyApp.Mailer.deliver
refute_email_delivered_with(subject: ~r/some/) # Will fail
"""
defmacro refute_email_delivered_with(email_params) do
Expand All @@ -255,7 +267,7 @@ defmodule Bamboo.Test do
receive do
{:delivered_email, email} -> Map.from_struct(email)
after
100 -> []
Bamboo.Test.refute_timeout() -> []
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is going to be a potentially test breaking concern is it worth a deprecation warning or minor version bump?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this is a good question. I honestly don't know. Technically, this is a bug that we're fixing, so it could be a patch. But if we consider the existing behavior as correct and us changing this, it could be considered potentially breaking. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now that I've thought about this more. Maybe the change we should make is to default the refute_timeout to 100ms instead of 0ms. That would make this not a breaking change. Other tests might wait longer if the email fails to deliver, but they pass as soon as the email is received, so 100ms is a better default than 0ms.

I also like that ExUnit's refute_receive_timeout defaults to 100ms. So it keeps us consistent with that, which probably matches people's expectations more closely.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like a nice approach to this. They can always change the default explicitly but half a second longer test suite seems like a low price to pay for a suitable default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@brimatteng I updated the default to 100ms, so there's no longer a potential for a breaking change, and I updated the PR description.

I'm also hoping to quickly follow up with a PR that allows users to pass the timeout directly, so users should be able to change the timeout per test as desired.

end

if is_nil(received_email_params) do
Expand Down Expand Up @@ -435,7 +447,8 @@ defmodule Bamboo.Test do
""")
end

defp refute_timeout do
@doc false
def refute_timeout do
if using_shared_mode?() do
Application.get_env(:bamboo, :refute_timeout) ||
raise """
Expand All @@ -449,7 +462,7 @@ defmodule Bamboo.Test do
but may incorrectly pass if an email is delivered *after* the timeout.
"""
else
0
100
end
end

Expand Down