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

Fix tests and resolve a bug in ExecutionBroadcaster #468

Merged
merged 3 commits into from
Nov 30, 2020
Merged

Fix tests and resolve a bug in ExecutionBroadcaster #468

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4299b3b
Fix test issues
pnezis Nov 30, 2020
f8a1a17
Return `state` in case of invalid timezone
pnezis Nov 30, 2020
414bee2
Do not set crash_reason if elixir < 1.10
pnezis Nov 30, 2020
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
2 changes: 1 addition & 1 deletion config/config.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
use Mix.Config
config :logger, :console, metadata: :all
config :logger, :console, metadata: [:all, :crash_reason]
config :elixir, :time_zone_database, Tzdata.TimeZoneDatabase
2 changes: 2 additions & 0 deletions lib/quantum/execution_broadcaster.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,8 @@ defmodule Quantum.ExecutionBroadcaster do
job: job,
error: e
)

state
end

defp get_next_execution_time(
Expand Down
11 changes: 1 addition & 10 deletions lib/quantum/executor.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,15 +159,6 @@ defmodule Quantum.Executor do
def log_exception(kind, reason, stacktrace) do
reason = Exception.normalize(kind, reason, stacktrace)

crash_reason =
case kind do
:throw -> {{:nocatch, reason}, stacktrace}
_ -> {reason, stacktrace}
end

Logger.error(
Exception.format(kind, reason, stacktrace),
crash_reason: crash_reason
)
Logger.error(Exception.format(kind, reason, stacktrace))
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Member

@maennchen maennchen Nov 30, 2020 •
edited
Loading

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

end
end
8 changes: 3 additions & 5 deletions test/quantum/executor_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ defmodule Quantum.ExecutorTest do
assert :ok == wait_for_termination(task)
end)

assert logs =~ ~r/type=error/
assert logs =~ ~r/[error]/
assert logs =~ ~r/Execution failed for job/
assert_receive %{test_id: ^test_id, type: :start}

Expand Down Expand Up @@ -409,8 +409,6 @@ defmodule Quantum.ExecutorTest do
assert :ok == wait_for_termination(task)
end)

assert logs =~ ~r/type=exit/
assert logs =~ ~r/value=failure/
assert logs =~ "[error] ** (exit) :failure"
assert_receive %{test_id: ^test_id, type: :start}

Expand Down Expand Up @@ -463,8 +461,8 @@ defmodule Quantum.ExecutorTest do
end)

'#Ref' ++ rest = :erlang.ref_to_list(ref)
assert logs =~ "type=throw"
assert logs =~ "value=#{rest}"
assert logs =~ "[error] ** (throw)"
assert logs =~ "#{rest}"
assert_receive %{test_id: ^test_id, type: :start}

assert_receive %{
Expand Down