Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions lib/ex_unit/lib/ex_unit/case.ex
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ defmodule ExUnit.Case do
It should be enabled only if tests do not change any global state.
Defaults to `false`.

* `:group` - configures async tests in this module to run within a group.
Tests in the same group never run concurrently. Tests from different
groups can run concurrently with `async: true`.
Defaults to `nil`.

* `:register` - when `false`, does not register this module within
ExUnit server. This means the module won't run when ExUnit suite runs.

Expand Down Expand Up @@ -294,7 +299,7 @@ defmodule ExUnit.Case do

@type env :: module() | Macro.Env.t()
@compile {:no_warn_undefined, [IEx.Pry]}
@reserved [:module, :file, :line, :test, :async, :registered, :describe]
@reserved [:module, :file, :line, :test, :async, :group, :registered, :describe]

@doc false
defmacro __using__(opts) do
Expand All @@ -317,7 +322,7 @@ defmodule ExUnit.Case do
end

{register?, opts} = Keyword.pop(opts, :register, true)
{next_opts, opts} = Keyword.split(opts, [:async, :parameterize])
{next_opts, opts} = Keyword.split(opts, [:async, :group, :parameterize])

if opts != [] do
IO.warn("unknown options given to ExUnit.Case: #{inspect(opts)}")
Expand Down Expand Up @@ -552,6 +557,7 @@ defmodule ExUnit.Case do

opts = Module.get_attribute(module, :ex_unit_module, [])
async? = Keyword.get(opts, :async, false)
group = Keyword.get(opts, :group, nil)
parameterize = Keyword.get(opts, :parameterize, nil)

if not (parameterize == nil or (is_list(parameterize) and Enum.all?(parameterize, &is_map/1))) do
Expand All @@ -569,7 +575,11 @@ defmodule ExUnit.Case do
end

def __ex_unit__(:config) do
{unquote(async?), unquote(Macro.escape(parameterize))}
%{
async?: unquote(async?),
group: unquote(Macro.escape(group)),
parameterize: unquote(Macro.escape(parameterize))
}
end
end
end
Expand Down
22 changes: 21 additions & 1 deletion lib/ex_unit/lib/ex_unit/runner.ex
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,27 @@ defmodule ExUnit.Runner do
running
end

defp spawn_modules(config, [{module, params} | modules], async?, running) do
defp spawn_modules(
config,
[{_group, group_modules} | modules],
Copy link
Member

@josevalim josevalim Oct 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sync modules are not arriving in this shape and they are being skipped (because the group modules is treated as an empty map). I will add a pattern to help us catch it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, that masked the other issues!
Thanks for fixing it.

And bummer about the tests being flaky, it would have been nice to have tests for this functionality.

true = async?,
running
) do
if max_failures_reached?(config) do
running
else
{pid, ref} =
spawn_monitor(fn ->
Enum.each(group_modules, fn {module, params} ->
run_module(config, module, async?, params)
end)
end)

spawn_modules(config, modules, async?, Map.put(running, ref, pid))
end
end

defp spawn_modules(config, [{_group, [{module, params}]} | modules], async?, running) do
if max_failures_reached?(config) do
running
else
Expand Down
90 changes: 76 additions & 14 deletions lib/ex_unit/lib/ex_unit/server.ex
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,21 @@ defmodule ExUnit.Server do
GenServer.start_link(__MODULE__, :ok, name: @name)
end

def add_module(name, {async?, parameterize}) do
def add_module(name, config) do
%{
async?: async?,
group: group,
parameterize: parameterize
} = config

modules =
if parameterize do
Enum.map(parameterize, &{name, &1})
else
[{name, %{}}]
end

case GenServer.call(@name, {:add, async?, modules}, @timeout) do
case GenServer.call(@name, {:add, {async?, group}, modules}, @timeout) do
:ok ->
:ok

Expand Down Expand Up @@ -51,6 +57,7 @@ defmodule ExUnit.Server do
state = %{
loaded: System.monotonic_time(),
waiting: nil,
async_groups: MapSet.new(),
async_modules: :queue.new(),
sync_modules: :queue.new()
}
Expand Down Expand Up @@ -78,6 +85,7 @@ defmodule ExUnit.Server do
%{
state
| loaded: :done,
async_groups: MapSet.new(async_modules, fn {group, _} -> group end),
async_modules: :queue.from_list(async_modules),
sync_modules: :queue.from_list(sync_modules)
}}
Expand All @@ -91,7 +99,15 @@ 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()
async_modules =
state.async_modules
|> :queue.to_list()
|> Enum.uniq()
|> Enum.map(fn {group, modules} ->
{group, Enum.uniq(modules)}
end)
|> :queue.from_list()

sync_modules = :queue.to_list(state.sync_modules) |> Enum.uniq() |> :queue.from_list()

%{
Expand All @@ -107,28 +123,59 @@ defmodule ExUnit.Server do
{:reply, diff, take_modules(%{state | loaded: :done})}
end

def handle_call({:add, true, names}, _from, %{loaded: loaded} = state)
def handle_call({:add, {true, group}, names}, _from, %{loaded: loaded} = state)
when is_integer(loaded) do
state =
update_in(
state.async_modules,
&Enum.reduce(names, &1, fn name, q -> :queue.in(name, q) end)
)
Enum.reduce(names, state, fn name, updated_state ->
group = group || default_group(name)
group_exists? = MapSet.member?(updated_state.async_groups, group)

if group_exists? do
update_in(updated_state.async_modules, fn q ->
q
|> :queue.to_list()
|> Enum.map(fn
{^group, modules} ->
{group, [name | modules]}

other ->
other
end)
|> :queue.from_list()
end)
else
updated_state
|> update_in([:async_modules], &:queue.in({group, [name]}, &1))
|> update_in([:async_groups], &MapSet.put(&1, group))
end
end)

{:reply, :ok, take_modules(state)}
end

def handle_call({:add, false, names}, _from, %{loaded: loaded} = state)
def handle_call({:add, {false, 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))
update_in(
state.sync_modules,
&Enum.reduce(names, &1, fn name, q -> :queue.in({group, [name]}, q) end)
)

{:reply, :ok, state}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We lost a call to take_modules here.

end

def handle_call({:add, _async?, _names}, _from, state),
def handle_call({:add, {_async?, _group}, _names}, _from, state),
do: {:reply, :already_running, state}

defp default_group({module, params}) do
if params == %{} do
module
else
# if no group is specified, parameters need to run concurrently
"#{module}_#{:erlang.phash2(params)}"
end
end

defp take_modules(%{waiting: nil} = state) do
state
end
Expand All @@ -145,9 +192,24 @@ defmodule ExUnit.Server do
state

true ->
{modules, async_modules} = take_until(count, state.async_modules)
GenServer.reply(from, modules)
%{state | async_modules: async_modules, waiting: nil}
{async_modules, remaining_modules} = take_until(count, state.async_modules)

remaining_groups =
Enum.reduce(async_modules, state.async_groups, fn {group, _modules}, groups ->
MapSet.delete(groups, group)
end)

async_modules =
Enum.map(async_modules, fn {group, modules} -> {group, Enum.reverse(modules)} end)

GenServer.reply(from, async_modules)

%{
state
| async_groups: remaining_groups,
async_modules: remaining_modules,
waiting: nil
}
end
end

Expand Down
Loading