Re: std.unittests for (final?) review [Update]
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__,
Re: std.unittests for (final?) review [Update]
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,
> >
Re: std.unittests for (final?) review [Update]
On 11/01/11 03:09, Jonathan M Davis wrote: On Monday 10 January 2011 05:40:39 Justin Johansson wrote: On 10/01/11 23:29, Jonathan M Davis wrote: From the sounds of it, if this code gets voted in, it'll be going into std.exception. - Jonathan M Davis May it be asked by what authority you can say that (ie. as said above)? Andrei's posts in this thread. Assuming that the code passes the vote (the deadline for which he set as February 7th), he thinks that std.exception is the best place for it rather than it being in its own module. Oh okay. Thanks for that; I missed Andrei's post in earlier this thread but found it now. Good luck with the voting. -- Justin
Re: std.unittests for (final?) review [Update]
On Monday, January 10, 2011 14:41:51 Jonathan M Davis wrote:
> On Monday, January 10, 2011 13:48:50 Tomek Sowiński wrote:
> > 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.
Actually, I now remember the big reason that your version doesn't work at all.
The AssertError thrown by assert(false); inside of the try block would just be
caught by the catch block. So, it doesn't work at all. You have to do it pretty
much the way that I did it. It was one of those nuances in writing
assertNotThrown that initially missed when coming up with it in the first
place.
So, the ugly way is necessary.
- Jonathan M Davis
Re: std.unittests for (final?) review [Update]
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;
>
>
Re: std.unittests for (final?) review [Update]
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
Re: std.unittests for (final?) review [Update]
"Jonathan M Davis" wrote in message
news:mailman.535.1294662576.4748.digitalmar...@puremagic.com...
>Okay, here's the new code and its documentation: http://is.gd/ktURa
>
>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.
>
>So, now there's just assertThrown, assertNotThrown, collectExceptionMsg,
>and
>assertPred (though there are eight different overloads of assertPred). So,
>review
>away.
>
>From the sounds of it, if this code gets voted in, it'll be going into
>std.exception.
I like it. (I still think a way to make them collect an array of exceptions
instead of throwing should make it's way in there at some point though.)
Couple small things though:
- The error message from the conditional version of assertPred should
probably omit the two words "is not". That much is already clear from the
fact that assertPred threw and says "failed:", and getting rid of it makes
it much more readable as an expression: "failed: [hello] == [goodbye]" or
"failed: [5] < [2]"
- The docs should have a section that gives an overview of assertPred by
using a few simple examples of each of the assertPred uses. The way it is
now, it's hard to see the overall logic behind the design of the
assertPred's params since it's spread out all over. For instance:
-
//Equivalent to assert(4 <= 5);
assertPred!"<="(4, 5);
//Equivalent to assert(1 * 2 == 2);
assertPred!"=="(1 * 2, 2);
//Equivalent to assert(7 + 5 == 12);
assertPred!"+"(7, 5, 12);
/*
Equivalent to
void func(T, U)(T lhs, U rhs)
{
auto result = lhs = rhs;
assert(lhs == rhs);
assert(result == rhs);
}
func(5, 7);
*/
assertPred!"opAssign"(5, 7);
/*
Equivalent to
void func(T, U, E)(T lhs, U rhs, E expected)
{
auto result = lhs += rhs;
assert(lhs == expected);
assert(result == expected);
}
func(5, 7, 12);
*/
assertPred!"-="(7, 5, 2);
assertPred!"*="(7, 5, 35);
//Equivalent to assert("hello " ~ "world" == "hello world");
assertPred!"~"("hello ", "world", "hello world");
//Equivalent to assert(1 == 1);
assertPred!"a == 1"(1);
//Equivalent to assert(2 * 3.0 == 6);
assertPred!"a * 3.0 == 6.0"(2);
//Equivalent to assert(42 == 42);
assertPred!"a == b"(42, 42);
//Equivalent to assert("hello " ~ "world" == "hello world");
assertPred!`a ~ b == "hello world"`("hello ", "world");
//Equivalent to assert((int[] range, int i){return canFind(range, i);}([1,
5, 7, 2], 7));
assertPred!((int[] range, int i){return canFind(range, i);})([1, 5, 7, 2],
7);
//Equivalent to assert((int a, int b, int c){return a == b && b == c;}(5, 5,
5));
assertPred!((int a, int b, int c){return a == b && b == c;})(5, 5, 5);
struct IntWrapper
{
int value;
int opCmp(const ref IntWrapper rhs) const
{
if(value < rhs.value)
return -1;
else if(value > rhs.value)
return 1;
return 0;
}
string toString()
{
return to!string(value);
}
}
// Equivalent to assert(IntWrapper(0).opCmp(IntWrapper(6)) < 0);
assertPred!("opCmp", "<")(IntWrapper(0), IntWrapper(6));
// Equivalent to assert(IntWrapper(42).opCmp(IntWrapper(42)) == 0);
assertPred!("opCmp", "==")(IntWrapper(42), IntWrapper(42));
-
Also notice that I cleaned up a few things, like eliminating the unnecessary
struct constructor and making a few of the assert equivalents more clear.
Re: std.unittests for (final?) review [Update]
On Monday 10 January 2011 05:40:39 Justin Johansson wrote: > On 10/01/11 23:29, Jonathan M Davis wrote: > > From the sounds of it, if this code gets voted in, it'll be going into > > > > std.exception. > > > > - Jonathan M Davis > > May it be asked by what authority you can say that (ie. as said above)? Andrei's posts in this thread. Assuming that the code passes the vote (the deadline for which he set as February 7th), he thinks that std.exception is the best place for it rather than it being in its own module. - Jonathan M Davis
Re: std.unittests for (final?) review [Update]
On 10/01/11 23:29, Jonathan M Davis wrote: From the sounds of it, if this code gets voted in, it'll be going into std.exception. - Jonathan M Davis May it be asked by what authority you can say that (ie. as said above)?
Re: std.unittests for (final?) review [Update]
Okay, here's the new code and its documentation: http://is.gd/ktURa 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. So, now there's just assertThrown, assertNotThrown, collectExceptionMsg, and assertPred (though there are eight different overloads of assertPred). So, review away. From the sounds of it, if this code gets voted in, it'll be going into std.exception. - Jonathan M Davis
