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

[Xiyue Deng <manphiz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]

Xiyue Deng <manphiz@gmail.com> writes:

> Mattias Engdegård <mattias.engdegard@gmail.com> writes:
>
>> 29 okt. 2023 kl. 22.53 skrev Xiyue Deng <manphiz@gmail.com>:
>>
>>> As far as I know, there is no practical use for a test case to depend on
>>> another test case in any language I worked with as a test case provides
>>> no reusable component.  On the other hand, there are alternatives that
>>> makes more sense to do what they (e.g. lsp-mode) are doing.  Again, IMHO
>>> I think it makes sense for ERT not to support it or to even forbid it.
>>> Doing this also avoids such misleading error message I was reporting,
>>> which got triggered depending on the loading sequence which may make
>>> test suites flaky (as the loading sequence is not deterministic).
>>
>> I don't think ERT can forbid anyone from loading in the same .el file multiple times because it's none of its business. Or did you have something else in mind?
>>
>
> It's not that loading the same .el multiple times that caused this
> error, but loading a module with `ert-deftest' and both .el modules are
> passed to batch ert processing which is common practice.  But see below.
>
>> All in all, I'm not sure there is anything that we can do in Emacs right now that would help, but if you have any concrete suggestions, do tell.
>>
>
> I understand if upstream don't want to complicate `require' logic too
> much.  However I wonder whether it's OK to add warning if a required
> module has `ert-deftest' in it, so that it can help people identify that
> a `Test "foo" redefined' error is due to requiring other module instead
> of an actual duplicated test name.  How does this sound?

As I didn't get an answer I assume this was a no-go.  So instead I'd
like to propose a slight change to the error message to mention that it
may also be caused by an ert test being loaded multiple times.  Patch is
attached, please let me know whether this works.

-- 
Xiyue Deng

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Revise-test-redefined-error-message.patch --]
[-- Type: text/x-diff, Size: 1000 bytes --]

From 4efc6a84c4bb0758d67e2721fcb479494a312d69 Mon Sep 17 00:00:00 2001
From: Xiyue Deng <manphiz@gmail.com>
Date: Thu, 2 Nov 2023 10:08:48 -0700
Subject: [PATCH] Revise test redefined error message

Add the possibility that an ert test that is loaded multiple times may
trigger the same error (Bug#66782).
* lisp/emacs-lisp/ert.el (ert-set-test): Update error message.
---
 lisp/emacs-lisp/ert.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index d727bc94ec5..4d189ed614e 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -152,7 +152,7 @@ mode.")
   (when (and noninteractive (get symbol 'ert--test))
     ;; Make sure duplicated tests are discovered since the older test would
     ;; be ignored silently otherwise.
-    (error "Test `%s' redefined" symbol))
+    (error "Test `%s' redefined or loaded multiple times" symbol))
   (define-symbol-prop symbol 'ert--test definition)
   definition)
 
-- 
2.39.2