use a :queue to store modules in ExUnit.Server (#13636)

In order to have more deterministic test runs when using `--max-cases 1`
and `--max-requires 1` (#13635)
(see also #13589), we need to
run tests in compilation order (FIFO).

In the past, ExUnit.Server appended new tests to the front of a list,
which would result in the most recently added test to be run first.

Let's quickly demonstrate the problem this causes for deterministic runs
with a simple example:

Imagine a test (let's call if FooTest) that takes a non-deterministic
amount of time to run. For now let's assume that it sometimes takes 1
second and sometimes up to 5. And as async tests execute in parallel with
compilation of other test files, we could have the following scenario:

FooTest is compiled and because it's async it is immediately started.
It takes 1 second to run.
In this 1 second two more tests are compiled. First BarTest is prepended
to the list, then BazTest.
The order of test runs now is:

FooTest, BazTest, then whatever is last compiled while BazTest runs, ...

Now another run, FooTest takes 5 seconds to run.
While FooTest runs, more than two other tests are compiled.
The order of test runs is:

FooTest, LastCompiledTest, SecondLastCompiledTest, ..., BazTest, BarTest

This can be fixed either by appending new test modules to the end of the
list, or - and that's what this commit does - by using a `:queue`
instead.

Author: SteffenDE

lib/elixir/test/elixir/exception_test.exs

@@ -430,8 +430,8 @@ defmodule ExceptionTest do
         end
       )
 
-      :code.delete(OperatorPrecedence)
       :code.purge(OperatorPrecedence)
+      :code.delete(OperatorPrecedence)
 
       assert blame_message(OperatorPrecedence, & &1.test!(1, 2)) =~ """
              no function clause matching in ExceptionTest.OperatorPrecedence.test!/2
@@ -469,8 +469,8 @@ defmodule ExceptionTest do
         end
       )
 
-      :code.delete(Req)
       :code.purge(Req)
+      :code.delete(Req)
 
       assert blame_message(Req, & &1.get!(url: "https://elixir-lang.org")) =~ """
              no function clause matching in ExceptionTest.Req.get!/1
@@ -623,8 +623,8 @@ defmodule ExceptionTest do
              """
 
       for module <- modules do
-        :code.delete(module)
         :code.purge(module)
+        :code.delete(module)
       end
     end
 
@@ -839,8 +839,8 @@ defmodule ExceptionTest do
         end
       )
 
-      :code.delete(Blaming)
       :code.purge(Blaming)
+      :code.delete(Blaming)
 
       {:ok, :def, clauses} = Exception.blame_mfa(Blaming, :with_elem, [1, 2])
 

lib/elixir/test/elixir/inspect_test.exs

@@ -795,8 +795,8 @@ defmodule Inspect.OthersTest do
     Application.put_env(:elixir, :anony, V.fun())
     Application.put_env(:elixir, :named, &V.fun/0)
 
-    :code.delete(V)
     :code.purge(V)
+    :code.delete(V)
 
     anony = Application.get_env(:elixir, :anony)
     named = Application.get_env(:elixir, :named)

lib/elixir/test/elixir/kernel/raise_test.exs

@@ -434,8 +434,8 @@ defmodule Kernel.RaiseTest do
 
       fun = BadFunction.Missing.fun()
 
-      :code.delete(BadFunction.Missing)
       :code.purge(BadFunction.Missing)
+      :code.delete(BadFunction.Missing)
 
       defmodule BadFunction.Missing do
         def fun, do: fn -> :another end

lib/elixir/test/elixir/kernel/warning_test.exs

@@ -2233,7 +2233,7 @@ defmodule Kernel.WarningTest do
   end
 
   defp purge(module) when is_atom(module) do
-    :code.delete(module)
     :code.purge(module)
+    :code.delete(module)
   end
 end

lib/elixir/test/elixir/kernel_test.exs

@@ -11,8 +11,8 @@ defmodule KernelTest do
   def empty_map, do: %{}
 
   defp purge(module) do
-    :code.delete(module)
     :code.purge(module)
+    :code.delete(module)
   end
 
   defp assert_eval_raise(error, msg, string) do

lib/elixir/test/elixir/record_test.exs

@@ -374,8 +374,8 @@ defmodule RecordTest do
     end
 
     defp purge(module) when is_atom(module) do
-      :code.delete(module)
       :code.purge(module)
+      :code.delete(module)
     end
   end
 end

lib/elixir/test/elixir/typespec_test.exs

@@ -14,8 +14,8 @@ defmodule TypespecTest do
           unquote(block)
         end
 
-      :code.delete(TypespecSample)
       :code.purge(TypespecSample)
+      :code.delete(TypespecSample)
       bytecode
     end
   end
@@ -1070,8 +1070,8 @@ defmodule TypespecTest do
                 {TypeModuleAttributes, _}}
              ] = TypeModuleAttributes.typep2()
     after
-      :code.delete(TypeModuleAttributes)
       :code.purge(TypeModuleAttributes)
+      :code.delete(TypeModuleAttributes)
     end
 
     test "@spec, @callback, and @macrocallback as module attributes" do
@@ -1129,8 +1129,8 @@ defmodule TypespecTest do
                 {SpecModuleAttributes, _}}
              ] = SpecModuleAttributes.macrocallback()
     after
-      :code.delete(SpecModuleAttributes)
       :code.purge(SpecModuleAttributes)
+      :code.delete(SpecModuleAttributes)
     end
 
     test "@callback" do

lib/ex_unit/lib/ex_unit/server.ex

@@ -51,8 +51,8 @@ defmodule ExUnit.Server do
     state = %{
       loaded: System.monotonic_time(),
       waiting: nil,
-      async_modules: [],
-      sync_modules: []
+      async_modules: :queue.new(),
+      sync_modules: :queue.new()
     }
 
     {:ok, state}
@@ -65,8 +65,11 @@ defmodule ExUnit.Server do
 
   # Called once after all async modules have been sent and reverts the state.
   def handle_call(:take_sync_modules, _from, state) do
-    %{waiting: nil, loaded: :done, async_modules: []} = state
-    {:reply, state.sync_modules, %{state | sync_modules: [], loaded: System.monotonic_time()}}
+    %{waiting: nil, loaded: :done, async_modules: async_modules} = state
+    0 = :queue.len(async_modules)
+
+    {:reply, :queue.to_list(state.sync_modules),
+     %{state | sync_modules: :queue.new(), loaded: System.monotonic_time()}}
   end
 
   # Called by the runner when --repeat-until-failure is used.
@@ -75,8 +78,8 @@ defmodule ExUnit.Server do
      %{
        state
        | loaded: :done,
-         async_modules: async_modules,
-         sync_modules: sync_modules
+         async_modules: :queue.from_list(async_modules),
+         sync_modules: :queue.from_list(sync_modules)
      }}
   end
 
@@ -88,10 +91,13 @@ defmodule ExUnit.Server do
       when is_integer(loaded) do
     state =
       if uniq? do
+        async_modules = :queue.to_list(state.async_modules) |> Enum.uniq() |> :queue.from_list()
+        sync_modules = :queue.to_list(state.sync_modules) |> Enum.uniq() |> :queue.from_list()
+
         %{
           state
-          | async_modules: Enum.uniq(state.async_modules),
-            sync_modules: Enum.uniq(state.sync_modules)
+          | async_modules: async_modules,
+            sync_modules: sync_modules
         }
       else
         state
@@ -103,13 +109,20 @@ defmodule ExUnit.Server do
 
   def handle_call({:add, true, names}, _from, %{loaded: loaded} = state)
       when is_integer(loaded) do
-    state = update_in(state.async_modules, &(names ++ &1))
+    state =
+      update_in(
+        state.async_modules,
+        &Enum.reduce(names, &1, fn name, q -> :queue.in(name, q) end)
+      )
+
     {:reply, :ok, take_modules(state)}
   end
 
   def handle_call({:add, false, names}, _from, %{loaded: loaded} = state)
       when is_integer(loaded) do
-    state = update_in(state.sync_modules, &(names ++ &1))
+    state =
+      update_in(state.sync_modules, &Enum.reduce(names, &1, fn name, q -> :queue.in(name, q) end))
+
     {:reply, :ok, state}
   end
 
@@ -120,18 +133,35 @@ defmodule ExUnit.Server do
     state
   end
 
-  defp take_modules(%{waiting: {from, _count}, async_modules: [], loaded: :done} = state) do
-    GenServer.reply(from, nil)
-    %{state | waiting: nil}
-  end
+  defp take_modules(%{waiting: {from, count}} = state) do
+    has_async_modules? = not :queue.is_empty(state.async_modules)
 
-  defp take_modules(%{async_modules: []} = state) do
-    state
+    cond do
+      not has_async_modules? and state.loaded == :done ->
+        GenServer.reply(from, nil)
+        %{state | waiting: nil}
+
+      not has_async_modules? ->
+        state
+
+      true ->
+        {modules, async_modules} = take_until(count, state.async_modules)
+        GenServer.reply(from, modules)
+        %{state | async_modules: async_modules, waiting: nil}
+    end
   end
 
-  defp take_modules(%{waiting: {from, count}, async_modules: modules} = state) do
-    {reply, modules} = Enum.split(modules, count)
-    GenServer.reply(from, reply)
-    %{state | async_modules: modules, waiting: nil}
+  # :queue.split fails if the provided count is larger than the queue size;
+  # as we also want to return the values as a list later, we directly
+  # return {list, queue} instead of {queue, queue}
+  defp take_until(n, queue), do: take_until(n, queue, [])
+
+  defp take_until(0, queue, acc), do: {Enum.reverse(acc), queue}
+
+  defp take_until(n, queue, acc) do
+    case :queue.out(queue) do
+      {{:value, item}, queue} -> take_until(n - 1, queue, [item | acc])
+      {:empty, queue} -> {Enum.reverse(acc), queue}
+    end
   end
 end

lib/ex_unit/test/ex_unit/assertions_test.exs

@@ -199,8 +199,8 @@ defmodule ExUnit.AssertionsTest do
              """)
            end) =~ "variable \"var\" is unused"
   after
-    :code.delete(ExSample)
     :code.purge(ExSample)
+    :code.delete(ExSample)
   end
 
   test "assert match with quote on left-side" do

lib/ex_unit/test/ex_unit_test.exs

@@ -1054,6 +1054,45 @@ defmodule ExUnitTest do
     assert output =~ "2 tests, 0 failures, 2 excluded\n"
   end
 
+  test "tests are run in compile order (FIFO)" do
+    defmodule FirstTestFIFO do
+      use ExUnit.Case
+
+      test "first test" do
+        assert true
+      end
+    end
+
+    defmodule SecondTestFIFO do
+      use ExUnit.Case
+
+      test "second test" do
+        assert true
+      end
+    end
+
+    defmodule ThirdTestFIFO do
+      use ExUnit.Case
+
+      test "third test" do
+        assert true
+      end
+    end
+
+    configure_and_reload_on_exit(trace: true)
+
+    output =
+      capture_io(fn ->
+        assert ExUnit.run() == %{total: 3, failures: 0, excluded: 0, skipped: 0}
+      end)
+
+    [_, first, second, third | _] = String.split(output, "\n\n")
+
+    assert first =~ "FirstTestFIFO"
+    assert second =~ "SecondTestFIFO"
+    assert third =~ "ThirdTestFIFO"
+  end
+
   ##  Helpers
 
   defp run_with_filter(filters, cases) do