Refactor 1 (#3)

- Refactoring warn_on_invalid_dist/0.
- Tests for warnings from warn_on_invalid_dist/0.
- resolver.lookup now distinguish between successful lookup with 0 names and an error in the lookup.
- Better warnings.

coveralls.json

@@ -0,0 +1,5 @@
+{
+  "skip_files": [
+    "lib/dns_srv_cluster/app/default.ex"
+  ]
+}

lib/dns_srv_cluster/resolver.ex

@@ -26,11 +26,10 @@ defmodule DNSSRVCluster.Resolver do
   def lookup(query, type) when is_binary(query) and type in [:srv] do
     case :inet_res.getbyname(~c"#{query}", type) do
       {:ok, hostent(h_addr_list: addr_list)} ->
-        addr_list
+        {:ok, addr_list}
 
-      {:error, _} ->
-        Logger.warning(~s(inet_res.getbyname for query "#{query}" with type "#{type}"failed.))
-        []
+      {:error, err} ->
+        {:error, err}
     end
   end
 

lib/dns_srv_cluster/worker.ex

@@ -71,15 +71,19 @@ defmodule DNSSRVCluster.Worker do
     records = resolver.lookup(query, :srv)
 
     case records do
-      [] ->
+      {:ok, []} ->
         Logger.warning("DNS query `#{query}` has not found any records.")
         []
 
-      records when is_list(records) ->
+      {:ok, records} when is_list(records) ->
         Enum.map(records, fn srv ->
           node = get_name_from_srv_record(srv)
           :"#{basename}@#{node}"
         end)
+
+      {:error, err} ->
+        Logger.warning("DNS lookup failed with error: #{err}.")
+        []
     end
   end
 
@@ -96,40 +100,64 @@ defmodule DNSSRVCluster.Worker do
     schedule_next_poll(state)
   end
 
-  # credo:disable-for-next-line
-  defp warn_on_invalid_dist do
-    release? = is_binary(System.get_env("RELEASE_NAME"))
-    net_state = if function_exported?(:net_kernel, :get_state, 0), do: :net_kernel.get_state()
+  defp get_net_state do
+    if function_exported?(:net_kernel, :get_state, 0) do
+      :net_kernel.get_state()
+    end
+  end
 
-    cond do
-      !net_state ->
-        :ok
+  defp warn_node_in_release_not_running_distributed_mode do
+    Logger.warning("""
+    Node not running in distributed mode. Ensure the following exports are set in your rel/env.sh.eex file:
+
+        export RELEASE_DISTRIBUTION="${RELEASE_DISTRIBUTION:-"name"}"
+        export RELEASE_NODE="${RELEASE_NODE:-"<%= @release.name %>"}"
+    """)
+  end
 
-      net_state.started == :no and release? ->
-        Logger.warning("""
-        Node not running in distributed mode. Ensure the following exports are set in your rel/env.sh.eex file:
+  defp warn_node_out_of_release_not_running_distributed_mode do
+    Logger.warning("""
+    Node not running in distributed mode. When running outside of a release, you must start net_kernel manually with
+    longnames.
+    See: https://hexdocs.pm/elixir/Node.html#start/3
+    """)
+  end
+
+  def warn_node_out_of_release_running_without_longnames do
+    Logger.warning("""
+    Node not running with longnames which are required for DNS discovery.
+    See: https://hexdocs.pm/elixir/Node.html#start/3
+    """)
+  end
+
+  defp warn_node_in_release_running_without_longnames do
+    Logger.warning("""
+    Node not running with longnames which are required for DNS discovery.
+    Ensure the following exports are set in your rel/env.sh.eex file:
+
+        export RELEASE_DISTRIBUTION="${RELEASE_DISTRIBUTION:-"name"}"
+        export RELEASE_NODE="${RELEASE_NODE:-"<%= @release.name %>"}"
+    """)
+  end
+
+  defp warn_on_invalid_dist do
+    release? = is_binary(System.get_env("RELEASE_NAME"))
+    net_state = get_net_state()
 
-            export RELEASE_DISTRIBUTION="${RELEASE_DISTRIBUTION:-"name"}"
-            export RELEASE_NODE="${RELEASE_NODE:-"<%= @release.name %>"}"
-        """)
+    case net_state do
+      %{started: :no} = _state when release? ->
+        warn_node_in_release_not_running_distributed_mode()
 
-      net_state.started == :no or (!release? and net_state.started != :no and net_state[:name_domain] != :longnames) ->
-        Logger.warning("""
-        Node not running in distributed mode. When running outside of a release, you must start net_kernel manually with
-        longnames.
-        https://www.erlang.org/doc/man/net_kernel.html#start-2
-        """)
+      %{started: :no} = _state when not release? ->
+        warn_node_out_of_release_not_running_distributed_mode()
 
-      net_state[:name_domain] != :longnames and release? ->
-        Logger.warning("""
-        Node not running with longnames which are required for DNS discovery.
-        Ensure the following exports are set in your rel/env.sh.eex file:
+      %{started: started, name_domain: :shortnames} = _state when not release? and started != :no ->
+        warn_node_out_of_release_running_without_longnames()
 
-            export RELEASE_DISTRIBUTION="${RELEASE_DISTRIBUTION:-"name"}"
-            export RELEASE_NODE="${RELEASE_NODE:-"<%= @release.name %>"}"
-        """)
+      %{name_domain: :shortnames} = _state when release? ->
+        warn_node_in_release_running_without_longnames()
 
-      true ->
+      _ ->
         :ok
     end
   end

mix.exs

@@ -27,10 +27,14 @@ defmodule DNSSRVCluster.MixProject do
       deps: deps(styler_compat),
       source_url: @scm_url,
       homepage_url: @scm_url,
+      elixirc_paths: elixirc_paths(Mix.env()),
       description: "Elixir clustering with DNS SRV records"
     ]
   end
 
