-
Notifications
You must be signed in to change notification settings - Fork 32
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
More flexible result matching #61
Changes from 9 commits
bd9e566
54add38
91a9338
dbaead1
bb4a735
612d182
4d3a7b6
85dcb9f
22f1bd9
72a210d
ee87ac3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,10 +3,13 @@ defmodule RetryTest do | |
use Retry | ||
|
||
import Stream | ||
import ExUnit.CaptureLog | ||
require Logger | ||
|
||
doctest Retry | ||
|
||
defmodule(CustomError, do: defexception(message: "custom error!")) | ||
defmodule(NotOkay, do: defstruct([])) | ||
|
||
describe "retry" do | ||
test "retries execution for specified attempts when result is error tuple" do | ||
|
@@ -45,44 +48,59 @@ defmodule RetryTest do | |
assert elapsed / 1_000 >= 250 | ||
end | ||
|
||
test "retries execution for specified attempts when result is a specified atom" do | ||
retry_atom = :not_ok | ||
|
||
{elapsed, _} = | ||
:timer.tc(fn -> | ||
result = | ||
retry with: linear_backoff(50, 1) |> take(5), atoms: [retry_atom] do | ||
retry_atom | ||
after | ||
_ -> :ok | ||
else | ||
error -> error | ||
end | ||
|
||
assert result == retry_atom | ||
end) | ||
test "retries execution for specified attempts when allowed result is returned" do | ||
testcases = [ | ||
{:not_ok, :all}, | ||
{:not_ok, [:foo, :all]}, | ||
{:not_ok, :not_ok}, | ||
{:not_ok, [:foo, :not_ok]}, | ||
{{:not_ok, :foo}, [:foo, :not_ok]}, | ||
{%NotOkay{}, NotOkay}, | ||
{%NotOkay{}, [Foo, NotOkay]}, | ||
{:not_ok, fn _ -> true end}, | ||
{:not_ok, [fn _ -> false end, fn _ -> true end]}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I think this is a great feature, I feel like passing a function or a list of functions to Thinking out loud, what if we create a new option called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that As far as choosing a new name, I think |
||
{:not_ok, [fn _ -> nil end, fn _ -> 1 end]}, | ||
{:not_ok, [fn :partial -> false end, fn _ -> true end]}, | ||
{:not_ok, | ||
fn | ||
:partial -> false | ||
:not_ok -> true | ||
end} | ||
] | ||
|
||
assert elapsed / 1_000 >= 250 | ||
for {rval, atoms} <- testcases do | ||
{elapsed, _} = | ||
:timer.tc(fn -> | ||
result = | ||
retry with: linear_backoff(50, 1) |> take(5), atoms: atoms do | ||
rval | ||
after | ||
_ -> :ok | ||
else | ||
error -> error | ||
end | ||
|
||
assert result == rval | ||
end) | ||
|
||
assert elapsed / 1_000 >= 250 | ||
end | ||
end | ||
|
||
test "retries execution for specified attempts when result is a tuple with a specified atom" do | ||
retry_atom = :not_ok | ||
|
||
{elapsed, _} = | ||
:timer.tc(fn -> | ||
result = | ||
retry with: linear_backoff(50, 1) |> take(5), atoms: [retry_atom] do | ||
{retry_atom, "Some error message"} | ||
after | ||
_ -> :ok | ||
else | ||
error -> error | ||
end | ||
|
||
assert result == {retry_atom, "Some error message"} | ||
end) | ||
test "does not retry on :error if atoms is specified" do | ||
f = fn -> | ||
retry with: linear_backoff(50, 1) |> take(5), atoms: :not_ok do | ||
Logger.info("running") | ||
:error | ||
after | ||
result -> result | ||
else | ||
error -> :not_this | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mind fixing the warning(s) by renaming this to |
||
end | ||
end | ||
|
||
assert elapsed / 1_000 >= 250 | ||
assert f.() == :error | ||
assert Regex.scan(~r/running/, capture_log(f)) |> length == 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need to capture the log messages here and elsewhere? I personally find it redundant and a bit noisy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was my way of counting the number of executions. Basically just using logs as a side effect that I can count with capture_log. I prefer it over the duration measuring, but I'll use that method if you prefer. |
||
end | ||
|
||
test "retries execution for specified attempts when error is raised" do | ||
|
@@ -102,23 +120,33 @@ defmodule RetryTest do | |
assert elapsed / 1_000 >= 250 | ||
end | ||
|
||
test "retries execution when a whitelisted exception is raised" do | ||
custom_error_list = [CustomError] | ||
test "retries execution when an allowed exception is raised" do | ||
testcases = [ | ||
CustomError, | ||
[OtherThing, CustomError], | ||
:all, | ||
[:other_thing, :all], | ||
fn _ -> true end, | ||
[fn _ -> false end, fn _ -> true end], | ||
[fn :partial -> false end, fn _ -> true end] | ||
] | ||
|
||
{elapsed, _} = | ||
:timer.tc(fn -> | ||
assert_raise CustomError, fn -> | ||
retry with: linear_backoff(50, 1) |> take(5), rescue_only: custom_error_list do | ||
raise CustomError | ||
after | ||
_ -> :ok | ||
else | ||
error -> raise error | ||
for testcase <- testcases do | ||
{elapsed, _} = | ||
:timer.tc(fn -> | ||
assert_raise CustomError, fn -> | ||
retry with: linear_backoff(50, 1) |> take(5), rescue_only: testcase do | ||
raise CustomError | ||
after | ||
_ -> :ok | ||
else | ||
error -> raise error | ||
end | ||
end | ||
end | ||
end) | ||
end) | ||
|
||
assert elapsed / 1_000 >= 250 | ||
assert elapsed / 1_000 >= 250 | ||
end | ||
end | ||
|
||
test "does not retry execution when an unknown exception is raised" do | ||
|
@@ -138,17 +166,37 @@ defmodule RetryTest do | |
assert elapsed / 1_000 < 250 | ||
end | ||
|
||
test "does not retry on RuntimeError if some other rescue_only is specified" do | ||
f = fn -> | ||
assert_raise RuntimeError, fn -> | ||
retry with: linear_backoff(50, 1) |> take(5), rescue_only: CustomError do | ||
Logger.info("running") | ||
raise RuntimeError | ||
after | ||
_ -> :ok | ||
else | ||
error -> raise error | ||
end | ||
end | ||
end | ||
|
||
assert Regex.scan(~r/running/, capture_log(f)) |> length == 1 | ||
end | ||
|
||
test "does not have to retry execution when there is no error" do | ||
result = | ||
f = fn -> | ||
retry with: linear_backoff(50, 1) |> take(5) do | ||
Logger.info("running") | ||
{:ok, "Everything's so awesome!"} | ||
after | ||
result -> result | ||
else | ||
_ -> :error | ||
end | ||
end | ||
|
||
assert result == {:ok, "Everything's so awesome!"} | ||
assert f.() == {:ok, "Everything's so awesome!"} | ||
assert Regex.scan(~r/running/, capture_log(f)) |> length == 1 | ||
end | ||
|
||
test "uses the default 'after' action" do | ||
|
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.
Nitpick:
val.starts_with("ok")
looks more like snake-cased Scala than Elixir 😛. Do you meanString.starts_with?(val, "ok")
?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.
lol. Yeah my scala background is showing. I'll fix it.