-
-
Notifications
You must be signed in to change notification settings - Fork 147
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
Fix tests and resolve a bug in ExecutionBroadcaster #468
Conversation
- Include `:crash_reason` in logger metadata - Remove `:crash_reason` from `log_exception` - Change tests pattern matching
In case of invalid timezone no state was returned and as a result the GenServer was crashing due to a `FunctionClauseError`
Pull Request Test Coverage Report for Build 414bee22b4d6846254a703b342587281fcdb595c-PR-468
💛 - Coveralls |
lib/quantum/executor.ex
Outdated
Exception.format(kind, reason, stacktrace), | ||
crash_reason: crash_reason | ||
) | ||
Logger.error(Exception.format(kind, reason, stacktrace)) |
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.
The crash reason metadata is actually quite important. Have a look at this PR: #464
We also discussed it together with Jose Va im via the mailing list.
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.
According to the docs (https://hexdocs.pm/logger/Logger.html) :crash_reason
is always added automatically by Logger whenever possible
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.
@pnezis Except we're logging manually and it can't do it automatically.
https://groups.google.com/g/elixir-lang-core/c/pWz-uTVMEVM/m/QGXncxD1AQAJ
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.
defmodule CaptureLogErrorTest do
use ExUnit.Case
require Logger
import ExUnit.CaptureLog
test "greets the world" do
assert capture_log(fn ->
try do
raise "foo"
rescue
error in RuntimeError ->
reason = Exception.normalize(:error, error, __STACKTRACE__)
Logger.error(
Exception.format(:error, reason, __STACKTRACE__),
crash_reason: {reason, __STACKTRACE__}
)
end
end) =~ "foo"
end
end
This produces no error for me. Does it for you?
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.
Oh this only happens in Elixir < 1.10.
Then I'd rather just raise the elixir requirement.
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.
Or detect the elixir version and only do it in Elixir ~> 1.10
with a TODO
to remove the condition in the future.
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 confirm it is an elixir 1.9 issue. I will add the version check and push
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.
@maennchen Added the version check.
@pnezis Thanks a lot again! |
Includes the following changes:
:crash_reason
in logger metadata, and remove the custom:crash_reason
fromlog_exception
ExecutionBroadcaster
. In case of invalid timezone nostate
was returned and as a result theGenServer
was crashing:Closes #466