std.unittests for (final?) review [Update]

[Tomek Sowiński]

Jonathan M Davis napisał:

> I followed Andrei's suggestion and merged most of the functions into a highly 
> flexible assertPred. I also renamed the functions as suggested and attempted to 
> fully document everything with fully functional examples instead of examples 
> using types or functions which don't actually exist.

Did you zip the right file? I still see things like nameFunc and assertPlease.

On the whole the examples are too long. It's just daunting I can't see docs for *one* function without scrolling. Please give them a solid hair-cut -- max 10 lines with a median of 5. The descriptions are also watered down by over-explanatory writing.

> So, now there's just assertThrown, assertNotThrown, collectExceptionMsg, and 
> assertPred (though there are eight different overloads of assertPred). So, review 
> away.

Some suggestions:

assertPred:
Try putting expected in front; uniform call syntax can then set it apart from the operands:
assertPred!"%"(7, 5, 2); // old
2.assertPred!"%"(7, 5); // new

assertNotThrown: chain the original exception with AssertError as its cause?
Oh, this one badly needs a real-life example.

assertThrown: I'd rather see generified collectException (call it collectThrown?). assertThrown may stay as a convenience wrapper, though.


Looking at the code I'm seeing the same cancerous coding style std.datetime suffered from (to a lesser extent, I admit).

For instance, this routine:

    if(result != expected)
    {
        if(msg.empty)
        {
            throw new AssertError(format(`assertPred!"%s" failed: [%s] %s [%s]: actual [%s], expected [%s].`,
                                         op,
                                         lhs,
                                         op,
                                         rhs,
                                         result,
                                         expected),
                                   file,
                                   line);
        }
        else
        {
            throw new AssertError(format(`assertPred!"%s" failed: [%s] %s [%s]: actual [%s], expected [%s]: %s`,
                                         op,
                                         lhs,
                                         op,
                                         rhs,
                                         result,
                                         expected,
                                         msg),
                                  file,
                                  line);
        }
    }

can be easily compressed to:

enforce(result==expected, new AssertError(
    format("[%s] %s [%s] failed: actual [%s], expected [%s]" ~ (msg.empty ? "." : ": %s"),
        op, lhs, op, rhs, result, expected, msg), file, line));

BTW, actual and expected should be in new lines directly under each other for eye-diffing (does wonders for long input):
format("[%s] %s [%s] failed:\n[%s] - actual\n[%s] - expected" ~ ...

Another example:

{
        bool thrown = false;
        try
            assertNotThrown!AssertError(throwEx(new AssertError("It's an AssertError", __FILE__, __LINE__)), "It's a message");
        catch(AssertError)
            thrown = true;

        assert(thrown);
    }

can be:

    try {
        assertNotThrown!AssertError(throwEx(new AssertError("It's an AssertError", __FILE__, __LINE__)), "It's a message");
        assert(false);
    } catch(AssertError) { /*OK*/ }

and you don't have to introduce a new scope every time.

Not to mention that such routines recur in your code with little discrepancies, so abstracting out private helpers may pay off. Fixing such "readability bugs" is essential for a standard library module.

On the bright side, I do appreciate the thoroughness and extent of unittests in this module. Is coverage 100%?

> From the sounds of it, if this code gets voted in, it'll be going into std.exception.

Please don't rush the adoption. This module, albeit useful, still needs work.

-- 
Tomek