Review: A new stab at a potential std.unittests

[Lutger Blijdestijn]

Jonathan M Davis wrote:

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

I know of std.exception.enforceEx, and catch(Exception ex) is also regularly 
used in examples.
 
>> - 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.

Suppose you want assert that a FileNotFoundException is thrown. Now if you 
get an Exception then:
- technically the unittest has passed because no AssertError has been thrown 
(splitting hairs) 
- if for whatever reason you catch this error in the unittest, things will 
get screwed up
- You (or possibly some other script) won' t get the same command line 
output, it's then harder to correlate the exception with the assertion

I agree about wanting to know the original Exception though. I think this is 
possible by setting the .next field of AssertError with that exception.

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

I agree but it doesn't matter for general use, people will want this and 
this is also practice in phobos (mostly through the use of enforce I think).
 
...
> 
>> - 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!()).

Except that assert does not print the expression which resulted in the 
assertion like all other functions do, so assertTrue does improve the output 
too.
 
> 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.

bool isSorted();
int[] numbers = getSortedNumbers();
assertPred!isSorted(numbers);

It's the same as assert(isSorted(numbers)), except it allows for improved 
output. Not very important, but I find it common to use such predicates for 
testing so it might help. 

Alternatively assertTrue and assertFalse could take an optional predicate, 
defaulting to the identity and negation respectively:

assertTrue!isSorted(numbers);