Improve test reliability (#488)

* Improve test reliability

Add tags to allow running only some of the tests
- `mix test --only fixture` to run only the tests that use our custom
  in_fixture helper
- `mix test --exclude slow` to exclude extra slow tests (primarily dialyzer)
- Fix a race-condition that showed up in some debugger tests
  - Without this fix (which admittedly is hacky) the "notifies about
    process exit" debugger test would fail about 1/4 of the time.
- Capture output and logs from debugger tests
  - Otherwise that output would make it more difficult to read the test output
- Rename the error raised in debugger tests to make it more obvious that
  these are test-specific errors and not a generic ArgumentError (which
  could be raised for multiple reasons)

* Increase timeout for build diagnostics test

* Use start_supervised! to ensure that processes finish before next test

Intent is to fix this test error:

    1) test put_chars mfa mfa binary (ElixirLS.Utils.OutputDeviceTest)
  Error:      test/output_device_test.exs:175
       ** (MatchError) no match of right hand side value: {:error, {:already_started, #PID<0.484.0>}}
       stacktrace:
         test/output_device_test.exs:72: ElixirLS.Utils.OutputDeviceTest.__ex_unit_setup_0/1
         test/output_device_test.exs:1: ElixirLS.Utils.OutputDeviceTest.__ex_unit__/2

https://github.com/elixir-lsp/elixir-ls/pull/488/checks?check_run_id=1899187694

Author: axelson

apps/elixir_ls_debugger/lib/debugger/server.ex

@@ -835,6 +835,10 @@ defmodule ElixirLS.Debugger.Server do
   end
 
   defp launch_task(task, args) do
+    # This fixes a race condition in  the tests and likely improves reliability when using the
+    # debugger as well.
+    Process.sleep(100)
+
     Mix.Task.run(task, args)
 
     # Starting from Elixir 1.9 Mix.Task.run will return so we need to sleep our

apps/elixir_ls_debugger/test/debugger_test.exs

@@ -89,6 +89,7 @@ defmodule ElixirLS.Debugger.ServerTest do
     end
   end
 
+  @tag :fixture
   test "basic debugging", %{server: server} do
     in_fixture(__DIR__, "mix_project", fn ->
       Server.receive_packet(server, initialize_req(1, %{}))
@@ -100,6 +101,7 @@ defmodule ElixirLS.Debugger.ServerTest do
           "request" => "launch",
           "type" => "mix_task",
           "task" => "test",
+          "taskArgs" => ["--only", "quadruple"],
           "projectDir" => File.cwd!()
         })
       )
@@ -132,7 +134,8 @@ defmodule ElixirLS.Debugger.ServerTest do
                        "allThreadsStopped" => false,
                        "reason" => "breakpoint",
                        "threadId" => thread_id
-                     })
+                     }),
+                     5_000
 
       Server.receive_packet(server, stacktrace_req(7, thread_id))
 
@@ -191,6 +194,7 @@ defmodule ElixirLS.Debugger.ServerTest do
     end)
   end
 
+  @tag :fixture
   test "handles invalid requests", %{server: server} do
     in_fixture(__DIR__, "mix_project", fn ->
       Server.receive_packet(server, initialize_req(1, %{}))
@@ -234,7 +238,8 @@ defmodule ElixirLS.Debugger.ServerTest do
                        "allThreadsStopped" => false,
                        "reason" => "breakpoint",
                        "threadId" => thread_id
-                     })
+                     }),
+                     1_000
 
       Server.receive_packet(server, stacktrace_req(7, "not existing"))
 
@@ -341,6 +346,7 @@ defmodule ElixirLS.Debugger.ServerTest do
     end)
   end
 
+  @tag :fixture
   test "notifies about process exit", %{server: server} do
     in_fixture(__DIR__, "mix_project", fn ->
       Server.receive_packet(server, initialize_req(1, %{}))
@@ -377,7 +383,7 @@ defmodule ElixirLS.Debugger.ServerTest do
       assert_receive(response(_, 5, "configurationDone", %{}))
 
       Server.receive_packet(server, request(6, "threads", %{}))
-      assert_receive(response(_, 6, "threads", %{"threads" => threads}))
+      assert_receive(response(_, 6, "threads", %{"threads" => threads}), 1_000)
       # ensure thread ids are unique
       thread_ids = Enum.map(threads, & &1["id"])
       assert Enum.count(Enum.uniq(thread_ids)) == Enum.count(thread_ids)
@@ -389,14 +395,21 @@ defmodule ElixirLS.Debugger.ServerTest do
                      }),
                      500
 
-      assert_receive event(_, "thread", %{
-                       "reason" => "exited",
-                       "threadId" => ^thread_id
-                     }),
-                     5000
+      {log, stderr} =
+        capture_log_and_io(:standard_error, fn ->
+          assert_receive event(_, "thread", %{
+                           "reason" => "exited",
+                           "threadId" => ^thread_id
+                         }),
+                         5000
+        end)
+
+      assert log =~ "Fixture MixProject expected error"
+      assert stderr =~ "Fixture MixProject expected error"
     end)
   end
 
