tests in separate test modules are performed twice by eunit command

[fififionek]

Environment

Rebar3 report
 version 3.22.1
 generated at 2024-04-05T13:52:26+00:00
=================
Please submit this along with your issue at https://github.com/erlang/rebar3/issues (and feel free to edit out private information, if any)
-----------------
Task: eunit
Entered as:
  eunit
-----------------
Operating System: x86_64-pc-linux-gnu
ERTS: Erlang/OTP 26 [erts-14.1] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit:ns]
Root Directory: /home/ ... /Erlang/runtime/26.1/lib/erlang
Library directory: /home/ ... /Erlang/runtime/26.1/lib/erlang/lib
-----------------
Loaded Applications:
bbmustache: 1.12.2
certifi: 2.11.0
cf: 0.3.1
common_test: 1.25.1
compiler: 8.4
crypto: 5.3
cth_readable: 1.5.1
dialyzer: 5.1.1
edoc: 1.2
erlware_commons: 1.6.0
eunit: 2.8.2
eunit_formatters: 0.5.0
getopt: 1.0.2
inets: 9.0.2
kernel: 9.1
providers: 1.9.0
public_key: 1.14.1
relx: 4.8.0
sasl: 4.2.1
snmp: 5.15
ssl_verify_fun: 1.1.6
stdlib: 5.1
syntax_tools: 3.1
tools: 3.6

-----------------
Escript path: /home/ ... /Erlang/runtime/26.1/bin/rebar3
Providers:
  app_discovery as clean compile compile cover ct deps dialyzer do edoc escriptize eunit get-deps help install install_deps list lock new path pkgs release relup report repos shell state tar tree unlock update upgrade upgrade upgrade vendor version xref 

---

Project I discovered this in by chance is fairly big, but I managed to replicate it with this simple application:

rebar.config

{erl_opts, [debug_info]}.
{deps, []}.

src/p.app.src

{application, p, [
  {description, "An OTP library"},
  {vsn, "0.1.0"},
  {applications, [kernel, stdlib]},
  {modules, [p]}
]}.

src/p.erl

-module(p).

-include_lib("eunit/include/eunit.hrl").

-export([p/1]).

p(true) -> false;
p(false) -> true;
p(_)->error(badarg).

-ifdef(EUNIT).

tests_test_() -> [
        {"annotation", [
                ?_test(?debugHere)
        ]},
        {"actual tests", [
                ?_assertEqual(true, p:p(false)),
                ?_assertError(badarg, p:p(undefined))
        ]}
].

-endif.

src/p_tests.erl

-module(p_tests).

-include_lib("eunit/include/eunit.hrl").

-ifdef(EUNIT).

tests_test_() -> [
        {"annotation", [
                ?_test(?debugHere)
        ]},
        {"actual tests", [
                ?_assertEqual(false, p:p(true))
        ]}
].

-endif.

Current behaviour

All eunit tests located in separate modules (i.e. the modules with _tests suffix) are run twice.
Here is result of running reabr3 eunit on the example application:

===> Verifying dependencies...
===> Analyzing applications...
===> Compiling p
===> Performing EUnit tests...
/home/andy/Empyreum/svn/Erlang/releases/q/src/p.erl:15:<0.247.0>: <-
../home/andy/Empyreum/svn/Erlang/releases/q/src/p_tests.erl:9:<0.247.0>: <-
../home/andy/Empyreum/svn/Erlang/releases/q/src/p_tests.erl:9:<0.247.0>: <-
...
Finished in 0.028 seconds
7 tests, 0 failures

Expected behaviour

Obviously, all the tests should run only once.

[ferd]

If you move your p_tests module to an extra directory like test/ they will run just once.

The issue comes from a weird interaction between how we track both source and test directories differently, and how deduping is currently done only on each set individually:

rebar3/apps/rebar/src/rebar_prv_eunit.erl

