Address review concerns

- Also show errors in the client via window/showMessage
- Improve logging cases
- Only register with nodemapper when node started ok
- For now do not introduce full expert test

Author: badosu

apps/expert/lib/expert.ex

@@ -4,6 +4,7 @@ defmodule Expert do
   alias Expert.Protocol.Id
   alias Expert.Provider.Handlers
   alias Expert.State
+  alias GenLSP.Enumerations
   alias GenLSP.Requests
   alias GenLSP.Structures
 
@@ -55,10 +56,12 @@ defmodule Expert do
 
     with {:ok, response, state} <- State.initialize(state, request),
          {:ok, response} <- Expert.Protocol.Convert.to_lsp(response) do
-      config = state.configuration
-
       Task.Supervisor.start_child(:expert_task_queue, fn ->
-        Logger.info("Starting project at uri #{config.project.root_uri}")
+        config = state.configuration
+
+        start_message = "Starting project at uri #{config.project.root_uri}"
+        GenLSP.info(lsp, start_message)
+        Logger.info(start_message)
 
         start_result = Project.Supervisor.start(config.project)
 
@@ -201,11 +204,42 @@ defmodule Expert do
   end
 
   def handle_info({:engine_initialized, {:error, reason}}, lsp) do
-    GenLSP.error(get_lsp(), initialization_error_message(reason))
+    error_message = initialization_error_message(reason)
+    error(error_message)
 
     {:noreply, lsp}
   end
 
+  def info(message) do
+    Logger.info(message)
+
+    log_show(message, Enumerations.MessageType.info())
+  end
+
+  def error(message) do
+    Logger.error(message)
+
+    log_show(message, Enumerations.MessageType.error())
+  end
+
+  defp log_show(message, log_level) do
+    lsp = get_lsp()
+
+    GenLSP.notify(lsp, %GenLSP.Notifications.WindowShowMessage{
+      params: %GenLSP.Structures.ShowMessageParams{
+        type: log_level,
+        message: message
+      }
+    })
+
+    GenLSP.notify(lsp, %GenLSP.Notifications.WindowLogMessage{
+      params: %GenLSP.Structures.LogMessageParams{
+        type: log_level,
+        message: message
+      }
+    })
+  end
+
   defp apply_to_state(%State{} = state, %{} = request_or_notification) do
     case State.apply(state, request_or_notification) do
       {:ok, response, new_state} -> {:ok, response, new_state}
@@ -298,13 +332,13 @@ defmodule Expert do
       # NOTE:
       # ** (Mix.Error) httpc request failed with: ... Could not install Hex because Mix could not download metadata ...
       {:shutdown, {:error, :normal, message}} ->
-        "Node #{name} shutdown with error:\n\n#{message}"
+        "Engine #{name} shutdown with error:\n\n#{message}"
 
       {:shutdown, {:node_exit, node_exit}} ->
-        "Node #{name} exit with status #{node_exit.status}, last message:\n\n#{node_exit.last_message}"
+        "Engine #{name} exit with status #{node_exit.status}, last message:\n\n#{node_exit.last_message}"
 
       reason ->
-        "Failed to start node #{name}: #{inspect(reason)}"
+        "Failed to start engine #{name}: #{inspect(reason)}"
     end
   end
 end

apps/expert/lib/expert/application.ex

@@ -66,7 +66,18 @@ defmodule Expert.Application do
 
     ensure_epmd_module!()
 
-    children = [
+    LogFilter.hook_into_logger()
+
+    children_spec = children(buffer: buffer_opts)
+    opts = [strategy: :one_for_one, name: Expert.Supervisor]
+
+    Supervisor.start_link(children_spec, opts)
+  end
+
+  def children(opts) do
+    buffer_opts = Keyword.fetch!(opts, :buffer)
+
+    [
       {Forge.NodePortMapper, []},
       document_store_child_spec(),
       {DynamicSupervisor, Expert.Project.DynamicSupervisor.options()},
@@ -81,11 +92,6 @@ defmodule Expert.Application do
        dynamic_supervisor: Expert.DynamicSupervisor,
        assigns: Expert.Assigns}
     ]
-
-    LogFilter.hook_into_logger()
-
-    opts = [strategy: :one_for_one, name: Expert.Supervisor]
-    Supervisor.start_link(children, opts)
   end
 
   @doc false

apps/expert/lib/expert/engine_node.ex

@@ -51,9 +51,9 @@ defmodule Expert.EngineNode do
           # everything works fine.
           """
           node_start = Node.start(:"#{Project.node_name(state.project)}", :longnames)
-          #{Forge.NodePortMapper}.register()
           case node_start do
             {:ok, _} ->
+              #{Forge.NodePortMapper}.register()
               IO.puts(\"ok\")
             {:error, reason} ->
               IO.puts(\"error starting node:\n  \#{inspect(reason)}\")
@@ -112,16 +112,18 @@ defmodule Expert.EngineNode do
     end
 
     def on_exit_status(%__MODULE__{} = state, exit_status) do
-      Logger.debug(
-        "Node exited with status #{exit_status}, last message: #{to_string(state.last_message)}"
-      )
-
       stop_reason =
         case exit_status do
           0 ->
+            Logger.info("Engine shutdown")
+
             :shutdown
 
           _error_status ->
+            Logger.error(
+              "Engine shut down unexpectedly, node exited with status #{exit_status}). Last message: #{state.last_message}"
+            )
+
             {:shutdown, {:node_exit, %{status: exit_status, last_message: state.last_message}}}
         end
 

apps/expert/test/expert_test.exs

@@ -1,75 +1,11 @@
 defmodule Expert.ExpertTest do
-  alias Forge.Test.Fixtures
-
-  use ExUnit.Case, async: false
+  use ExUnit.Case, async: true
   use Patch
-  use Forge.Test.EventualAssertions
 
   require GenLSP.Test
 
   import Expert.Test.Protocol.TransportSupport
 
-  defp start_expert do
-    patch(System, :argv, fn -> ["--port", "0"] end)
-
-    assert {:ok, _} = Application.ensure_all_started(:expert)
-
-    on_exit(fn -> Application.stop(:expert) end)
-  end
-
-  describe "server testing" do
-    setup do
-      start_expert()
-
-      %{lsp: lsp} = :sys.get_state(Expert.Buffer)
-      {:ok, port} = :inet.port(GenLSP.Buffer.comm_state(Expert.Buffer).lsocket)
-
-      expert = %{
-        lsp: lsp,
-        port: port
-      }
-
-      client = GenLSP.Test.client(expert)
-
-      [expert: expert, client: client]
-    end
-
-    test "replies to initialize with expert info and capabilities", %{client: client} do
-      id = System.unique_integer([:positive])
-
-      project = Fixtures.project()
-
-      root_uri = project.root_uri
-      root_path = Forge.Project.root_path(project)
-
-      assert :ok ==
-               GenLSP.Test.request(client, %{
-                 "id" => id,
-                 "jsonrpc" => "2.0",
-                 "method" => "initialize",
-                 "params" => %{
-                   "rootPath" => root_path,
-                   "rootUri" => root_uri,
-                   "capabilities" => %{},
-                   "workspaceFolders" => [
-                     %{
-                       uri: root_uri,
-                       name: root_path
-                     }
-                   ]
-                 }
-               })
-
-      GenLSP.Test.assert_result(^id, result, 500)
-
-      {:ok, initialize_result} =
-        GenLSP.Requests.Initialize.result()
-        |> Schematic.dump(Expert.State.initialize_result())
-
-      assert result == initialize_result
-    end
-  end
-
   test "sends an error message on engine initialization error" do
     with_patched_transport()
 
@@ -92,5 +28,13 @@ defmodule Expert.ExpertTest do
                         message: ^error_message
                       }
                     }}
+
+    assert_receive {:transport,
+                    %GenLSP.Notifications.WindowShowMessage{
+                      params: %GenLSP.Structures.ShowMessageParams{
+                        type: ^error_message_type,
+                        message: ^error_message
+                      }
+                    }}
   end
 end