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