+  defp elixirc_paths(:test), do: ["lib", "test/support"]
+  defp elixirc_paths(_), do: ["lib"]
+
   defp package do
     [
       maintainers: [@maintainer],

test/dns_srv_cluster_app_test.exs

@@ -11,18 +11,31 @@ defmodule DNSSRVClusterAppTest do
     no_connect_diff_base: ~c"no_connect_diff_base.internal"
   }
 
-  defp wait_for_node_discovery(cluster) do
-    :sys.get_state(cluster)
+  defp tests_cleanup do
+    Application.stop(:dns_srv_cluster)
+    System.delete_env("RELEASE_NAME")
+    Application.delete_env(:dns_srv_cluster, :query)
+    :net_kernel.stop()
+  end
+
+  setup do
+    on_exit(&tests_cleanup/0)
     :ok
   end
 
-  defp prerun do
-    Application.stop(:dns_srv_cluster)
-    :ok = Application.start(:dns_srv_cluster)
+  setup_all do
+    {stdout, res} = System.cmd("epmd", ["-daemon"])
+
+    if res == 0 do
+      :ok
+    else
+      {:error, "EPMD start failed. Return code: #{res}, stdout: #{stdout}."}
+    end
   end
 
-  defp postrun do
-    Application.stop(:dns_srv_cluster)
+  defp wait_for_node_discovery(cluster) do
+    :sys.get_state(cluster)
+    :ok
   end
 
   test "discovers nodes" do
@@ -35,7 +48,7 @@ defmodule DNSSRVClusterAppTest do
       ]
     )
 
-    prerun()
+    :ok = Application.start(:dns_srv_cluster)
 
     worker = DNSSRVCluster.get_pid()
     wait_for_node_discovery(worker)
@@ -59,8 +72,6 @@ defmodule DNSSRVClusterAppTest do
 
     assert n2 == [:"app@already_known.internal"]
     assert n3 == [:"app@new.internal", :"app@no_connect_diff_base.internal"]
-
-    postrun()
   end
 
   test "query with :ignore does not start worker" do
@@ -70,11 +81,9 @@ defmodule DNSSRVClusterAppTest do
       ]
     )
 
-    prerun()
+    :ok = Application.start(:dns_srv_cluster)
 
     assert DNSSRVCluster.get_pid() == nil
-
-    postrun()
   end
 
   test "Emits warning if DNS records was not found" do
@@ -85,26 +94,17 @@ defmodule DNSSRVClusterAppTest do
       ]
     )
 
-    Application.stop(:dns_srv_cluster)
-
     res =
       ExUnit.CaptureLog.capture_log(fn ->
         :ok = Application.start(:dns_srv_cluster)
         :sys.get_state(DNSSRVCluster.get_pid())
       end)
 
     assert res =~ "not found"
-
-    postrun()
   end
 
   test "discover nodes without query fails" do
-    Application.delete_env(:dns_srv_cluster, :query)
-
-    Application.stop(:dns_srv_cluster)
     assert match?({:error, _}, Application.start(:dns_srv_cluster))
-
-    postrun()
   end
 
   test "discover nodes query must be a string" do
@@ -114,9 +114,149 @@ defmodule DNSSRVClusterAppTest do
       ]
     )
 
-    Application.stop(:dns_srv_cluster)
     assert match?({:error, _}, Application.start(:dns_srv_cluster))
