Improve tests (#1627)

lib/livebook/runtime/erl_dist/node_manager.ex

@@ -188,6 +188,7 @@ defmodule Livebook.Runtime.ErlDist.NodeManager do
       opts
       |> Keyword.put_new(:ebin_path, ebin_path(state.tmp_dir))
       |> Keyword.put_new(:tmp_dir, child_tmp_dir(state.tmp_dir))
+      |> Keyword.put_new(:base_path_env, System.get_env("PATH", ""))
 
     {:ok, server_pid} =
       DynamicSupervisor.start_child(state.server_supervisor, {ErlDist.RuntimeServer, opts})

lib/livebook/runtime/erl_dist/runtime_server.ex

@@ -47,6 +47,10 @@ defmodule Livebook.Runtime.ErlDist.RuntimeServer do
       those from file inputs. When not specified, operations relying
       on the directory are not possible
 
+    * `:base_path_env` - the value of `PATH` environment variable
+      to merge new values into when setting environment variables.
+      Defaults to `System.get_env("PATH", "")`
+
   """
   def start_link(opts \\ []) do
     GenServer.start_link(__MODULE__, opts)
@@ -300,7 +304,8 @@ defmodule Livebook.Runtime.ErlDist.RuntimeServer do
        extra_smart_cell_definitions: Keyword.get(opts, :extra_smart_cell_definitions, []),
        memory_timer_ref: nil,
        last_evaluator: nil,
-       initial_path: System.get_env("PATH", ""),
+       base_env_path:
+         Keyword.get_lazy(opts, :base_env_path, fn -> System.get_env("PATH", "") end),
        ebin_path: Keyword.get(opts, :ebin_path),
        tmp_dir: Keyword.get(opts, :tmp_dir)
      }}
@@ -574,7 +579,7 @@ defmodule Livebook.Runtime.ErlDist.RuntimeServer do
   def handle_cast({:put_system_envs, envs}, state) do
     envs
     |> Enum.map(fn
-      {"PATH", path} -> {"PATH", state.initial_path <> os_path_separator() <> path}
+      {"PATH", path} -> {"PATH", state.base_env_path <> os_path_separator() <> path}
       other -> other
     end)
     |> System.put_env()
@@ -585,7 +590,7 @@ defmodule Livebook.Runtime.ErlDist.RuntimeServer do
   def handle_cast({:delete_system_envs, names}, state) do
     names
     |> Enum.map(fn
-      "PATH" -> {"PATH", state.initial_path}
+      "PATH" -> {"PATH", state.base_env_path}
       name -> {name, nil}
     end)
     |> System.put_env()

lib/livebook/runtime/evaluator/io_proxy.ex

@@ -124,7 +124,13 @@ defmodule Livebook.Runtime.Evaluator.IOProxy do
 
   def handle_cast({:tracer_updates, updates}, state) do
     state = update_in(state.tracer_info, &Evaluator.Tracer.apply_updates(&1, updates))
-    {:noreply, state}
+
+    modules_defined =
+      for {:module_defined, module, _vars} <- updates,
+          into: state.modules_defined,
+          do: module
+
+    {:noreply, %{state | modules_defined: modules_defined}}
   end
 
   @impl true
@@ -133,12 +139,7 @@ defmodule Livebook.Runtime.Evaluator.IOProxy do
   end
 
   def handle_call(:get_tracer_info, _from, state) do
-    modules_defined =
-      state.tracer_info.modules_defined
-      |> Map.keys()
-      |> Enum.into(state.modules_defined)
-
-    {:reply, state.tracer_info, %{state | modules_defined: modules_defined}}
+    {:reply, state.tracer_info, state}
   end
 
   @impl true

test/livebook/intellisense_test.exs

@@ -148,7 +148,7 @@ defmodule Livebook.IntellisenseTest do
                detail: "module",
                documentation: "No documentation available",
                insert_text: "Elixir"
-             } in Intellisense.get_completion_items("E", context)
+             } in Intellisense.get_completion_items("Eli", context)
     end
 
     test "Elixir module completion" do

test/livebook/runtime/evaluator_test.exs

@@ -377,7 +377,7 @@ defmodule Livebook.Runtime.EvaluatorTest do
       defmodule Livebook.Runtime.EvaluatorTest.Exited do
       end
 
-      Task.async(fn -> raise "error" end)
+      Process.exit(self(), :kill)
       """
 
       {:group_leader, gl} = Process.info(evaluator.pid, :group_leader)

test/livebook/session_test.exs

@@ -397,7 +397,7 @@ defmodule Livebook.SessionTest do
       Session.set_file(session.pid, file)
 
       # Wait for the session to deal with the files
-      Process.sleep(500)
+      wait_for_session_update(session.pid)
 
       assert {:ok, true} =
                FileSystem.File.exists?(FileSystem.File.resolve(tmp_dir, "images/test.jpg"))
@@ -419,7 +419,7 @@ defmodule Livebook.SessionTest do
       Session.set_file(session.pid, nil)
 
       # Wait for the session to deal with the files
-      Process.sleep(500)
+      wait_for_session_update(session.pid)
 
       assert {:ok, true} = FileSystem.File.exists?(image_file)
 
@@ -483,8 +483,6 @@ defmodule Livebook.SessionTest do
 
       assert {:ok, false} = FileSystem.File.exists?(file)
 
-      Process.flag(:trap_exit, true)
-
       # Calling twice can happen in a race, make sure it doesn't crash
       Session.close(session.pid)
       Session.close([session.pid])
@@ -499,11 +497,11 @@ defmodule Livebook.SessionTest do
 
       assert {:ok, true} = FileSystem.File.exists?(images_dir)
 
-      Process.flag(:trap_exit, true)
       Session.close(session.pid)
 
       # Wait for the session to deal with the files
-      Process.sleep(50)
+      ref = Process.monitor(session.pid)
+      assert_receive {:DOWN, ^ref, :process, _, _}
 
       assert {:ok, false} = FileSystem.File.exists?(images_dir)
     end
@@ -952,4 +950,11 @@ defmodule Livebook.SessionTest do
     {:ok, runtime} = Livebook.Runtime.NoopRuntime.new() |> Livebook.Runtime.connect()
     runtime
   end
+
+  defp wait_for_session_update(session_pid) do
+    # This call is synchronous, so it gives the session time
+    # for handling the previously sent change messages.
+    Session.get_data(session_pid)
+    :ok
+  end
 end

test/livebook/sessions_test.exs

@@ -1,18 +1,22 @@
 defmodule Livebook.SessionsTest do
   use ExUnit.Case, async: true
 
-  alias Livebook.Sessions
+  alias Livebook.{Sessions, Session}
 
   describe "create_session/0" do
     test "starts a new session process under the sessions supervisor" do
       {:ok, session} = Sessions.create_session()
       assert has_child_with_pid?(Livebook.SessionSupervisor, session.pid)
+
+      Session.close(session.pid)
     end
 
     test "broadcasts a message to subscribers" do
       Sessions.subscribe()
-      {:ok, %{id: id}} = Sessions.create_session()
+      {:ok, %{id: id} = session} = Sessions.create_session()
       assert_receive {:session_created, %{id: ^id}}
+
+      Session.close(session.pid)
     end
   end
 
@@ -20,6 +24,8 @@ defmodule Livebook.SessionsTest do
     test "lists all sessions" do
       {:ok, session} = Sessions.create_session()
       assert session in Sessions.list_sessions()
+
+      Session.close(session.pid)
     end
   end
 
@@ -32,6 +38,8 @@ defmodule Livebook.SessionsTest do
     test "returns session matching the given id" do
       {:ok, session} = Sessions.create_session()
       assert {:ok, ^session} = Sessions.fetch_session(session.id)
+
+      Session.close(session.pid)
     end
   end
 
@@ -42,6 +50,8 @@ defmodule Livebook.SessionsTest do
       updated_session = %{session | notebook_name: "New name"}
       Livebook.Sessions.update_session(updated_session)
       assert_receive {:session_updated, %{id: ^id, notebook_name: "New name"}}
+
+      Session.close(session.pid)
     end
   end
 

test/livebook_web/controllers/session_controller_test.exs

@@ -53,6 +53,8 @@ defmodule LivebookWeb.SessionControllerTest do
 
       assert conn.status == 400
       assert conn.resp_body == "Invalid format, supported formats: livemd, exs"
+
+      Session.close(session.pid)
     end
 
     test "handles live markdown notebook source", %{conn: conn} do

test/livebook_web/live/home_live_test.exs

@@ -226,8 +226,6 @@ defmodule LivebookWeb.HomeLiveTest do
       refute render(view) =~ session1.id
       refute render(view) =~ session2.id
       refute render(view) =~ session3.id
-
-      Session.close([session1.pid, session2.pid, session3.pid])
     end
   end
 

test/livebook_web/live/learn_live_test.exs

@@ -15,14 +15,21 @@ defmodule LivebookWeb.LearnLiveTest do
 
     {:ok, view, _} = live(conn, to)
     assert render(view) =~ "Welcome to Livebook"
+
+    close_session_by_path(to)
   end
 
   test "link to a new notebook creates an empty session", %{conn: conn} do
-    {:ok, view, _} =
-      conn
-      |> live("/learn/notebooks/new")
-      |> follow_redirect(conn)
+    assert {:error, {:live_redirect, %{to: to}}} = result = live(conn, "/learn/notebooks/new")
+    {:ok, view, _} = follow_redirect(result, conn)
 
     assert render(view) =~ "Untitled notebook"
+
+    close_session_by_path(to)
+  end
+
+  defp close_session_by_path("/sessions/" <> session_id) do
+    {:ok, session} = Livebook.Sessions.fetch_session(session_id)
+    Livebook.Session.close(session.pid)
   end
 end

test/livebook_web/live/session_live_test.exs

@@ -1145,6 +1145,10 @@ defmodule LivebookWeb.SessionLiveTest do
     @tag :tmp_dir
     test "outputs persisted PATH delimited with os PATH env var",
          %{conn: conn, session: session, tmp_dir: tmp_dir} do
+      # Start the standalone runtime, to encapsulate env var changes
+      {:ok, runtime} = Runtime.ElixirStandalone.new() |> Runtime.connect()
+      Session.set_runtime(session.pid, runtime)
+
       separator =
         case :os.type() do
           {:win32, _} -> ";"
@@ -1162,17 +1166,16 @@ defmodule LivebookWeb.SessionLiveTest do
 
       section_id = insert_section(session.pid)
 
-      cell_id =
-        insert_text_cell(session.pid, section_id, :code, ~s{System.get_env("PATH") |> IO.write()})
+      cell_id = insert_text_cell(session.pid, section_id, :code, ~s{System.get_env("PATH")})
 
       view
       |> element(~s{[data-el-session]})
       |> render_hook("queue_cell_evaluation", %{"cell_id" => cell_id})
 
       assert_receive {:operation,
-                      {:add_cell_evaluation_output, _, ^cell_id, {:stdout, ^expected_path}}}
+                      {:add_cell_evaluation_response, _, ^cell_id, {:text, output}, _}}
 
-      assert_receive {:operation, {:add_cell_evaluation_response, _, ^cell_id, _, _}}
+      assert output == "\e[32m\"#{expected_path}\"\e[0m"
 
       Settings.unset_env_var("PATH")
 
@@ -1181,7 +1184,9 @@ defmodule LivebookWeb.SessionLiveTest do
       |> render_hook("queue_cell_evaluation", %{"cell_id" => cell_id})
 
       assert_receive {:operation,
-                      {:add_cell_evaluation_output, _, ^cell_id, {:stdout, ^initial_os_path}}}
+                      {:add_cell_evaluation_response, _, ^cell_id, {:text, output}, _}}
+
+      assert output == "\e[32m\"#{initial_os_path}\"\e[0m"
     end
   end