Skip to content

Conversation

@SteffenDE
Copy link
Contributor

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?

This commit changes the default warn_test_pattern to "*.{ex,exs}", which matches bascially all Elixir files and restricts them with a separate warn_test_ignore_pattern that can be a more complex regular expression, defaulting to .*_helper.exs, which should be a good default. It also ignores any compiled files, for example test support files, by also looking at the configured elixirc_paths.

For projects with an existing warn_test_pattern configuration that want to keep the same behavior, the warn_test_ignore_pattern can be set to a falsy value, although it would only make a difference if they had any *_helper.exs they do not want to have ignored, which should be quite rare.

elixirc_paths
) do
# any test support files should be ignored
compiled_files = Mix.Utils.extract_files(elixirc_paths, [:ex])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am worried about this additional extraction because in large projects this will return tens of thousand of files, which will add one second or more on each mix test invocation.

Even today's solution, of doing one additional traversal on warn_test_pattern is non-optimal. Ideally, we should probably do this:

  1. Have one pattern for finding both unwanted and wanted test files, looking up the file system once
  2. Then have a string or a regex that finds the ones we want to warn on

Then by default we could warn on all .ex files that end with _test.ex or any .exs that does not match _helper.exs Do you think we could adapt this new approach with what you want to achieve?

Another option is to only do this check if a flag is given, which folks could enable on CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was worried about this too. One simple solution here would be to just perform a String.starts_with check with the elixirc_paths, which won't be 100% correct, but probably fine. If we want to only do extract_files once, I think we can only make it work if we are fine with breaking changes (e.g. not supporting glob path patterns in the config).

@josevalim
Copy link
Member

josevalim commented Dec 7, 2024 via email

… 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?

This commit changes the default `warn_test_pattern` to "*.{ex,exs}",
which matches bascially all Elixir files and restricts them with a separate
`warn_test_ignore_pattern` that can be a more complex regular expression,
defaulting to `.*_helper.exs`, which should be a good default. It also
ignores any compiled files, for example test support files, by also
looking at the configured `elixirc_paths`.

For projects with an existing `warn_test_pattern` configuration that
want to keep the same behavior, the `warn_test_ignore_pattern` can be set
to a falsy value, although it would only make a difference if they had
any `*_helper.exs` they do not want to have ignored, which should be
quite rare.
@SteffenDE
Copy link
Contributor Author

I adjusted this PR to do the "cheap" elixirc_paths check and sent #14036 with an alternative implementation (with breaking changes).

@josevalim
Copy link
Member

Closing in favor of the other PR.

@josevalim josevalim closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants