std.unittests for (final?) review [Update]
Jonathan M Davis
jmdavisProg at gmx.com
Tue Jan 11 14:01:16 PST 2011
On Tuesday, January 11, 2011 12:25:53 Tomek Sowiński wrote:
> Jonathan M Davis napisał:
> > 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.
>
> I just thought "instead of examples using types or functions which don't
> actually exist" meant well-known Phobos functions would be used.
Well, that would be better, but at least when it comes to types, that doesn't
work. Not only is Phobos generally lacking in types, but some of the examples
which show what a typical error message from the functions would look like
require incorrectly implemented types. I might be able to use existing functions
for the examples using functions though.
> > > 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.
>
> I mean now collectException doesn't have a parametrized catch block like
> assertThrown does. If it did, the latter could come down to:
>
> void assertThrown(T : Throwable = Exception, F)
> (lazy F funcToCall, string msg = null, string file =
> __FILE__, size_t line = __LINE__) {
> T e = collectThrown!T(funcToCall);
> if (e is null)
> throw new AssertError(...);
> }
>
> Shortening assertThrown's implementation is a bonus, main gain is better
> collectThrown().
>
> [there's more down]
>
> > > 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.
> >
> > > 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.
>
> OK, you're right. A generic collectThrown() would help also here, though.
I still don't get the point of collectThrown() and what such a function would
do. It just looks like you're wrapping a try-catch block in a function and
returning the exception which was thrown instead of putting the try-catch block
in the function calling collectThrown. It seems pointless.
The whole point of assertThrown is to test that a specific exception type was
thrown from the given function call. If it was, then nothing happens. If it
wasn't, then an AssertError is thrown. Any other exceptions are irrelevant and
will be propagated normally.
The whole point of assertNotThrown is to test that no exception of a specific
type was thrown from the given function call. If none was thrown, then nothing
happens. If one _was_ thrown, then an AssertError is thrown. Any other
exceptions are irrelevant and will be propagated normally.
The idea is to use them to test that a function throws the exceptions that it's
supposed to throw when it's supposed to throw them and that it doesn't throw
those exceptions when it's not supposed to.
> > 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.
>
> The actual code is also repetitive.
>
> At work I just finished a task of realigning applications in the system to
> reflect recent changes in configuration database persistence. Our system
> is mainly written in two languages. One of them took the course of
> abstracting away persistence routines for all applications; reworking took
> a day. Applications in the other language were created by copy-pasting
> existing code and sculpting it to achieve desired functionality. So the
> very same persistence routines were written N times, each different due to
> varied intensity of maintenance and keeping up with changes in the DB
> schema over time. Reworking took nearly a month.
>
> Copy-pasted code is bound to age badly and the only way to treat cancer is
> to kill it now before it grows. It *is* a big deal.
While I agree that refactoring code so that you have a separate function call
instead of copy-pasting code is definitely a good idea and that it's very poor
practice to just copy-paste everything everwhere, I don't agree that some level
of duplication _within_ a function is necessarily a problem - particularly if
the resulting code is harder to understand, which does tend to happen if you're
overly zealous in trying to eliminate duplicate code. If code duplication can be
reasonably removed within a function without harming readability, then it's
likely a good idea to do it. And perhaps I don't pay enough attention to
removing code duplication within functions sometimes, but I don't believe that
any of the code that's duplicated here makes any sense being split out into
separate functions, so I don't think that it's a big deal. However, I will look
at reducing the code duplication in a readable manner. I do, however, rate code
legibility as more important than removing code duplication within a function,
and doing stuff like heavily stuff like unaryFun or binaryFun within a function
just to avoid having separate static if blocks is not the sort of thing that I'm
likely to consider better in many cases, though I get the impression that you
likely would (I don't think that that really applies to these particular
functions though).
- Jonathan M Davis
More information about the Digitalmars-d
mailing list