Review: A new stab at a potential std.unittests

[Jonathan M Davis]

On Saturday 20 November 2010 10:16:55 Lutger Blijdestijn wrote:
> I'm not particularly fond of this interface and think that a solution with
> a delegate / lazy or alias template parameter would be more convenient.
> However, until we have ast macros I do see the added value in this
> approach.
> 
> Some remarks about the api, not a proper review of the code itself:
> 
> - core.exception and std.string must be imported to use the module,
> relevant symbols should be selectively and publicy imported instead.

I hadn't thought of that. Good idea.

> - exception would be better abbreviated as ex instead of exc, I believe
> this is more conventional. (assertExThrown instead of assertExcThrown)

I'll think about it. I don't think that I've often seen it abbreviated at all. 
But if Ex really is more common, then that would be better.

> - assertExcThrown should fail if an exception is thrown that is not T or a
> subclass of T. (catch and throw AssertError)

It currently catches the exact exception (well, throwable) that you tell it to. 
The others are let through exactly like they would be with an assert. It also 
means that you get to see exactly what the erroneous exception was so that you 
can deal with it. An AssertError would be less informative, maybe even 
misleading. You get an AssertError because it didn't throw any exception, 
whereas you get the wrong exception if it threw the wrong one.

I suppose that I could see an argument that because it didn't throw that exact 
exception, it should throw an AssertError, but I think that it's worse, because 
you can't see what the unexpected exception was. It would be less helpful in 
debugging rather than more.

> - assertEqual and assertNotEqual is inconsistent in naming with the
> assertOpFoo class of functions

Not quite. assertEqual() doesn't explicitly test opEqual(). It uses ==. So, it 
works with primitives as well as structs that didn't declare an opEquals(). 
assertOpCmp() on the other hand, explicitly calls opCmp() - so, for instance, 
assertOpCmp!"=="() specifically tests that opCmp() returned zero rather than 
using ==. However, assertOpOpAssign doesn't actually call opOpAssign(), so there 
arguably is an inconsistency there. I'll have to think about how it would be 
best to adjust those names. I think that assertEqual() should definitely stay 
assertEqual() - it doesn't explicitly call opEquals() and other unit testing 
frameworks frequent have a function named assertEquals(), so it will be more 
familiar to many. I also think that assertOpCmp!() should stay as it is, since 
it is explicitly calling opCmp(). The really question is what to rename 
assertOpOpAssign!() to, since like with assertEqual(), it would be too 
restrictive to directly call opOpAssign(), and I can't rename it 
assertOpAssign() because opAssign() is something else entirely. So, if a 
different name would be better, I'm going to have to find one, since I don't know 
what else to name it.

> I believe these assertions should be added:
> 
> - assertExcThrown and assertExcNotThrown with a default T (should be
> Exception, not Throwable)

That's not a bad idea, though personally, I tend to think that if someone used 
an Exception rather than a derived type that they aren't be specific enough (I'm 
sure that many would argue with me on that one though). It might work to just 
give it a default parameter (and I do agree that the default should be Exception 
rather than Throwable).

> - something equivalent to std.algorithm.equal, the latter is very useful in
> unittests

I'm not sure that I'd noticed std.algorithm.equal() before. That's a good idea. 
assertEqual() could simply be templatized in the same manner as 
std.algorithm.equal().

> - assertOpBinary, assertTrue and perhaps assertPred (where the predicate is
> an template alias parameter)

I decided that assertTrue() was a waste of time, because that's what a bare 
assert does. The same goes for assertFalse(), because all you have to do is add 
! in front of the expression. All of the functions that I have are there to 
either improve the output (such as printing out the actual values of the two 
values being compared in assertEqual()), or they get rid of boilerplate code 
(such as the try-catch block and other details in assertExcThrown!()).

As for assertOpBinary, are you suggesting a function which does the given binary 
operation and then compares the result with the given expected result? That 
would be a good addition.

As for assertPred. I don't know what that would do. Would that take a predicate 
and its parameters and then assert that the result was true? If that's what 
you're looking for, then assertPredTrue and assertPredFalse would be better. I 
think that you need to clarify quite what you meant though.

I pretty much created the unit test functions which I needed for std.datetime. 
So, I'm sure that there are other good ones which could be added. And I think 
that we should do so. My main concern though has been that because some version 
of these is going to have to go into Phobos when std.datetime does (be it as 
private in std.datetime or as its own module), I wanted to try and get the unit 
test functions themselves vetted and get them put in their own module. That way 
other people can use them, and more can be added later. So, I haven't been as 
concerned with finding every possibly useful unit testing function - particularly 
when for some of these, you really have to use them to see how useful they 
really are. But I'm definitely open to suggestions as to good unit testing 
functions to add.

- Jonathan M Davis