bug#66782: 29.1; ERT tests report test redefined depending on loading sequence

[Xiyue Deng]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Xiyue Deng <manphiz@gmail.com>
>> Date: Fri, 27 Oct 2023 13:59:07 -0700
>> 
>> As you can see there's only one `first-test' defined.  The error message
>> is misleading.
>> 
>> A real world example of this can be found in lsp-mode, where
>> test/lsp-clangd-test.el[1] requires test/lsp-integration-test.el[2].
>> See also the discussion on an Debian bug[3].
>
> If test2 requires test1, why are both of them explicitly run from the
> command line?  Isn't that redundant, since running test2 will also run
> the tests defined by test1?
>

IIUC most projects just run all tests without explicit dependency
detection.  For example, in lsp-mode it uses eask ert-runner to run all
tests[1].  In Debian it doesn't support cask or eask, so it also has a
custom script that detects all test files and loads them all to run
under ert[2].  In those cases the loading sequence of the test files is
random.  I think few projects actually check for such dependency, which
is not worth the trouble when the test file count is large.

On the other hand, such use is pretty rare if not wrong.  As in the case
of lsp-mode, lsp-clangd-test actually just requires lsp-integration-test
just for a macro, which could have been defined elsewhere in a helper
module and be required from both tests and the problem would be solved.

As an alternative exists, I wonder whether upstream may consider
requiring a file containing ert tests a misuse in ert tests and should
be discouraged, as it may lead to unexpected side effects depending on
loading sequence.

>> However, I'd like to see whether upstream considers this type of usage
>> well-formed and should be supported.  If not, upstream should give a
>> warning on such usage, such as printing a warning when requiring other
>> modules that has `ert-deftest'.  Meanwhile, an improved error message
>> would also be great.
>
> I could agree to improving the error message, but I don't see why we
> should do anything beyond that, FWIW.
>

This would also be better.

> Adding Mattias, who added this check 2 years ago.


[1] https://github.com/emacs-lsp/lsp-mode/blob/master/Makefile#L42
[2] 
https://salsa.debian.org/emacsen-team/dh-elpa/-/blob/master/dh_elpa_test#L386-396
-- 
Xiyue Deng