fix: Test Manifest, parameterized do not override sibling failures

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
 

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
 

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,18 +177,18 @@ 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
     test "returns a blank manifest when the file is corrupted", context 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

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})