From 5b56d008adb8fa9b3fc2445796dc07f23dd47d3f Mon Sep 17 00:00:00 2001 From: Steffen Deusch Date: Thu, 5 Dec 2024 18:33:37 +0100 Subject: [PATCH] (mix test) add test_load_pattern and test_warn_pattern When using the default project configuration, mix test would only warn about files ending in `_test.ex`. The only mistake this warns about is forgetting the `s` of `.exs`. In a work project we noticed (after more than a year) that we had multiple test files that did not match the test pattern, preventing them from running in mix test locally and CI. Because we have many other tests, nobody noticed this. If CI passes, all is good, right? Because there is no easy way to evaluate glob patterns in Elixir without touching the filesystem, I decided to deprecate the old configurations and instead add two new configurations: `test_load_pattern` and `test_warn_pattern`. Now, we can load all potential test files once and then match their paths to the patterns. The `test_load_pattern` is used to filter the files that are loaded by mix test. This defaults to the regex equivalent of "*_test.exs". The `test_warn_pattern` is used to filter the files that we warned about if they are not loaded. By default, we warn about any file that either ends in `_test.ex` (the old behavior) but also any file that ends in `.exs`, but does not end in `_helper.exs`, which will prevent the default test_helper.exs from generating a warning and also provides a way to name other files that might be required explicitly in tests. As an escape hatch the `test_warn_ignore_files` list can be used to ignore specific files where the warning should not be generated. For projects with an existing `test_pattern` or `warn_test_pattern` configuration, a deprecation warning is logged. The new warnings can be disabled by setting `test_warn_pattern` to a falsy value. --- lib/mix/lib/mix/tasks/test.ex | 81 ++++++++++++++++--- lib/mix/test/fixtures/test_failed/mix.exs | 2 +- lib/mix/test/fixtures/test_stale/mix.exs | 2 +- lib/mix/test/fixtures/test_warn/mix.exs | 12 +++ .../fixtures/test_warn/test/a_missing.exs | 0 .../test/fixtures/test_warn/test/a_tests.ex | 0 .../test/fixtures/test_warn/test/a_tests.exs | 7 ++ .../fixtures/test_warn/test/other_file.txt | 0 .../fixtures/test_warn/test/test_helper.exs | 1 + .../fixtures/umbrella_test/apps/bar/mix.exs | 2 +- .../fixtures/umbrella_test/apps/foo/mix.exs | 2 +- lib/mix/test/mix/tasks/test_test.exs | 21 +++++ 12 files changed, 113 insertions(+), 17 deletions(-) create mode 100644 lib/mix/test/fixtures/test_warn/mix.exs create mode 100644 lib/mix/test/fixtures/test_warn/test/a_missing.exs create mode 100644 lib/mix/test/fixtures/test_warn/test/a_tests.ex create mode 100644 lib/mix/test/fixtures/test_warn/test/a_tests.exs create mode 100644 lib/mix/test/fixtures/test_warn/test/other_file.txt create mode 100644 lib/mix/test/fixtures/test_warn/test/test_helper.exs diff --git a/lib/mix/lib/mix/tasks/test.ex b/lib/mix/lib/mix/tasks/test.ex index ec3a0b60c5f..05e72d82204 100644 --- a/lib/mix/lib/mix/tasks/test.ex +++ b/lib/mix/lib/mix/tasks/test.ex @@ -228,10 +228,18 @@ defmodule Mix.Tasks.Test do `["test"]` if the `test` directory exists, otherwise, it defaults to `[]`. It is expected that all test paths contain a `test_helper.exs` file - * `:test_pattern` - a pattern to load test files. Defaults to `*_test.exs` + * `:test_load_pattern` - a regular expression to load test files. + Defaults to `~r/.*_test\\.exs$/` - * `:warn_test_pattern` - a pattern to match potentially misnamed test files - and display a warning. Defaults to `*_test.ex` + * `:test_warn_pattern` - a regular expression to match potentially + misnamed test files and display a warning. + Defaults to `~r/^(?!.*_helper\.exs$)(?:.*_test\.ex|.*\.exs)$/`, + matching any file that ends in `_test.ex` or `.exs`, ignoring files + ending in `_helper.exs`. Warnings can be disabled by setting this to + `nil` or `false`. + + * `:test_warn_ignore_files` - a list of files to ignore when displaying + warnings about potentially misnamed test files. Defaults to `[]`. ## Coloring @@ -595,20 +603,49 @@ defmodule Mix.Tasks.Test do # Prepare and extract all files to require and run test_paths = project[:test_paths] || default_test_paths() - test_pattern = project[:test_pattern] || "*_test.exs" - warn_test_pattern = project[:warn_test_pattern] || "*_test.ex" + test_load_pattern = project[:test_load_pattern] || ~r/.*_test\.exs$/ + + test_warn_pattern = + Keyword.get(project, :test_warn_pattern, ~r/^(?!.*_helper\.exs$)(?:.*_test\.ex|.*\.exs)$/) + + test_warn_ignore_files = project[:test_warn_ignore_files] || [] + + # Warn about deprecated configurations + if project[:test_pattern] do + Mix.shell().info( + "warning: the `:test_pattern` configuration is deprecated and will be ignored. Use `:test_load_pattern` instead." + ) + end + + if project[:warn_test_pattern] do + Mix.shell().info( + "warning: the `:warn_test_pattern` configuration is deprecated and will be ignored. Use `:test_warn_pattern` instead." + ) + end {test_files, test_opts} = if files != [], do: ExUnit.Filters.parse_paths(files), else: {test_paths, []} - unfiltered_test_files = Mix.Utils.extract_files(test_files, test_pattern) + # get a list of all files in the test folders, which we filter by the test_load_pattern + potential_test_files = Mix.Utils.extract_files(test_files, "**") + + unfiltered_test_files = + Enum.filter(potential_test_files, &Regex.match?(test_load_pattern, &1)) matched_test_files = unfiltered_test_files |> filter_to_allowed_files(allowed_files) |> filter_by_partition(shell, partitions) - display_warn_test_pattern(test_files, test_pattern, unfiltered_test_files, warn_test_pattern) + # do not warn if test_warn_pattern is nil or false + test_warn_pattern && + maybe_warn_misnamed_test_files( + potential_test_files, + unfiltered_test_files, + test_warn_ignore_files, + test_load_pattern, + test_warn_pattern + ) try do Enum.each(test_paths, &require_test_helper(shell, &1)) @@ -679,13 +716,31 @@ defmodule Mix.Tasks.Test do end end - defp display_warn_test_pattern(test_files, test_pattern, matched_test_files, warn_test_pattern) do - files = Mix.Utils.extract_files(test_files, warn_test_pattern) -- matched_test_files + defp maybe_warn_misnamed_test_files( + potential_test_files, + unfiltered_test_files, + test_warn_ignore_files, + test_load_pattern, + test_warn_pattern + ) + when is_struct(test_warn_pattern, Regex) do + missing_files = (potential_test_files -- unfiltered_test_files) -- test_warn_ignore_files - for file <- files do - Mix.shell().info( - "warning: #{file} does not match #{inspect(test_pattern)} and won't be loaded" - ) + ignored = Enum.filter(missing_files, &Regex.match?(test_warn_pattern, &1)) + + if ignored != [] do + Mix.shell().info(""" + warning: the following files do not match the test load pattern #{inspect(test_load_pattern)} and won't be loaded: + + #{Enum.join(ignored, "\n")} + + This might indicate a typo in a test file name (for example, using "foo_tests.exs" instead of "foo_test.exs"). + + You can adjust which files trigger this warning by configuring the `:test_warn_pattern` option in your + Mix project's configuration. To disable the warning entirely, set that option to false. + + For more information, run `mix help test`. + """) end end diff --git a/lib/mix/test/fixtures/test_failed/mix.exs b/lib/mix/test/fixtures/test_failed/mix.exs index 26e152b8d86..f5fcc8d9041 100644 --- a/lib/mix/test/fixtures/test_failed/mix.exs +++ b/lib/mix/test/fixtures/test_failed/mix.exs @@ -5,7 +5,7 @@ defmodule TestFailed.MixProject do [ app: :test_only_failures, version: "0.0.1", - test_pattern: "*_test_failed.exs" + test_load_pattern: ~r/,*_test_failed\.exs/ ] end end diff --git a/lib/mix/test/fixtures/test_stale/mix.exs b/lib/mix/test/fixtures/test_stale/mix.exs index 2300c8b58fe..057b6a82c0a 100644 --- a/lib/mix/test/fixtures/test_stale/mix.exs +++ b/lib/mix/test/fixtures/test_stale/mix.exs @@ -5,7 +5,7 @@ defmodule TestStale.MixProject do [ app: :test_stale, version: "0.0.1", - test_pattern: "*_test_stale.exs" + test_load_pattern: ~r/.*_test_stale\.exs/ ] end end diff --git a/lib/mix/test/fixtures/test_warn/mix.exs b/lib/mix/test/fixtures/test_warn/mix.exs new file mode 100644 index 00000000000..c0730f2c695 --- /dev/null +++ b/lib/mix/test/fixtures/test_warn/mix.exs @@ -0,0 +1,12 @@ +defmodule TestWarn.MixProject do + use Mix.Project + + def project do + [ + app: :test_warn, + version: "0.0.1", + test_load_pattern: ~r/.*_tests\.exs/, + test_warn_pattern: ~r/^(?!.*_helper\.exs$)(?:.*_tests\.ex|.*\.exs)$/ + ] + end +end diff --git a/lib/mix/test/fixtures/test_warn/test/a_missing.exs b/lib/mix/test/fixtures/test_warn/test/a_missing.exs new file mode 100644 index 00000000000..e69de29bb2d diff --git a/lib/mix/test/fixtures/test_warn/test/a_tests.ex b/lib/mix/test/fixtures/test_warn/test/a_tests.ex new file mode 100644 index 00000000000..e69de29bb2d diff --git a/lib/mix/test/fixtures/test_warn/test/a_tests.exs b/lib/mix/test/fixtures/test_warn/test/a_tests.exs new file mode 100644 index 00000000000..06ebaca48b6 --- /dev/null +++ b/lib/mix/test/fixtures/test_warn/test/a_tests.exs @@ -0,0 +1,7 @@ +defmodule ATests do + use ExUnit.Case + + test "dummy" do + assert true + end +end diff --git a/lib/mix/test/fixtures/test_warn/test/other_file.txt b/lib/mix/test/fixtures/test_warn/test/other_file.txt new file mode 100644 index 00000000000..e69de29bb2d diff --git a/lib/mix/test/fixtures/test_warn/test/test_helper.exs b/lib/mix/test/fixtures/test_warn/test/test_helper.exs new file mode 100644 index 00000000000..869559e709e --- /dev/null +++ b/lib/mix/test/fixtures/test_warn/test/test_helper.exs @@ -0,0 +1 @@ +ExUnit.start() diff --git a/lib/mix/test/fixtures/umbrella_test/apps/bar/mix.exs b/lib/mix/test/fixtures/umbrella_test/apps/bar/mix.exs index f12eed69de9..bfa7e4a222d 100644 --- a/lib/mix/test/fixtures/umbrella_test/apps/bar/mix.exs +++ b/lib/mix/test/fixtures/umbrella_test/apps/bar/mix.exs @@ -7,7 +7,7 @@ defmodule Bar.MixProject do version: "0.1.0", # Choose something besides *_test.exs so that these test files don't # get accidentally swept up into the actual Mix test suite. - test_pattern: "*_tests.exs", + test_load_pattern: ~r/.*_tests\.exs/, test_coverage: [ignore_modules: [Bar, ~r/Ignore/]], aliases: [mytask: fn _ -> Mix.shell().info("bar_running") end] ] diff --git a/lib/mix/test/fixtures/umbrella_test/apps/foo/mix.exs b/lib/mix/test/fixtures/umbrella_test/apps/foo/mix.exs index 4c4e8358212..33fbcc29e15 100644 --- a/lib/mix/test/fixtures/umbrella_test/apps/foo/mix.exs +++ b/lib/mix/test/fixtures/umbrella_test/apps/foo/mix.exs @@ -7,7 +7,7 @@ defmodule Foo.MixProject do version: "0.1.0", # Choose something besides *_test.exs so that these test files don't # get accidentally swept up into the actual Mix test suite. - test_pattern: "*_tests.exs", + test_load_pattern: ~r/.*_tests\.exs/, aliases: [mytask: fn _ -> Mix.shell().info("foo_running") end] ] end diff --git a/lib/mix/test/mix/tasks/test_test.exs b/lib/mix/test/mix/tasks/test_test.exs index b51ed680db1..58972cce8b5 100644 --- a/lib/mix/test/mix/tasks/test_test.exs +++ b/lib/mix/test/mix/tasks/test_test.exs @@ -624,6 +624,27 @@ defmodule Mix.Tasks.TestTest do end end + describe "test_load_pattern and test_warn_pattern" do + test "warns for files that are not loaded and match test_warn_pattern" do + in_fixture("test_warn", fn -> + output = mix(["test"]) + + # we don't warn about test_helper.exs or a_tests.exs (<- matches the load pattern) + assert output =~ """ + the following files do not match the test load pattern ~r/.*_tests\\.exs/ and won't be loaded: + + test/a_missing.exs + test/a_tests.ex + + This might indicate a typo\ + """ + + # the dummy test ran successfully + assert output =~ "1 test, 0 failures" + end) + end + end + defp receive_until_match(port, expected, acc) do receive do {^port, {:data, output}} ->