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]
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);
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__, __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
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
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]
Jonathan M Davis jmdavisp...@gmx.com 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]
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]
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
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
