Do not run async groups on load and support --repeat-until-failure (#14107)

Author: josevalim

lib/ex_unit/lib/ex_unit/runner.ex

@@ -125,7 +125,7 @@ defmodule ExUnit.Runner do
 
         # Run all sync modules directly
         for pair <- sync_modules do
-          running = spawn_modules(config, [[pair]], false, %{})
+          running = spawn_modules(config, [{nil, [pair]}], false, %{})
           running != %{} and wait_until_available(config, running)
         end
 
@@ -161,7 +161,7 @@ defmodule ExUnit.Runner do
     running
   end
 
-  defp spawn_modules(config, [[_ | _] = modules | groups], async?, running) do
+  defp spawn_modules(config, [{_group, [_ | _] = modules} | groups], async?, running) do
     if max_failures_reached?(config) do
       running
     else

lib/ex_unit/lib/ex_unit/server.ex

@@ -57,9 +57,10 @@ defmodule ExUnit.Server do
     state = %{
       loaded: System.monotonic_time(),
       waiting: nil,
-      async_groups: %{},
+      groups: %{},
+      async_groups: [],
       async_modules: :queue.new(),
-      sync_modules: :queue.new()
+      sync_modules: []
     }
 
     {:ok, state}
@@ -72,31 +73,31 @@ 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: async_modules} = state
-    0 = :queue.len(async_modules)
+    %{waiting: nil, loaded: :done, async_groups: []} = state
+    true = :queue.is_empty(state.async_modules)
 
-    {:reply, :queue.to_list(state.sync_modules),
-     %{state | sync_modules: :queue.new(), loaded: System.monotonic_time()}}
+    {:reply, state.sync_modules, %{state | sync_modules: [], loaded: System.monotonic_time()}}
   end
 
   # Called by the runner when --repeat-until-failure is used.
   def handle_call({:restore_modules, async_modules, sync_modules}, _from, state) do
