Review: A new stab at a potential std.unittests

[Jonathan M Davis]

On Sunday 21 November 2010 08:11:06 Lutger Blijdestijn wrote:
> 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.z

Then, I'll change it to assertExThrown.

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

Actually, not only does AssertError not take a next as one if its constructor 
parameters, but even if you set it directly before throwing it, it still doesn't 
print out the other exception. So, if you throw an AssertError in the case of 
the wrong exception, the best that you're going to get is a message saying what 
type of exception was thrown and maybe what it said, but you couldn't get its 
stack trace, which could be far more important.

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

I'll do it, but I do think that it's generally bad practice to throw an 
Exception rather than a subtype of Exception (except perhaps in a script or 
other similarly short program).

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

Neither do any of the others. They print the value. So if 
assertEqual(func("hello", "world"), 2); on line 115 of config.d failed because 
func("hello", "world") returned a 3, you'd get

core.exception.AssertError at config.d(115): assertEquals() failed: actual [3], 
expected [2].

whereas with assert(func("hello", "world") == 2); you'd get

core.exception.AssertError at config(115): unittest failure

assertEqual() is way more informative, but it doesn't print the actual 
expression, just the result of the expression. Short of using string mixins, I 
don't know how to do any better than that. But I don't think that you need to 
either. The file and line number are plenty for you to be able to go and see what 
the actual expression was - even with just the assert. What assertEqual() does 
is give you what the actual values being compared was. That being the case, 
assertTrue() and assertFalse() wouldn't help you any. If assertTrue() failed, 
then the result was false. If assertFalse() failed, then the result was true.

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

I'd lean toward something like assertPred!() or assertPredTrue!(). Like I said, 
I see no point in a general assertTrue because it can't actually add anything 
over assert - at least not in any sane way that I can see. I'll look at adding 
something like assertPred!() though.

- Jonathan M Davis