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

[Steven Schveighoffer]

On 10/22/20 12:05 PM, Mathias LANG wrote:
> 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).

Yes, I know. It's exactly what I did:

https://github.com/schveiguy/dcollections/blob/master/dcollections/TreeMap.d#L123

https://github.com/schveiguy/dcollections/blob/master/dcollections/TreeMap.d#L253

> 
>>>
>>> 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.

You did not answer the question though, the unittest is making sure that 
an assert error is thrown when you attempt to get a null value. How else 
do you test for this?

Nullable using exceptions means it can't be used in nothrow code. This 
argument has been had countless times for many different discussions.

I thought AssertErrors during unit tests are allowed to be caught by the 
unit test framework? Isn't assert the main tool to test things? If you 
aren't supposed to catch them ever, how does unittesting work?

> 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).

Isn't this for finding a bug though? I didn't think you were going to 
enable the custom assert handler for production. So who cares if it's 
not neat and clean, get it to work, find the bug, and move on.

>>> 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.

It needs to happen. There's no reason I shouldn't be able to use 
documented unittests in templates.

Another thing I've advocated for strongly is to not run imported 
unittests ever, even in templates.

> 
>>> - 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.
>>
> 
> You disagree with the spec then. The spec says that the stack might not 
> be unwound if an `Error` is thrown.

The stack may not be unwound. That doesn't mean the program is in an 
invalid state, especially if you control all the code you are testing.

For sure, it should be almost never done. But almost never is not never.

> 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

That has no relation to this. There are no unwinding tasks.

-Steve