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

[Tomek Sowiński]

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.

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

Not sure if longer means better.

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

I agree. Perhaps a synopsis for the whole module like in std.variant would help too.

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

OK, I understand.

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

I meant a real-life example in documentation. People may often ask themselves "how is it different than !assertThrown()?".

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

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

-- 
Tomek