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

[Jonathan M Davis]

On Monday, January 10, 2011 13:48:50 Tomek Sowiński wrote:
> 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.

??? Those are supposed to be there. All examples are tested in the unit tests 
exactly as they are.

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

Perhaps. If I cut down on the examples though, the usage wouldn't be as clear. 
The idea was to be thorough. Andrei wanted better examples, so I gave better 
examples. However, it is a bit of a balancing act, and I may have put too many 
in. It's debatable. Nick's suggestion of a main description before each 
individual overload would help with that.

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

I really don't see any value to this.

1. You can't do that with assert, and assertPred is essentially supposed to be a 
fancy assert.

2. A number of assertPred overloads don't even have an expected, so it would be 
inconsistent.

3. People already are annoyed enough that the operator doesn't end up between 
the arguments. Putting the result on the left-hand side of the operator like 
that would make it that much more confusing.

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

I suppose that chaining it would be a good idea. I didn't think of that. But if 
you want examples, it's used in the unit tests in this very module, and I used 
it heavily in std.datetime.

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

??? I don't get what you're trying for here. assertThrown isn't trying to 
collect exceptions at all. It's testing whether the given exception was thrown 
like it's supposed to be for the given function call. If it was, then the 
assertion succeeded. If it wasn't, then an AssertError is thrown. Just like 
assert.

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

I really have no problem with them being separate as they are. I think that I 
end up writing them that way because I see them as two separate code paths. It 
wouldn't necessarily be a bad idea to combine them, but I really don't think 
that it's a big deal.

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

Good idea.

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

Doesn't work actually - at least not in the general case (for this particular 
test, it's arguably okay). It doesn't take into account the case where an 
exception other than AssertError is thrown. The exception escapes instead of 
hitting the assertion. I believe that they are the way they are because I was 
essentialy re-writing assertThrown to test assertThrown. Regardless, we're 
talking about the unit tests here, not the actual code, so I don't think that 
it's as big a deal.

> 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%?

I doubt it. I'm likely to have missed _something_. I haven't actually run a code 
coverage tool of any kind. IIRC, dmd -cov will do something along those lines, 
but I've never actually used it. Perhaps I should look into starting to use it. 
I just usually try and be really thorough.

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

Well, Andrei set the vote deadline for February 7th, so it's not like it's being 
rushed. Regardless, the main question is the API. Things like whether the actual 
function bodies are as clear or terse as they maybe should be aren't as big a 
deal IMO (though not unimportant), because those can be fixed later if need be.

- Jonathan M Davis