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

[Xiyue Deng]

Hi Mattias,

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> 2 nov. 2023 kl. 18.17 skrev Xiyue Deng <manphiz@gmail.com>:
>
>>> 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.
>
> No, please don't make such an assumption -- I was just busy elsewhere and 
> hadn't given your message the attention it deserved. Sorry about that.
>

Sorry if I sounded pushy which I didn't intend to, ...

> That said, in this case I'm not sure how to implement your suggestion in a 
> clean
> way and if all that effort is really worth the trouble, so perhaps the answer
> would be the same anyway. And we probably don't want to prohibit `ert-deftest`
> in required modules in general for reasons mentioned -- they could be used 
> with
> perfectly fine discipline elsewhere.
>

And glad we are in agreement here, as I realizing adding this extra
checking to require may add some unnecessary complexity.

>>  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.
>
> I wouldn't mind such a change if it really would help. Would it? Isn't it 
> just restating the problem in other words?
>

Let me clarify my intent, which is trying to do is to distinguish the
two possible scenarios that cause this error:

1) Tests that are different but use the same test name.

2) There is no tests sharing the same name, but caused by double loading
the same test unit through a dependency by require.

Case 1 happens a lot in the wild and has caused many FTBFS bugs in
Debian after upgrading Emacs to 29.1 (e.g. [1][2]), and the fix is
simply to rename the tests.

Case 2 is kinda tricky as there are no tests actually sharing a name
here.  See also in [3], which had many test with same name as in case 1
but for the specific error on `lsp-text-document-hover-request' it's
actually case 2.  As a matter of fact I've spent a non-trivial time
trying to debug this one as it depends on the loading sequence which
caused the failure to be flaky.  So I hope my proposed change can help
people on realizing that it's case 2 a bit faster.

>

[1] https://bugs.debian.org/1052865
[2] https://bugs.debian.org/1052922
[3] https://bugs.debian.org/1052939

-- 
Xiyue Deng