Lines 216 to 228 in 7ff5618

	set_modules([], State, {AppAcc, TestAcc}) ->
	Regex = rebar_state:get(State, eunit_test_regex, ?DEFAULT_TEST_REGEX),
	BareTestDir = [filename:join([rebar_state:dir(State), "test"])],
	TestSrc = gather_src(BareTestDir, Regex),
	dedupe_tests({AppAcc, TestAcc ++ TestSrc});
	set_modules([App|Rest], State, {AppAcc, TestAcc}) ->
	F = fun(Dir) -> filename:join([rebar_app_info:dir(App), Dir]) end,
	AppDirs = lists:map(F, rebar_dir:src_dirs(rebar_app_info:opts(App))),
	Regex = rebar_state:get(State, eunit_test_regex, ?DEFAULT_TEST_REGEX),
	AppSrc = gather_src(AppDirs, Regex),
	TestDirs = [filename:join([rebar_app_info:dir(App), "test"])],
	TestSrc = gather_src(TestDirs, Regex),
	set_modules(Rest, State, {AppSrc ++ AppAcc, TestSrc ++ TestAcc}).

The deduplication is explicitly set up to remove the dupes between both directories, but not within any of them:

rebar3/apps/rebar/src/rebar_prv_eunit.erl

Lines 236 to 248 in 7ff5618

	dedupe_tests({AppMods, TestMods}) ->
	UniqueTestMods = lists:usort(TestMods) -- AppMods,
	%% for each modules in TestMods create a test if there is not a module
	%% in AppMods that will trigger it
	F = fun(TestMod) ->
	M = filename:rootname(filename:basename(TestMod)),
	MatchesTest = fun(AppMod) -> filename:rootname(filename:basename(AppMod)) ++ "_tests" == M end,
	case lists:any(MatchesTest, AppMods) of
	false -> {true, {module, list_to_atom(M)}};
	true -> false
	end
	end,
	rebar_utils:filtermap(F, UniqueTestMods).

Rebar3 was built with the opinionated approach/expectation that a test-specific module should be in a test-specific directory, but still tries to support the test being co-located within the source file.

We could argue that the de-duplication should go even deeper there, but it isn't clear, in a world where both src/ and test/ may exist, whether mod_test should be seen as the test module for mod or whether they should be perceived as independent (and if so, what happens with mod_test_test?)

[fififionek]

Thank you for the workaround pointer. I didn't realize Erlang has standard directory for test modules. I will use this directory from now on. I don't think test co-location within the module can ever go away since it is necessary when testing internal functions.

I think, that for the purpose of running tests, it does not matter, if we consider the tests to test this or that module. What matters is that all relevant tests should be run and preferably only once since tests can be very time consuming. The result of a test does not change depending on our perception of its belonging.

What tests should be run is dictated by EUnit. To test module mod, tests in modules mod and mod_tests should be run. To test module mod_tests tests in modules mod_tests and mod_tests_tests should be run. Until the specification of EUnit is changed, people will write tests by these rules.

[ferd]

Eunit allows extensive configuration of which tests to run.

You may get the default behavior by specifying tests with

{eunit_tests, [{application, p}]}.

Which will ignore test directories and just run everything it can infer from the source files.

[fififionek]

I only just now realized rebar3 overrides which modules are part of the application and uses the src and test directory distinction to decide which modules to include. Which means it also has to decide which modules it is supposed to preform tests from for the application.

Since rebar3 already manages the module lists, maybe it could use {exact_execution, true} option when calling eunit:test/2. That would prevent triggering of <module>_tests module tests along with <module> module tests. This would remove the need to deduplicate at all. It would also prevent the double triggering when the tests module is part of the application (i.e. in the src directory).

[fififionek]

Ok, I am sorry, that would work for applications, but not for modules, which would require duplication of EUnit functionality. I am just trying to wrap my head around the fact, that my management of application modules is ignored and irrelevant...

[ferd]

I don't know, I barely use eunit anymore. The current approach supported by Rebar3 has been pretty much unchanged since 2015 when they were first supported, with the current filtering set done in 2016. Everybody else who has used EUnit with Rebar3 for the last 8 years has had mostly unchanged behavior.

I don't think there would be harm in figuring out better ways to de-duplicate them to better handle the edge case you have here, but I'd be unlikely to consider it a good idea to majorly change how tests are scheduled.