diff --git a/lib/ex_unit/lib/ex_unit/failures_manifest.ex b/lib/ex_unit/lib/ex_unit/failures_manifest.ex index 239b63201f..7d2af1c2cb 100644 --- a/lib/ex_unit/lib/ex_unit/failures_manifest.ex +++ b/lib/ex_unit/lib/ex_unit/failures_manifest.ex @@ -5,30 +5,41 @@ defmodule ExUnit.FailuresManifest do @moduledoc false - @opaque t :: %{optional(ExUnit.test_id()) => test_file :: Path.t()} + @opaque t :: {passed, failed} + + @typep passed :: [ExUnit.test_id()] + @typep failed :: %{optional(ExUnit.test_id()) => test_file :: Path.t()} @manifest_vsn 1 @spec new() :: t - def new, do: %{} + def new, do: {[], %{}} @spec put_test(t, ExUnit.Test.t()) :: t - def put_test(%{} = manifest, %ExUnit.Test{state: {ignored_state, _}}) + def put_test({_passed, _failed} = manifest, %ExUnit.Test{state: {ignored_state, _}}) when ignored_state in [:skipped, :excluded], do: manifest - def put_test(%{} = manifest, %ExUnit.Test{state: nil} = test) do - Map.delete(manifest, {test.module, test.name}) + def put_test({passed, failed}, %ExUnit.Test{state: nil} = test) do + test_id = {test.module, test.name} + {[test_id | passed], failed} end - def put_test(%{} = manifest, %ExUnit.Test{state: {failed_state, _}} = test) + def put_test({passed, failed}, %ExUnit.Test{state: {failed_state, _}} = test) when failed_state in [:failed, :invalid] do - Map.put(manifest, {test.module, test.name}, test.tags.file) + test_id = {test.module, test.name} + {passed, Map.put(failed, test_id, test.tags.file)} end - @spec write!(t, Path.t()) :: :ok - def write!(manifest, file) when is_binary(file) do - manifest = prune_deleted_tests(manifest) + @spec update!(t, Path.t()) :: :ok + def update!({passed, failed}, file) when is_binary(file) do + manifest = + file + |> read() + |> prune_deleted_tests() + |> Map.drop(passed) + |> Map.merge(failed) + binary = :erlang.term_to_binary({@manifest_vsn, manifest}) Path.dirname(file) |> File.mkdir_p!() File.write!(file, binary) @@ -41,13 +52,13 @@ defmodule ExUnit.FailuresManifest do File.write!(file, binary) end - @spec read(Path.t()) :: t + @spec read(Path.t()) :: failed def read(file) when is_binary(file) do with {:ok, binary} <- File.read(file), {:ok, {@manifest_vsn, %{} = manifest}} <- safe_binary_to_term(binary) do manifest else - _ -> new() + _ -> %{} end end diff --git a/lib/ex_unit/lib/ex_unit/runner_stats.ex b/lib/ex_unit/lib/ex_unit/runner_stats.ex index 7c61f5fe7d..4a8de3222a 100644 --- a/lib/ex_unit/lib/ex_unit/runner_stats.ex +++ b/lib/ex_unit/lib/ex_unit/runner_stats.ex @@ -73,15 +73,9 @@ defmodule ExUnit.RunnerStats do {:noreply, state} end - def handle_cast({:suite_started, _opts}, %{failures_manifest_path: file} = state) - when is_binary(file) do - state = %{state | failures_manifest: FailuresManifest.read(file)} - {:noreply, state} - end - def handle_cast({:suite_finished, _}, %{failures_manifest_path: file} = state) when is_binary(file) do - FailuresManifest.write!(state.failures_manifest, file) + FailuresManifest.update!(state.failures_manifest, file) {:noreply, state} end diff --git a/lib/ex_unit/test/ex_unit/failures_manifest_test.exs b/lib/ex_unit/test/ex_unit/failures_manifest_test.exs index 219f723d36..569e5d27d0 100644 --- a/lib/ex_unit/test/ex_unit/failures_manifest_test.exs +++ b/lib/ex_unit/test/ex_unit/failures_manifest_test.exs @@ -27,7 +27,7 @@ defmodule ExUnit.FailuresManifestTest do |> put_test(invalid_1 = new_test(@invalid, context)) File.cd!(context.tmp_dir, fn -> - write!(manifest, @manifest_path) + update!(manifest, @manifest_path) assert info(@manifest_path) == {MapSet.new([context.file]), @@ -52,18 +52,19 @@ defmodule ExUnit.FailuresManifestTest do end describe "put_test/2 when the test is not already in the manifest" do - test "ignores passed tests since we only care to store failures" do - assert put_test(new(), new_test(@passed)) == new() + test "records passed tests so they clear failures from a prior run" do + test = new_test(@passed) + assert put_test(new(), test) == {[test_id(test)], %{}} end test "stores failed tests" do test = new_test(@failed) - assert put_test(new(), test) == %{test_id(test) => file(test)} + assert put_test(new(), test) == {[], %{test_id(test) => file(test)}} end test "stores invalid tests" do test = new_test(@invalid) - assert put_test(new(), test) == %{test_id(test) => file(test)} + assert put_test(new(), test) == {[], %{test_id(test) => file(test)}} end test "ignores skipped tests since we know nothing about their pass/fail status" do @@ -82,19 +83,19 @@ defmodule ExUnit.FailuresManifestTest do {:ok, %{failed_test: failed_test, manifest: manifest}} end - test "removes a newly passed test, since it is no longer failing", context do + test "stores passing test separately", context do test = %{context.failed_test | state: @passed} - assert put_test(context.manifest, test) == new() + assert put_test(context.manifest, test) == {[test_id(test)], %{test_id(test) => "file"}} end test "stores failed tests, updating the stored file value", context do test = %{context.failed_test | tags: %{file: "some-other-file"}} - assert put_test(context.manifest, test) == %{test_id(test) => file(test)} + assert put_test(context.manifest, test) == {[], %{test_id(test) => file(test)}} end test "stores invalid tests, updating the stored file value", context do test = %{context.failed_test | tags: %{file: "some-other-file"}, state: @invalid} - assert put_test(context.manifest, test) == %{test_id(test) => file(test)} + assert put_test(context.manifest, test) == {[], %{test_id(test) => file(test)}} end test "ignores skipped tests since we know nothing about their pass/fail status", context do @@ -108,27 +109,40 @@ defmodule ExUnit.FailuresManifestTest do end end - describe "write!/2" do + describe "update!/2" do @tag :tmp_dir test "stores a manifest that can later be read with read/1", context do manifest = non_blank_manifest(context) File.cd!(context.tmp_dir, fn -> - assert write!(manifest, @manifest_path) == :ok - assert read(@manifest_path) == manifest + assert update!(manifest, @manifest_path) == :ok + assert read(@manifest_path) == elem(manifest, 1) end) end @tag :tmp_dir - test "prunes tests from files that no longer exist", context do + test "merges the results from this run with the prior manifest", context do + failed_test = new_test(@failed, context) + passed_test = %{failed_test | state: @passed} + + File.cd!(context.tmp_dir, fn -> + assert update!(put_test(new(), failed_test), @manifest_path) == :ok + assert update!(put_test(new(), passed_test), @manifest_path) == :ok + assert read(@manifest_path) == %{} + end) + end + + @tag :tmp_dir + test "prunes tests from files that no longer exist in the prior manifest", context do test = new_test(@failed, %{context | file: "missing_file.exs"}) File.cd!(context.tmp_dir, fn -> - new() - |> put_test(test) - |> write!(@manifest_path) + binary = :erlang.term_to_binary({1, %{test_id(test) => file(test)}}) + File.write!(@manifest_path, binary) + + update!(new(), @manifest_path) - assert read(@manifest_path) == new() + assert read(@manifest_path) == %{} end) end @@ -138,21 +152,22 @@ defmodule ExUnit.FailuresManifestTest do manifest = new() |> put_test(test) File.cd!(context.tmp_dir, fn -> - write!(manifest, @manifest_path) - assert read(@manifest_path) == manifest + update!(manifest, @manifest_path) + assert read(@manifest_path) == elem(manifest, 1) end) end @tag :tmp_dir - test "prunes tests defined in a function that no longer exists", context do + test "prunes tests from functions that no longer exist in the prior manifest", context do test = new_test(@failed, %{context | test: :not_a_function_anymore}) File.cd!(context.tmp_dir, fn -> - new() - |> put_test(test) - |> write!(@manifest_path) + binary = :erlang.term_to_binary({1, %{test_id(test) => file(test)}}) + File.write!(@manifest_path, binary) + + update!(new(), @manifest_path) - assert read(@manifest_path) == new() + assert read(@manifest_path) == %{} end) end end @@ -162,7 +177,7 @@ defmodule ExUnit.FailuresManifestTest do test "returns a blank manifest when loading a file that does not exit", context do path = Path.join(context.tmp_dir, "missing.manifest") refute File.exists?(path) - assert read(path) == new() + assert read(path) == %{} end @tag :tmp_dir @@ -170,10 +185,10 @@ defmodule ExUnit.FailuresManifestTest do manifest = non_blank_manifest(context) File.cd!(context.tmp_dir, fn -> - assert write!(manifest, @manifest_path) == :ok + assert update!(manifest, @manifest_path) == :ok corrupted = "corrupted" <> File.read!(@manifest_path) File.write!(@manifest_path, corrupted) - assert read(@manifest_path) == new() + assert read(@manifest_path) == %{} end) end @@ -182,10 +197,11 @@ defmodule ExUnit.FailuresManifestTest do manifest = non_blank_manifest(context) File.cd!(context.tmp_dir, fn -> - assert write!(manifest, @manifest_path) == :ok - assert {vsn, ^manifest} = @manifest_path |> File.read!() |> :erlang.binary_to_term() - File.write!(@manifest_path, :erlang.term_to_binary({vsn + 1, manifest})) - assert read(@manifest_path) == new() + assert update!(manifest, @manifest_path) == :ok + assert {vsn, failures} = @manifest_path |> File.read!() |> :erlang.binary_to_term() + assert failures == elem(manifest, 1) + File.write!(@manifest_path, :erlang.term_to_binary({vsn + 1, failures})) + assert read(@manifest_path) == %{} end) end end diff --git a/lib/ex_unit/test/ex_unit/runner_stats_test.exs b/lib/ex_unit/test/ex_unit/runner_stats_test.exs index 05db0642ee..d1114758e3 100644 --- a/lib/ex_unit/test/ex_unit/runner_stats_test.exs +++ b/lib/ex_unit/test/ex_unit/runner_stats_test.exs @@ -115,6 +115,55 @@ defmodule ExUnit.RunnerStatsTest do end end + describe "parameterized test variants sharing a test id (#15820)" do + @tag :tmp_dir + test "keeps the manifest entry when a failing variant runs before a passing sibling", %{ + tmp_dir: tmp_dir + } do + File.cd!(tmp_dir, fn -> + simulate_suite(fn formatter -> + simulate_test(formatter, :test_1, :failed) + simulate_test(formatter, :test_1, :passed) + end) + + assert read_failures_manifest() == %{{TestModule, :test_1} => __ENV__.file} + end) + end + + @tag :tmp_dir + test "keeps the manifest entry when a failing variant runs after a passing sibling", %{ + tmp_dir: tmp_dir + } do + File.cd!(tmp_dir, fn -> + simulate_suite(fn formatter -> + simulate_test(formatter, :test_1, :passed) + simulate_test(formatter, :test_1, :failed) + end) + + assert read_failures_manifest() == %{{TestModule, :test_1} => __ENV__.file} + end) + end + + @tag :tmp_dir + test "clears the manifest entry once every variant passes in a later run", %{ + tmp_dir: tmp_dir + } do + File.cd!(tmp_dir, fn -> + simulate_suite(fn formatter -> + simulate_test(formatter, :test_1, :failed) + simulate_test(formatter, :test_1, :passed) + end) + + simulate_suite(fn formatter -> + simulate_test(formatter, :test_1, :passed) + simulate_test(formatter, :test_1, :passed) + end) + + assert read_failures_manifest() == %{} + end) + end + end + defp simulate_suite(opts \\ [failures_manifest_path: @failures_manifest_path], fun) do {:ok, pid} = GenServer.start_link(RunnerStats, opts) GenServer.cast(pid, {:suite_started, opts})