From d96d71336855b7a1969e9b691ca14b0d94e94182 Mon Sep 17 00:00:00 2001 From: Tobias Pfeiffer Date: Tue, 1 Sep 2026 09:05:55 +0200 Subject: [PATCH] fix: Test manifest to respect parameterized tests Fixes #15820 In short, parameterized tests weren't accounted for in the test manifest, so if a variant first failed it could get overridden by a later run with different parameters as the key was just `{module, test}`. So, this PR introduces the `{module, test, parameters}` variant, to account for it. The old `{module, test}` is still supported instead of making it `{module, test, %{}}` as it is part of the official documentation (`:only_test_ids`) and so we don't want to break compatibility. ## Decisions Some may be worth revisiting: * bumped the manifest version as we changed it * decided to support both `{module, test}` and `{module, test, parameters}` (see above) but we could also transform it at the boundary (i.e. transform `{module, test}` input to `{module, test, %{}}`) and then have a more uniform manifest (and less branching code paths) ## Potential Problems * I believe right now if a parameter gets deleted from a test, a corresponding failure may not be pruned from the manifest as we don't get that data (it's not part of the loaded test module), so then `mix test --failed` would run but not find any test. Not sure how to best fix that. --- lib/ex_unit/lib/ex_unit.ex | 10 +++-- lib/ex_unit/lib/ex_unit/failures_manifest.ex | 31 +++++++++++-- lib/ex_unit/lib/ex_unit/runner.ex | 11 ++--- .../test/ex_unit/failures_manifest_test.exs | 45 ++++++++++++++++++- .../fixtures/test_failed_parameterize/mix.exs | 11 +++++ .../test/parameterized_test_failed.exs | 8 ++++ .../test/test_helper.exs | 1 + lib/mix/test/mix/tasks/test_test.exs | 13 ++++++ 8 files changed, 115 insertions(+), 15 deletions(-) create mode 100644 lib/mix/test/fixtures/test_failed_parameterize/mix.exs create mode 100644 lib/mix/test/fixtures/test_failed_parameterize/test/parameterized_test_failed.exs create mode 100644 lib/mix/test/fixtures/test_failed_parameterize/test/test_helper.exs diff --git a/lib/ex_unit/lib/ex_unit.ex b/lib/ex_unit/lib/ex_unit.ex index cd3ba58d184..b408a4b9b4c 100644 --- a/lib/ex_unit/lib/ex_unit.ex +++ b/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 diff --git a/lib/ex_unit/lib/ex_unit/failures_manifest.ex b/lib/ex_unit/lib/ex_unit/failures_manifest.ex index 239b63201fd..5cefb67e36e 100644 --- a/lib/ex_unit/lib/ex_unit/failures_manifest.ex +++ b/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 diff --git a/lib/ex_unit/lib/ex_unit/runner.ex b/lib/ex_unit/lib/ex_unit/runner.ex index e8f2fcf23a3..b17a2744807 100644 --- a/lib/ex_unit/lib/ex_unit/runner.ex +++ b/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 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 219f723d36a..1e49cd3a631 100644 --- a/lib/ex_unit/test/ex_unit/failures_manifest_test.exs +++ b/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)) diff --git a/lib/mix/test/fixtures/test_failed_parameterize/mix.exs b/lib/mix/test/fixtures/test_failed_parameterize/mix.exs new file mode 100644 index 00000000000..18e0e8e1ab4 --- /dev/null +++ b/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 diff --git a/lib/mix/test/fixtures/test_failed_parameterize/test/parameterized_test_failed.exs b/lib/mix/test/fixtures/test_failed_parameterize/test/parameterized_test_failed.exs new file mode 100644 index 00000000000..66bda88c3a6 --- /dev/null +++ b/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 diff --git a/lib/mix/test/fixtures/test_failed_parameterize/test/test_helper.exs b/lib/mix/test/fixtures/test_failed_parameterize/test/test_helper.exs new file mode 100644 index 00000000000..869559e709e --- /dev/null +++ b/lib/mix/test/fixtures/test_failed_parameterize/test/test_helper.exs @@ -0,0 +1 @@ +ExUnit.start() diff --git a/lib/mix/test/mix/tasks/test_test.exs b/lib/mix/test/mix/tasks/test_test.exs index 29bebd7f145..03e3adef677 100644 --- a/lib/mix/test/mix/tasks/test_test.exs +++ b/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)")