std.unittests for (final?) review

[Jonathan M Davis]

On Wednesday, January 05, 2011 16:09:17 Andrei Alexandrescu wrote:
> On 1/2/11 10:44 PM, Jonathan M Davis wrote:
> > Okay. As some of you are aware, I created a module with unit testing
> > functions when I wrote std.datetime, and it really helped with writing
> > the unit tests for std.datetime. It has been at least somewhat reviewed
> > here previously, and that has definitely improved it. However, it has
> > not yet been decided whether it's going to go into Phobos along with
> > std.datetime. I'd like it to, but it hasn't been voted on, and I'm not
> > sure that it's been reviewed as heavily as it should be. So, I'm posting
> > the most up-to-date version in the hopes of getting any necessary
> > changes found and made so that it can be voted on. Andrei has yet to
> > weigh in on std.unittests at all, so I don't know if we're actually
> > going to get a vote on it, but I'm presenting it for (final?) review so
> > that it can be voted in if that's what he wants to do.
> > 
> > The code: http://is.gd/jZEVl
> 
> Honest I feared I'll see a monster with a lot of useless crap, so the
> surprise was nice. I appreciate that the module is small and to the
> point. It contains functions that I can easily imagine needing while
> unittesting while at the same time are tedious enough to emulate with
> inline code.

Well, most of the functions were created out of need when developing 
std.datetime rather than just because they seemed useful. If I had simply been 
trying to create a unit testing module to create a unit testing module, it might 
have been much larger.  Then again, I've tried to come up with more functions 
which might be useful, and I couldn't think of many. As it is, several of the 
functions that are there are there because of suggestions in previous threads.

> Suggestions:
> 
> * Documentation should contain meaningful, working examples that
> exercise the most expected usage patterns of the functions.

They should for the most part, but several of them explicitly test overloaded 
operators, which requires a struct or class with the appropriate operator, which 
would have made the examples much larger, since there aren't really any in 
druntime or Phobos. So, in those cases, the examples don't work. Perhaps that 
was a poor decision though, and I should have just added minimal type definitions 
to the examples.

> * assertEqual and assertNotEqual are misnomers. They should be
> consolidated into one function called assertPredicate:
> 
> assertPredicate(1 + 1, 2); // pass, default predicate is a == b
> assertPredicate!"a < b"(1 + 1, 3); // pass, 1 + 1 < 3
> 
> * To emphasize: since we have the awesome short lambdas, there's no need
> to for a negation function - we can embed the negation inside the lambda.

Valid point. assertEqual didn't even take a predicate originally, so it didn't 
occur to me to combine assertEqual and assertNotEqual. I added the predicate 
after someone suggested that it be more like std.algorithm.equal.

> * Remove assertCmp and fold it into assertPredicate. With trivial
> metaprogramming you can distinguish an operator from an actual expression.
> 
> assertPredicate!"<"(1 + 1, 2.1);
> 
> * The explanation and the examples do not clarify for me what value
> assertOpCmp adds over assertCmp.

The key difference is that assertOpCmp explicitly calls the opCmp() while 
assertCmp just uses the given operator. This makes it possible to check that 
opCmp() returns 0 when it's supposed to rather than just having opEquals() 
checked when checking ==. Now, with a more general predicate, you could feed it 
opCmp and get around that problem, though it would be slightly more tedious. I 
fully understand the confusion between the two though. I originally just had 
assertOpCmp() and added assertCmp() after realizing that assertOpCmp() wouldn't 
work for primitive types and the like, and people are obviously going to want to 
be able to test primitives with comparisons other than equality. As long as I 
can get assertPred to test both the operators and the actual overloaded operator 
functions, then combining that way would certainly be ideal.

> * I'd replace assertOpBinary with a ternary predicate and fold that into
> assertPredicate, too. That is a bit more difficult but worth looking into.

That may take some doing, but it could certainly be worth doing. I'll look into 
it.

> * I think assertOpOpAssign is too rarely necessary to warrant inclusion.

I think that I actually am using it in std.datetime. I don't think that it's 
something that I would have come up with otherwise. But maybe I'm just more 
thorough about that sort of testing than most people. You can replace it with 
two tests though, so getting rid of it isn't necessarily a big deal, just 
somewhat less efficient.

> * Wow, assertPred was there at the bottom the whole time :o). Rename my
> assertPredicate into assertPred, fold the existing assertPred into it
> and at the end we have a useful library with a one-stop-shop function.

Well, it was the last one I came up with (based on a suggestion; I haven't 
actually used it in std.datetime), so it ended up at the bottom. But if 
assertPred can indeed be made powerful enough to absorb the functionality of 
most of the other functions, then that would definitely be worth doing. It'll 
take a bit of work though.

> * getMsg() should be renamed as collectExceptionMsg() because we already
> have collectException() in std.exception.

Fine with me. I was looking for the functionality. The name isn't terribly 
important as long as its clear.

> If some or all of the suggestions above are implemented (which I'd
> love), the library becomes lean and mean enough to be integrated within
> an existing module. I think that would be awesome.

I'll try and do most of them. It'll certainly be a challeng to implement such a 
powerful assertPred though. I'm just glad that D templates are so powerful. I'm 
not good enough with templates in C++ to even dream of doing some of the stuff 
that I do with D templates, and some if it just isn't possible. I'm going to be 
porting a portion of std.datetime to C++ for use at work, and some of the stuff 
in there (like the nice convert function) just can't be done the same way in 
C++. After using D templates, C++ templates seem really unwieldy.

If I'm able to combine most of the functions into a single assertPred as you 
suggest, then it does definitely look like a whole module is overkill. However, 
we're then going to have to decide where to put it. std.exception would probably 
be the best place, though it does strike me as somewhat of an odd place to put 
them. Of course, almost all of them _do_ throw AssertError, and the one 
exception to that (getMsg) operates on an exception (well, Throwable), so they 
do at least sort of fit.

- Jonathan M Davis