+  end
+
+  test "lookup error warning is printed" do
+    Application.put_all_env(
+      dns_srv_cluster: [
+        query: "_app._tcp.nonexistent.domain",
+        resolver: DNSSRVClusterAppTest.ErrResolver
+      ]
+    )
+
+    res =
+      ExUnit.CaptureLog.capture_log(fn ->
+        :ok = Application.start(:dns_srv_cluster)
+        :sys.get_state(DNSSRVCluster.get_pid())
+      end)
+
+    assert res =~ "DNS lookup failed with error: Lookup failed."
+  end
+
+  if function_exported?(:net_kernel, :get_state, 0) do
+    test "running in release without distribution should print the warning message" do
+      Application.put_all_env(
+        dns_srv_cluster: [
+          query: "_app._tcp.nonexistent.domain",
+          resolver: DNSSRVClusterAppTest.NullResolver
+        ]
+      )
+
+      System.put_env("RELEASE_NAME", "my_app")
+
+      res =
+        ExUnit.CaptureLog.capture_log(fn ->
+          :ok = Application.start(:dns_srv_cluster)
+          :sys.get_state(DNSSRVCluster.get_pid())
+        end)
+
+      assert res =~
+               "Node not running in distributed mode. Ensure the following exports are set in your rel/env.sh.eex file:"
+    end
+
+    test "running outside of a release should print the warning message" do
+      Application.put_all_env(
+        dns_srv_cluster: [
+          query: "_app._tcp.nonexistent.domain",
+          resolver: DNSSRVClusterAppTest.NullResolver
+        ]
+      )
+
+      res =
+        ExUnit.CaptureLog.capture_log(fn ->
+          :ok = Application.start(:dns_srv_cluster)
+          :sys.get_state(DNSSRVCluster.get_pid())
+        end)
+
+      assert res =~ """
+             [warning] Node not running in distributed mode. When running outside of a release, you must start net_kernel manually with
+             longnames.
+             """
+    end
+
+    test "running in release with short names distribution should print the warning message" do
+      Application.put_all_env(
+        dns_srv_cluster: [
+          query: "_app._tcp.nonexistent.domain",
+          resolver: DNSSRVClusterAppTest.NullResolver
+        ]
+      )
+
+      System.put_env("RELEASE_NAME", "my_app")
+      {:ok, _pid} = Node.start(:my_node, :shortnames)
+
+      res =
+        ExUnit.CaptureLog.capture_log(fn ->
+          :ok = Application.start(:dns_srv_cluster)
+          :sys.get_state(DNSSRVCluster.get_pid())
+        end)
+
+      assert res =~ """
+             Node not running with longnames which are required for DNS discovery.
+             Ensure the following exports are set in your rel/env.sh.eex file:
+             """
+    end
+
+    test "running out of a release with short names distribution should print the warning message" do
+      Application.put_all_env(
+        dns_srv_cluster: [
+          query: "_app._tcp.nonexistent.domain",
+          resolver: DNSSRVClusterAppTest.NullResolver
+        ]
+      )
+
+      {:ok, _pid} = Node.start(:my_node, :shortnames)
+
+      res =
+        ExUnit.CaptureLog.capture_log(fn ->
+          :ok = Application.start(:dns_srv_cluster)
+          :sys.get_state(DNSSRVCluster.get_pid())
+        end)
+
+      assert res =~ """
+             Node not running with longnames which are required for DNS discovery.
+             See: https://hexdocs.pm/elixir/Node.html#start/3
+             """
+    end
+
+    test "running out of a release with long names distribution should not print any warning messages" do
+      Application.put_all_env(
+        dns_srv_cluster: [
+          query: "_app._tcp.nonexistent.domain",
+          resolver: DNSSRVClusterAppTest.NullResolver
+        ]
+      )
+
+      {:ok, _pid} = Node.start(:my_node@localhost, :longnames)
+
+      res =
+        ExUnit.CaptureLog.capture_log(fn ->
+          :ok = Application.start(:dns_srv_cluster)
+          :sys.get_state(DNSSRVCluster.get_pid())
+        end)
+
+      refute res =~ "Node not running"
+    end
+  end
+
+  test "running in release with long names distribution should not print any warning messages" do
+    Application.put_all_env(
+      dns_srv_cluster: [
+        query: "_app._tcp.nonexistent.domain",
+        resolver: DNSSRVClusterAppTest.NullResolver
+      ]
+    )
+
+    System.put_env("RELEASE_NAME", "my_app")
+    {:ok, _pid} = Node.start(:my_node@localhost, :longnames)
+
+    res =
+      ExUnit.CaptureLog.capture_log(fn ->
+        :ok = Application.start(:dns_srv_cluster)
+        :sys.get_state(DNSSRVCluster.get_pid())
+      end)
 
-    postrun()
+    refute res =~ "Node not running"
   end
 end