Repository: quantum-elixir/quantum-core
Pull request #445 (2020-06-28 23:58:14)

feat: Handles task failures by returning a value
Right now, a value is not always returned, as exits, exceptions, and thrown values are not handled inside the task.

This ensures a value is always returned.

coveralls: ## Pull Request Test Coverage Report for [Build 8f059083561f17e9ca38233ff7e28f7c9743f137-PR-445](https://coveralls.io/builds/31737178)

* **2** of **2**   **(100.0%)**  changed or added relevant lines in **1** file are covered.
* **1** unchanged line in **1** file lost coverage.
* Overall coverage decreased (**-0.3%**) to **87.425%**

---


|  Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [lib/quantum/executor.ex](https://coveralls.io/builds/31737178/source?filename=lib%2Fquantum%2Fexecutor.ex#L61) | 1 | 96.0% |
<!-- | **Total:** | **1** |  | -->

|  Totals | [![Coverage Status](https://coveralls.io/builds/31737178/badge)](https://coveralls.io/builds/31737178) |
| :-- | --: |
| Change from base [Build 7d97024030b1b0b6872e54707932df5691fc9a45](https://coveralls.io/builds/31669055): |  -0.3% |
| Covered Lines: | 292 |
| Relevant Lines: | 334 |

---
##### 💛  - [Coveralls](https://coveralls.io)

maennchen: Thanks @isaacsanders ❤️ 

I think it would be better to log that an exception was thrown instead of the yielded result.

Patch:
diff --git a/lib/quantum/executor.ex b/lib/quantum/executor.ex
index 324209b..b77852c 100644
--- a/lib/quantum/executor.ex
+++ b/lib/quantum/executor.ex
@@ -84,14 +84,25 @@ defmodule Quantum.Executor do
           "[#{inspect(Node.self())}][#{__MODULE__}] Execute started for job #{inspect(job_name)}"
         end)
 
-      result = execute_task(task)
-
-      debug_logging &&
-        Logger.debug(fn ->
-          "[#{inspect(Node.self())}][#{__MODULE__}] Execution ended for job #{inspect(job_name)}, which yielded result: #{
-            inspect(result)
-          }"
-        end)
+      try do
+        execute_task(task)
+      catch
+        type, value ->
+          debug_logging &&
+            Logger.debug(fn ->
+              "[#{inspect(Node.self())}][#{__MODULE__}] Execution ended for job #{
+                inspect(job_name)
+              }, which failed due to: #{Exception.format(type, value, __STACKTRACE__)}"
+            end)
+      else
+        result ->
+          debug_logging &&
+            Logger.debug(fn ->
+              "[#{inspect(Node.self())}][#{__MODULE__}] Execution ended for job #{
+                inspect(job_name)
+              }, which yielded result: #{inspect(result)}"
+            end)
+      end
 
       :ok
     end)


Test patch:
diff --git a/test/quantum/executor_test.exs b/test/quantum/executor_test.exs
index cb39b92..aaa1780 100644
--- a/test/quantum/executor_test.exs
+++ b/test/quantum/executor_test.exs
@@ -189,9 +189,107 @@ defmodule Quantum.ExecutorTest do
 
       assert :marked_running = TaskRegistry.mark_running(task_registry, job.name, Node.self())
     end
+
+    test "logs error", %{
+      task_supervisor: task_supervisor,
+      task_registry: task_registry,
+      debug_logging: debug_logging
+    } do
+      job =
+        TestScheduler.new_job()
+        |> Job.set_task(fn -> raise "failed" end)
+        |> Job.set_overlap(false)
+
+      logs =
+        capture_log(fn ->
+          {:ok, task} =
+            Executor.start_link(
+              %StartOpts{
+                task_supervisor_reference: task_supervisor,
+                task_registry_reference: task_registry,
+                debug_logging: debug_logging
+              },
+              %Event{job: job, node: Node.self()}
+            )
+
+          assert :ok == wait_for_termination(task)
+        end)
+
+      assert logs =~ ~r/\(RuntimeError\) failed/
+    end
+
+    test "logs exit", %{
+      task_supervisor: task_supervisor,
+      task_registry: task_registry,
+      debug_logging: debug_logging
+    } do
+      job =
+        TestScheduler.new_job()
+        |> Job.set_task(fn -> exit(:failure) end)
+        |> Job.set_overlap(false)
+
+      logs =
+        capture_log(fn ->
+          {:ok, task} =
+            Executor.start_link(
+              %StartOpts{
+                task_supervisor_reference: task_supervisor,
+                task_registry_reference: task_registry,
+                debug_logging: debug_logging
+              },
+              %Event{job: job, node: Node.self()}
+            )
+
+          assert :ok == wait_for_termination(task)
+        end)
+
+      assert logs =~ ~r/\(exit\) :failure/
+    end
+
+    test "logs throw", %{
+      task_supervisor: task_supervisor,
+      task_registry: task_registry,
+      debug_logging: debug_logging
+    } do
+      ref = make_ref()
+
+      job =
+        TestScheduler.new_job()
+        |> Job.set_task(fn -> throw(ref) end)
+        |> Job.set_overlap(false)
+
+      logs =
+        capture_log(fn ->
+          {:ok, task} =
+            Executor.start_link(
+              %StartOpts{
+                task_supervisor_reference: task_supervisor,
+                task_registry_reference: task_registry,
+                debug_logging: debug_logging
+              },
+              %Event{job: job, node: Node.self()}
+            )
+
+          assert :ok == wait_for_termination(task)
+        end)
+
+      assert logs =~ "(throw) #{inspect(ref)}"
+    end
   end
 
   def send(caller) do
     send(caller, :executed)
   end
+
+  def wait_for_termination(pid, timeout \\ 5000) do
+    ref = Process.monitor(pid)
+
+    receive do
+      {:DOWN, ^ref, :process, _pid, _reason} ->
+        :ok
+    after
+      timeout ->
+        :error
+    end
+  end
 end