+  @tag :fixture
   test "notifies about mix task exit", %{server: server} do
     in_fixture(__DIR__, "mix_project", fn ->
       Server.receive_packet(server, initialize_req(1, %{}))
@@ -444,11 +457,17 @@ defmodule ElixirLS.Debugger.ServerTest do
                      }),
                      5000
 
-      assert_receive event(_, "thread", %{
-                       "reason" => "exited",
-                       "threadId" => ^thread_id
-                     }),
-                     5000
+      {log, io} =
+        capture_log_and_io(:stderr, fn ->
+          assert_receive event(_, "thread", %{
+                           "reason" => "exited",
+                           "threadId" => ^thread_id
+                         }),
+                         5000
+        end)
+
+      assert log =~ "Fixture MixProject raise for exit_self/0"
+      assert io =~ "Fixture MixProject raise for exit_self/0"
 
       assert_receive event(_, "exited", %{
                        "exitCode" => 1
@@ -460,6 +479,7 @@ defmodule ElixirLS.Debugger.ServerTest do
     end)
   end
 
+  @tag :fixture
   test "sets breakpoints in erlang modules", %{server: server} do
     in_fixture(__DIR__, "mix_project", fn ->
       Server.receive_packet(server, initialize_req(1, %{}))

apps/elixir_ls_debugger/test/fixtures/mix_project/lib/mix_project.ex

@@ -11,7 +11,7 @@ defmodule MixProject do
     Task.start(fn ->
       Task.start_link(fn ->
         Process.sleep(1000)
-        raise ArgumentError
+        raise "Fixture MixProject expected error"
       end)
 
       Process.sleep(:infinity)
@@ -23,7 +23,7 @@ defmodule MixProject do
   def exit_self do
     Task.start_link(fn ->
       Process.sleep(1000)
-      raise ArgumentError
+      raise "Fixture MixProject raise for exit_self/0"
     end)
 
     Process.sleep(:infinity)

apps/elixir_ls_debugger/test/fixtures/mix_project/test/mix_project_fixturetest.exs

@@ -2,10 +2,12 @@ defmodule MixProjectTest do
   use ExUnit.Case
   doctest MixProject
 
+  @tag :double
   test "double" do
     assert MixProject.double(2) == 4
   end
 
+  @tag :quadruple
   test "quadruple" do
     assert MixProject.quadruple(2) == 8
   end

apps/elixir_ls_utils/lib/output_device.ex

@@ -18,6 +18,16 @@ defmodule ElixirLS.Utils.OutputDevice do
     Task.start_link(fn -> loop({device, output_fn}) end)
   end
 
+  def child_spec(arguments) do
+    %{
+      id: __MODULE__,
+      start: {__MODULE__, :start_link, arguments},
+      type: :worker,
+      restart: :permanent,
+      shutdown: 500
+    }
+  end
+
   def get_opts, do: @opts
 
   ## Implementation

apps/elixir_ls_utils/test/output_device_test.exs

@@ -68,10 +68,16 @@ defmodule ElixirLS.Utils.OutputDeviceTest do
   end
 
   setup do
-    {:ok, fake_user} = FakeOutput.start_link([])
-    {:ok, fake_wire_protocol} = FakeWireProtocol.start_link([])
-    {:ok, output_device} = OutputDevice.start_link(fake_user, &FakeWireProtocol.send/1)
-    {:ok, output_device_error} = OutputDevice.start_link(fake_user, fn _ -> {:error, :emfile} end)
+    fake_user = start_supervised!({FakeOutput, []})
+    fake_wire_protocol = start_supervised!({FakeWireProtocol, []})
+
+    output_device =
+      start_supervised!({OutputDevice, [fake_user, &FakeWireProtocol.send/1]}, id: :output_device)
+
+    output_device_error =
+      start_supervised!({OutputDevice, [fake_user, fn _ -> {:error, :emfile} end]},
+        id: :output_device_error
+      )
 
     {:ok,
      %{

apps/elixir_ls_utils/test/placeholder_test.exs

@@ -0,0 +1,10 @@
+defmodule ElixirLS.Utils.PlaceholderTest do
+  use ExUnit.Case, async: true
+
+  @tag :fixture
+  test "pretend fixture" do
+    # This test is included to prevent the following error:
+    # > The --only option was given to "mix test" but no test was executed
+    :ok
+  end
+end

apps/elixir_ls_utils/test/support/mix_test.case.ex

@@ -74,6 +74,9 @@ defmodule ElixirLS.Utils.MixTest.Case do
     previous = :code.all_loaded()
     project_stack = clear_project_stack!()
 
+    ExUnit.CaptureLog.capture_log(fn -> Application.stop(:mix) end)
+    Application.start(:mix)
+
     try do
       File.cd!(dest, function)
     after
@@ -148,4 +151,19 @@ defmodule ElixirLS.Utils.MixTest.Case do
 
     module.clear_cache()
   end
+
+  def capture_log_and_io(device, fun) when is_function(fun, 0) do
+    # Logger gets stopped during some tests so restart it to be able to capture logs (and keept the
+    # test output clean)
+    Application.ensure_started(:logger)
+
+    log =
+      ExUnit.CaptureLog.capture_log(fn ->
+        io = ExUnit.CaptureIO.capture_io(device, fun)
+        send(self(), {:block_result, io})
+      end)
+
+    assert_received {:block_result, io_result}
+    {log, io_result}
+  end
 end

apps/language_server/test/dialyzer_test.exs

@@ -19,6 +19,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
     {:ok, %{server: server}}
   end
 
+  @tag slow: true, fixture: true
   test "reports diagnostics then clears them once problems are fixed", %{server: server} do
     in_fixture(__DIR__, "dialyzer", fn ->
       file_a = SourceFile.path_to_uri(Path.absname("lib/a.ex"))
@@ -91,6 +92,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
     end)
   end
 
+  @tag slow: true, fixture: true
   test "only analyzes the changed files", %{server: server} do
     in_fixture(__DIR__, "dialyzer", fn ->
       file_c = SourceFile.path_to_uri(Path.absname("lib/c.ex"))
@@ -151,6 +153,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
     end)
   end
 
+  @tag slow: true, fixture: true
   test "reports dialyxir_long formatted error", %{server: server} do
     in_fixture(__DIR__, "dialyzer", fn ->
       file_a = SourceFile.path_to_uri(Path.absname("lib/a.ex"))
@@ -204,6 +207,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
     end)
   end
 
+  @tag slow: true, fixture: true
   test "reports dialyxir_short formatted error", %{server: server} do
     in_fixture(__DIR__, "dialyzer", fn ->
       file_a = SourceFile.path_to_uri(Path.absname("lib/a.ex"))
@@ -248,6 +252,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
     end)
   end
 
+  @tag slow: true, fixture: true
   test "reports dialyzer_formatted error", %{server: server} do
     in_fixture(__DIR__, "dialyzer", fn ->
       file_a = SourceFile.path_to_uri(Path.absname("lib/a.ex"))
@@ -293,6 +298,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
     end)
   end
 
+  @tag slow: true, fixture: true
   test "reports dialyxir_short error in umbrella", %{server: server} do
     in_fixture(__DIR__, "umbrella_dialyzer", fn ->
       file_a = SourceFile.path_to_uri(Path.absname("apps/app1/lib/app1.ex"))
@@ -363,6 +369,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
     end)
   end
 
+  @tag slow: true, fixture: true
   test "protocol rebuild does not trigger consolidation warnings", %{server: server} do
     in_fixture(__DIR__, "protocols", fn ->
       root_uri = SourceFile.path_to_uri(File.cwd!())

apps/language_server/test/providers/formatting_test.exs

@@ -5,6 +5,7 @@ defmodule ElixirLS.LanguageServer.Providers.FormattingTest do
   alias ElixirLS.LanguageServer.SourceFile
   alias ElixirLS.LanguageServer.Test.FixtureHelpers
 
+  @tag :fixture
   test "Formats a file" do
     in_fixture(Path.join(__DIR__, ".."), "formatter", fn ->
       path = "lib/file.ex"
@@ -57,6 +58,7 @@ defmodule ElixirLS.LanguageServer.Providers.FormattingTest do
   defp assert_position_type(%{"character" => ch, "line" => line}),
     do: is_integer(ch) and is_integer(line)
 
+  @tag :fixture
   test "returns an error when formatting a file with a syntax error" do
     in_fixture(Path.join(__DIR__, ".."), "formatter", fn ->
       path = "lib/file.ex"
@@ -85,6 +87,7 @@ defmodule ElixirLS.LanguageServer.Providers.FormattingTest do
     end)
   end
 
+  @tag :fixture
   test "Proper utf-16 format: emoji 😀" do
     in_fixture(Path.join(__DIR__, ".."), "formatter", fn ->
       path = "lib/file.ex"
@@ -123,6 +126,7 @@ defmodule ElixirLS.LanguageServer.Providers.FormattingTest do
     end)
   end
 
+  @tag :fixture
   test "Proper utf-16 format: emoji 🏳️‍🌈" do
     in_fixture(Path.join(__DIR__, ".."), "formatter", fn ->
       path = "lib/file.ex"
@@ -161,6 +165,7 @@ defmodule ElixirLS.LanguageServer.Providers.FormattingTest do
     end)
   end
 
+  @tag :fixture
   test "Proper utf-16 format: zalgo" do
     in_fixture(Path.join(__DIR__, ".."), "formatter", fn ->
       path = "lib/file.ex"