-    {async_modules, async_groups} =
-      Enum.map_reduce(async_modules, %{}, fn
-        {nil, [module]}, {modules, groups} ->
-          {[{:module, module} | modules], groups}
+    {async_modules, async_groups, groups} =
+      Enum.reduce(async_modules, {[], [], []}, fn
+        {nil, [module]}, {async_modules, async_groups, groups} ->
+          {[module | async_modules], async_groups, groups}
 
-        {group, group_modules}, {modules, groups} ->
-          {[{:group, group} | modules], Map.put(groups, group, group_modules)}
+        {group, group_modules}, {async_modules, async_groups, groups} ->
+          {async_modules, [group | async_groups], [{group, group_modules} | groups]}
       end)
 
     {:reply, :ok,
      %{
        state
        | loaded: :done,
+         groups: Map.new(groups),
          async_groups: async_groups,
          async_modules: :queue.from_list(async_modules),
-         sync_modules: :queue.from_list(sync_modules)
+         sync_modules: sync_modules
      }}
   end
 
@@ -108,22 +109,24 @@ defmodule ExUnit.Server do
       when is_integer(loaded) do
     state =
       if uniq? do
-        async_groups =
-          Map.new(state.async_groups, fn {group, modules} ->
-            {group, Enum.uniq(modules)}
-          end)
-
+        groups = Map.new(state.groups, fn {group, modules} -> {group, Enum.uniq(modules)} end)
+        async_groups = state.async_groups |> Enum.uniq() |> Enum.reverse()
         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()
+        sync_modules = state.sync_modules |> Enum.uniq() |> Enum.reverse()
 
         %{
           state
-          | async_groups: async_groups,
+          | groups: groups,
+            async_groups: async_groups,
             async_modules: async_modules,
             sync_modules: sync_modules
         }
       else
-        state
+        %{
+          state
+          | async_groups: Enum.reverse(state.async_groups),
+            sync_modules: Enum.reverse(state.sync_modules)
+        }
       end
 
     diff = System.convert_time_unit(System.monotonic_time() - loaded, :native, :microsecond)
@@ -132,9 +135,7 @@ defmodule ExUnit.Server do
 
   def handle_call({:add, false = _async, _group, names}, _from, %{loaded: loaded} = state)
       when is_integer(loaded) do
-    state =
-      update_in(state.sync_modules, &Enum.reduce(names, &1, fn name, q -> :queue.in(name, q) end))
-
+    state = update_in(state.sync_modules, &Enum.reverse(names, &1))
     {:reply, :ok, state}
   end
 
@@ -143,25 +144,24 @@ defmodule ExUnit.Server do
     state =
       update_in(
         state.async_modules,
-        &Enum.reduce(names, &1, fn name, q -> :queue.in({:module, name}, q) end)
+        &Enum.reduce(names, &1, fn name, q -> :queue.in(name, q) end)
       )
 
     {:reply, :ok, take_modules(state)}
   end
 
   def handle_call({:add, true = _async, group, names}, _from, %{loaded: loaded} = state)
       when is_integer(loaded) do
-    {async_groups, async_modules} =
-      case state.async_groups do
-        %{^group => entries} = async_groups ->
-          {%{async_groups | group => names ++ entries}, state.async_modules}
+    {groups, async_groups} =
+      case state.groups do
+        %{^group => entries} = groups ->
+          {%{groups | group => Enum.reverse(names, entries)}, state.async_groups}
 
-        %{} = async_groups ->
-          {Map.put(async_groups, group, names), :queue.in({:group, group}, state.async_modules)}
+        %{} = groups ->
+          {Map.put(groups, group, names), [group | state.async_groups]}
       end
 
-    {:reply, :ok,
-     take_modules(%{state | async_groups: async_groups, async_modules: async_modules})}
+    {:reply, :ok, take_modules(%{state | groups: groups, async_groups: async_groups})}
   end
 
   def handle_call({:add, _async?, _group, _names}, _from, state) do
@@ -174,49 +174,44 @@ defmodule ExUnit.Server do
 
   defp take_modules(%{waiting: {from, count}} = state) do
     has_async_modules? = not :queue.is_empty(state.async_modules)
+    has_async_groups? = state.async_groups != []
 
     cond do
-      not has_async_modules? and state.loaded == :done ->
+      not has_async_modules? and not has_async_groups? and state.loaded == :done ->
         GenServer.reply(from, nil)
         %{state | waiting: nil}
 
-      not has_async_modules? ->
-        state
-
-      true ->
-        {async_modules, remaining_modules} = take_until(count, state.async_modules)
+      has_async_modules? ->
+        {reply, remaining_modules} = take_until(count, state.async_modules)
+        GenServer.reply(from, reply)
+        %{state | async_modules: remaining_modules, waiting: nil}
 
-        {async_modules, remaining_groups} =
-          Enum.map_reduce(async_modules, state.async_groups, fn
-            {:module, module}, async_groups ->
-              {[module], async_groups}
+      has_async_groups? ->
+        {groups, remaining_groups} = Enum.split(state.async_groups, count)
 
-            {:group, group}, async_groups ->
-              {group_modules, async_groups} = Map.pop!(async_groups, group)
-              {Enum.reverse(group_modules), async_groups}
+        {reply, groups} =
+          Enum.map_reduce(groups, state.groups, fn group, acc ->
+            {entries, acc} = Map.pop!(acc, group)
+            {{group, Enum.reverse(entries)}, acc}
           end)
 
-        GenServer.reply(from, async_modules)
+        GenServer.reply(from, reply)
+        %{state | groups: groups, async_groups: remaining_groups, waiting: nil}
 
-        %{
-          state
-          | async_groups: remaining_groups,
-            async_modules: remaining_modules,
-            waiting: nil
-        }
+      true ->
+        state
     end
   end
 
-  # :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}
+  # :queue.split fails if the provided count is larger than the queue size.
+  # We also want to return the values as tuples of shape {group, [modules]}.
   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])
+      {{:value, item}, queue} -> take_until(n - 1, queue, [{nil, [item]} | acc])
       {:empty, queue} -> {Enum.reverse(acc), queue}
     end
   end

lib/ex_unit/test/ex_unit_test.exs

@@ -997,6 +997,24 @@ defmodule ExUnitTest do
       assert length(runs) == 6
     end
 
+    test "repeats tests up to the configured number of times with groups" do
+      defmodule TestGroupedRepeatUntilFailureReached do
+        use ExUnit.Case, async: true, group: :example
+        test __ENV__.line, do: assert(true)
+      end
+
+      configure_and_reload_on_exit(repeat_until_failure: 5)
+
+      output =
+        capture_io(fn ->
+          assert ExUnit.run() == %{total: 1, failures: 0, skipped: 0, excluded: 0}
+        end)
+
+      runs = String.split(output, "Running ExUnit", trim: true)
+      # 6 runs in total, 5 repeats
+      assert length(runs) == 6
+    end
+
     test "stops on failure" do
       {:ok, pid} = Agent.start_link(fn -> 0 end)
       Process.register(pid, :ex_unit_repeat_until_failure_count)