Postmortem: Template unittests are bad & you shouldn't catch Error

[Mathias LANG]

On Thursday, 22 October 2020 at 14:36:11 UTC, Steven 
Schveighoffer wrote:
> On 10/22/20 12:04 AM, Mathias LANG wrote:
>> 
>> First, why on god's green earth are we putting unittests with 
>> explicit template instantiation inside a *template* ? 
>> `unittest` inside templates are a terrible "feature", and I've 
>> never seen a correct usage of it, which would be a unittest 
>> that relies on the user-instantiated instance to test that its 
>> expectation still holds.
>
> I used it in dcollections quite a bit as a way to ensure all 
> integer based types were tested.
>
> The advantage here is, I wanted to run the same tests on every 
> integer type. But I wanted the tests to be close to the 
> functions they were testing.
>
> But I think this can be done a different way, and it is 
> *really* hard to get this right. Plus, they are going to be run 
> when you instantiate them elsewhere, which is a problem.

The point was, if the unittest does not depend on the template 
parameters, it shouldn't be in the template.
So the following is a correct use of the feature:
```
struct Nullable (T)
{
     unittest
     {
         Nullable inst;
         assert(inst.isNull());
     }
}
```
While this is not:
```
struct Nullable (T)
{
     unittest
     {
         Nullable!int inst;
         assert(inst.isNull());
     }
}
```

Because the first one will run for the user instantiation while 
the second one is independent on the user instantiation (but will 
run anyway).

>> 
>> Second, as the title of the post says, *do not catch Error*. 
>> Catching `Error` is a sin, a crime, and every catch of `Error` 
>> derivative should have you outraged. It is in direct violation 
>> of the specs. And obviously, it's also incompatible with 
>> scheme that do not throw an `Error`.
>
> I disagree with this. How would you test that the assert error 
> is thrown? It seems that your specific case is being hampered 
> by this, but can't you just look at the parameters to see 
> whether you should handle it or not? Like check the file/line 
> and do the normal thing if it's from phobos?

You do not test for assert error. Assert errors are logic errors, 
and if they get triggered, all bets are off. Of course, one might 
find this impractical, because `get` cannot be tested this way. 
Perhaps that's a sign that `Nullable` should use exceptions, and 
not `assert`, for this.

Regarding the workaround: We currently do this, however, testing 
for it in a cross-platform and generic way is non-trivial. We can 
do `endsWith` but we have to account for different paths. And 
there's no guarantee that we won't have false-positive (e.g. 
ourcustomlib.std.typecons).

>> In any case, there's two things I really wish we fixed:
>> - Either ban unittests in templates, deprecate them, or find a 
>> way to make this mistake hard to do;
>
> What we need is a way to make a static unittest that serves to 
> provide documentation, but is run as if it were outside the 
> template.

That's something that Andrej Mitrovic brought up during the 
following discussion. While it would fix this specific instance, 
the default would still be bad, so I'm not really convinced.

>> - Deprecate the ability to catch `Error` in code. If it's not 
>> supposed to be caught, it shouldn't be so easy to do it;
>
> Disagree. This use case you have is very obscure, and it's much 
> less obscure to verify an Error is thrown in a unittest.
>
> -Steve

You disagree with the spec then. The spec says that the stack 
might not be unwound if an `Error` is thrown. Walter even raised 
a PR to actually do the optimization the spec promises. In his 
words:

> This PR is still the right thing to do. Attempting to unwind 
> when an Error is thrown is a bad idea, and leads to undefined 
> behavior. Essentially, code which relies on such unwinding 
> needs to be fixed.

Source: 
https://github.com/dlang/dmd/pull/6896#issuecomment-362855793