fix: Test manifest to respect parameterized tests

lib/ex_unit/lib/ex_unit.ex

@@ -92,7 +92,7 @@ defmodule ExUnit do
           total: non_neg_integer
         }
 
-  @type test_id :: {module, name :: atom}
+  @type test_id :: {module, name :: atom} | {module, name :: atom, parameters :: map}
 
   @typedoc """
   Configuration options for ExUnit.
@@ -370,9 +370,11 @@ defmodule ExUnit do
       [`setup_all/1,2`](`ExUnit.Callbacks.setup_all/1`) callbacks
       are counted as failures. Defaults to `:infinity`;
 
-    * `:only_test_ids` - a list of `{module_name, test_name}` tuples that limits
-      what tests get run. This is typically used by Mix to filter which tests
-      should run;
+    * `:only_test_ids` - a list of `{module_name, test_name}` or
+      `{module_name, test_name, parameters}` tuples that limits what tests run.
+      A `{module_name, test_name}` entry matches every parameterized
+      variant of that test, while `{module_name, test_name, parameters}`
+      matches only the variant with those exact parameters;
 
     * `:rand_algorithm` - algorithm to be used when generating the test seed.
       Available algorithms can be found in Erlang's

lib/ex_unit/lib/ex_unit/failures_manifest.ex

@@ -7,7 +7,7 @@ defmodule ExUnit.FailuresManifest do
 
   @opaque t :: %{optional(ExUnit.test_id()) => test_file :: Path.t()}
 
-  @manifest_vsn 1
+  @manifest_vsn 2
 
   @spec new() :: t
   def new, do: %{}
@@ -18,12 +18,31 @@ defmodule ExUnit.FailuresManifest do
       do: manifest
 
   def put_test(%{} = manifest, %ExUnit.Test{state: nil} = test) do
-    Map.delete(manifest, {test.module, test.name})
+    Map.delete(manifest, test_id(test))
   end
 
   def put_test(%{} = manifest, %ExUnit.Test{state: {failed_state, _}} = test)
       when failed_state in [:failed, :invalid] do
-    Map.put(manifest, {test.module, test.name}, test.tags.file)
+    Map.put(manifest, test_id(test), test.tags.file)
+  end
+
+  @doc """
+  Returns the manifest key for a test.
+
+  Tests without parameters are keyed by `{module, name}`.
+  Parameterized tests are keyed by `{module, name, parameters}` to distinguish the
+  different parameters.
+  """
+  # Keep supporting both variants as `{module, name}` is officially documented in the
+  # `:only_test_ids` option and so may be used by tooling and others, hence we can't
+  # always use the 3-tuple without breaking compatibility.
+  @spec test_id(ExUnit.Test.t()) :: ExUnit.test_id()
+  def test_id(%ExUnit.Test{module: module, name: name, parameters: parameters}) do
+    if parameters == %{} do
+      {module, name}
+    else
+      {module, name, parameters}
+    end
   end
 
   @spec write!(t, Path.t()) :: :ok
@@ -79,7 +98,8 @@ defmodule ExUnit.FailuresManifest do
 
   defp find_deleted_tests([], _file_existence, deleted_tests), do: deleted_tests
 
-  defp find_deleted_tests([{{mod, name} = id, file} | rest] = all, file_existence, acc) do
+  defp find_deleted_tests([{id, file} | rest] = all, file_existence, acc) do
+    {mod, name} = module_and_name(id)
     file_exists = Map.fetch(file_existence, file)
 
     cond do
@@ -104,4 +124,7 @@ defmodule ExUnit.FailuresManifest do
         find_deleted_tests(rest, file_existence, acc)
     end
   end
+
+  defp module_and_name({mod, name}), do: {mod, name}
+  defp module_and_name({mod, name, _parameters}), do: {mod, name}
 end

lib/ex_unit/lib/ex_unit/runner.ex

@@ -243,7 +243,7 @@ defmodule ExUnit.Runner do
 
     # Prepare tests, selecting which ones should be run or skipped
     {to_run_tests, excluded_and_skipped_tests} =
-      prepare_tests(config, async?, group, test_module.tests)
+      prepare_tests(config, async?, group, test_module.tests, params)
 
     for excluded_or_skipped_test <- excluded_and_skipped_tests do
       EM.test_started(config.manager, excluded_or_skipped_test)
@@ -278,14 +278,14 @@ defmodule ExUnit.Runner do
     end
   end
 
-  defp prepare_tests(config, async?, group, tests) do
+  defp prepare_tests(config, async?, group, tests, params) do
     tests = shuffle(config, tests)
     include = config.include
     exclude = config.exclude
     test_ids = config.only_test_ids
 
     {to_run, to_skip} =
-      for test <- tests, include_test?(test_ids, test), reduce: {[], []} do
+      for test <- tests, include_test?(test_ids, test, params), reduce: {[], []} do
         {to_run, to_skip} ->
           tags =
             Map.merge(test.tags, %{
@@ -304,8 +304,9 @@ defmodule ExUnit.Runner do
     {Enum.reverse(to_run), Enum.reverse(to_skip)}
   end
 
-  defp include_test?(test_ids, test) do
-    test_ids == nil or MapSet.member?(test_ids, {test.module, test.name})
+  defp include_test?(test_ids, test, params) do
+    test_ids == nil or MapSet.member?(test_ids, {test.module, test.name}) or
+      MapSet.member?(test_ids, {test.module, test.name, params})
   end
 
   defp run_module_tests(_config, test_module, _async?, []) do

lib/ex_unit/test/ex_unit/failures_manifest_test.exs

@@ -108,6 +108,49 @@ defmodule ExUnit.FailuresManifestTest do
     end
   end
 
+  describe "put_test/2 with parameterized tests" do
+    test "a passing variant does not erase a failing sibling variant from the manifest" do
+      failing = new_test(@failed)
+      failing = %{failing | parameters: %{value: :a}}
+      passing = %{failing | state: @passed, parameters: %{value: :b}}
+
+      manifest =
+        new()
+        |> put_test(failing)
+        |> put_test(passing)
+
+      assert manifest == %{test_id(failing) => file(failing)}
+    end
+
+    test "distinct failing variants both keep their own manifest entry", context do
+      a_variant = %{new_test(@failed, context) | parameters: %{value: :a}}
+      b_variant = %{new_test(@failed, context) | parameters: %{value: :b}}
+
+      manifest =
+        new()
+        |> put_test(a_variant)
+        |> put_test(b_variant)
+
+      assert manifest == %{
+               test_id(a_variant) => file(a_variant),
+               test_id(b_variant) => file(b_variant)
+             }
+    end
+  end
+
+  describe "test_id/1" do
+    test "disambiguates parameterized variants of the same test" do
+      no_params = new_test(@passed)
+      assert test_id(no_params) == {no_params.module, no_params.name}
+
+      with_params = %{no_params | parameters: %{value: :a}}
+      assert test_id(with_params) == {with_params.module, with_params.name, %{value: :a}}
+
+      with_params_b = %{no_params | parameters: %{value: :b}}
+      assert test_id(with_params) != test_id(with_params_b)
+    end
+  end
+
   describe "write!/2" do
     @tag :tmp_dir
     test "stores a manifest that can later be read with read/1", context do
@@ -210,8 +253,6 @@ defmodule ExUnit.FailuresManifestTest do
     }
   end
 
-  defp test_id(test), do: {test.module, test.name}
-
   defp file(test), do: test.tags.file
 
   defp non_blank_manifest(context), do: new() |> put_test(new_test(@failed, context))

lib/mix/test/fixtures/test_failed_parameterize/mix.exs

@@ -0,0 +1,11 @@
+defmodule TestFailedParameterize.MixProject do
+  use Mix.Project
+
+  def project do
+    [
+      app: :test_failed_parameterize,
+      version: "0.0.1",
+      test_load_filters: [~r/.*_test_failed\.exs/]
+    ]
+  end
+end

lib/mix/test/fixtures/test_failed_parameterize/test/parameterized_test_failed.exs

@@ -0,0 +1,8 @@
+defmodule ParameterizedTest do
+  use ExUnit.Case,
+    parameterize: [%{value: :a}, %{value: :b}]
+
+  test "checks value", %{value: value} do
+    assert value == :b
+  end
+end

lib/mix/test/fixtures/test_failed_parameterize/test/test_helper.exs

@@ -0,0 +1 @@
+ExUnit.start()

lib/mix/test/mix/tasks/test_test.exs

@@ -274,6 +274,19 @@ defmodule Mix.Tasks.TestTest do
       System.delete_env("PASS_FAILING_TESTS")
     end
 
+    test "keeps a failing parameterized variant in the manifest even when a passing sibling variant runs after it" do
+      in_fixture("test_failed_parameterize", fn ->
+        # The `:a` variant fails and the `:b` variant passes; `:a` runs first.
+        # Make sure `:a` is still in the manifest and retried. See #15820.
+        output = mix(["test"])
+        assert output =~ "Failed: 1 test"
+
+        output = mix(["test", "--failed"])
+        refute output =~ "There are no tests to run"
+        assert output =~ "Failed: 1 test"
+      end)
+    end
+
     test "marks the whole suite as failed on compilation error" do
       in_fixture("test_failed", fn ->
         File.write!("test/passing_and_failing_test_failed.exs", "raise ~s(oops)")