It looks as though Jonathan is willing to roll up the code into loops, so having this debate is academic at this point, but I wanted to respond to some points.


----- Original Message -----
> From:Andrei Alexandrescu <andrei at erdani.com>
> 
> On Feb 11, 2011, at 2:39 PM, Steve Schveighoffer <schveiguy at yahoo.com> wrote:

> > Please please, let's *NOT* make this a standard practice.  If a test
> fails, I don't want to get a debugger out or start printf debugging *to find the unit test*.  I want it to tell me where it failed, and focus on fixing the problem.
> 
> You find the unittest alright. With coming improvements to assert you will often see the arguments that caused the trouble.

This will help immensely.  Right now, you get a line number.  The rule should be, if a unit test fails, it should give you enough information to 1) find the failing assert and 2) give you all the information to understand why the assert failed.

> I don't understand how we both derive vastly different conclusions from the same extensive eperience with unittests. To me a difficult unittest to find is a crashing one; never ever once in my life I had problems figuring out why an assert fails in a uniitrst, and worse, I am incapable to imagine such.

foreach(x; someLongArray)
  assert(foo(x) > 5);

which x caused the problem?  all I get is the line number for the assert.

BTW, I had this happen in Tango unit tests that used loops (ironically, when I was rewriting Tango's date/time code), I had to instrument the code with printfs which sucked IMO.

> > I don't sympathize with you, we have tools to easily do this without
> much burden.
> 
> A little while ago you didn't care to start a debugger or use writeln - two simple tools. I cry double standard.

Um.. how are you reading the code if not using an editor?  Using ctrl-F and typing in the function name is several orders of magnitude simpler than adding writelns, recompiling, or recompiling in debug mode (I'm counting the part where you have to dissect the makefile to figure out how to put the thing in debug mode in the first place) and debugging.  It's like saying the burden of using the claw of a hammer that's already in your hand is the same as going to get a reciprocating saw to remove a nail.  It's not even close to a double standard.

> >  If you want to find a function to read, use your editor's find
> feature.  Some even can just allow you to click on the function and find the definition.  This argument is a complete red herring
> 
> I find it a valid argument. I and I suspect many just browse code to get a feel of it.

Which you can.  I think you can also get a feel for how well tested the library is also ("wow, look at all the unit tests...").

> > Good unit tests are independent and do not affect one another.  Jonathan is
> right, it's simply a different goal for unit tests, you want easily isolated blocks of code that are simple to understand so when something goes wrong you can work through it in your head instead of figuring out how the unit test works.
> 
> For me repeated code is worse by all metrics. There is zero advantages that it has, and only troubles.

Then I guess we differ.  I prefer to have unit tests be simple statements that are easily proven mentally without much context.  Complex code is more difficult to reason about, and as spir said, we don't want to have to unit test our unit tests.

I attach to this statement that the unit tests should test *different* things even if they look repetitive.  For example, testing two corner cases might look very repetitive, but they both are looking for weaknesses in different places.

Having extra unit tests that do the same exact thing is not productive, I agree there.

Let's look at a different type of repetition -- version statements.  The nature of version statements sometimes forces one to sometimes repeat whole sections of code.  However, understanding the code is much easier than some of the horrific abuses of C preprocessor stuff that I've seen.  At some point, factoring out repetitive code becomes more harmful than the alternative.  I feel like unit tests are one of those cases.

> > *That* being said, I have not read through all the datetime unit tests, and
> I don't know the nature of how many could be omitted.
> 
> Please do. I had a completely different opinion (virtually same as yours) before making a thorough pass thru it. Again, like in the sorcerer's apprentice, it's not about the deed, but instead the frightening scale at which the deed is realized.

When I have time, I'll peruse it.

-Steve

On 02/11/2011 08:20 PM, Steve Schveighoffer wrote:
>   At some point, factoring out repetitive code becomes more harmful than the alternative.  I feel like unit tests are one of those cases.

Hum, had a similar thought recently. I started to "unfactor out" some pieces of
code into very basic caller funcs (the kind that constantly get called, thus
would appear on top of a profiler's output in terms of number of executions).
The initial reason was lack of knowledge about whether the tool funcs would be
inlined or not (reason for the other thread).
Then, I realised the result is sometimes easier to understand, provided the
initially factored out part is properly "titled" by a comment, especially when
it is non-obvious to identify as a kind of sub-task.

Denis
-- 
_________________
vita es estrany
spir.wikidot.com

On Fri, 11 Feb 2011 14:20:09 -0500, Steve Schveighoffer <schveiguy at yahoo.com> wrote:
> It looks as though Jonathan is willing to roll up the code into loops, so having this debate is academic at this point, but I wanted to respond to some points.
>
>
> ----- Original Message -----
>> From:Andrei Alexandrescu <andrei at erdani.com>
>>
>> On Feb 11, 2011, at 2:39 PM, Steve Schveighoffer <schveiguy at yahoo.com> wrote:
>
>> > Please please, let's *NOT* make this a standard practice.  If a test
>> fails, I don't want to get a debugger out or start printf debugging *to
>> find
>> the unit test*.  I want it to tell me where it failed, and focus on
>> fixing the
>> problem.
>>
>> You find the unittest alright. With coming improvements to assert you
>> will often
>> see the arguments that caused the trouble.
>
> This will help immensely.  Right now, you get a line number.  The rule should be, if a unit test fails, it should give you enough information to 1) find the failing assert and 2) give you all the information to understand why the assert failed.
>
>> I don't understand how we both derive vastly different conclusions from
>> the
>> same extensive eperience with unittests. To me a difficult unittest to
>> find is a
>> crashing one; never ever once in my life I had problems figuring out
>> why an
>> assert fails in a uniitrst, and worse, I am incapable to imagine such.
>
> foreach(x; someLongArray)
>   assert(foo(x) > 5);
>
> which x caused the problem?  all I get is the line number for the assert.

*sigh*

  foreach(x; someLongArray)
    assert(foo(x) > 5, text(x) );

Problem solved. Personally, I don't usually add the extra clause to assert during initial coding; only when they fail do I add something like text("\nx:\t",x,"\nfoo:\t",foo(x)); or whatever. (Though enforces are a different story)

On 2/11/11 1:20 PM, Steve Schveighoffer wrote:
> It looks as though Jonathan is willing to roll up the code into loops, so having this debate is academic at this point, but I wanted to respond to some points.

Agreed. However, the horror of leaving this in limbo spurs me into continuing a debate that I feel is Kafkian. (I don't want to get beheaded through my inaction!)

> ----- Original Message -----
>> From:Andrei Alexandrescu<andrei at erdani.com>
>>
>> On Feb 11, 2011, at 2:39 PM, Steve Schveighoffer<schveiguy at yahoo.com> wrote:
>
>>> Please please, let's *NOT* make this a standard practice.  If a test
>> fails, I don't want to get a debugger out or start printf debugging *to find the unit test*.  I want it to tell me where it failed, and focus on fixing the problem.
>>
>> You find the unittest alright. With coming improvements to assert you will often see the arguments that caused the trouble.
>
> This will help immensely.  Right now, you get a line number.  The rule should be, if a unit test fails, it should give you enough information to 1) find the failing assert and 2) give you all the information to understand why the assert failed.
>
>> I don't understand how we both derive vastly different conclusions from the same extensive eperience with unittests. To me a difficult unittest to find is a crashing one; never ever once in my life I had problems figuring out why an assert fails in a uniitrst, and worse, I am incapable to imagine such.
>
> foreach(x; someLongArray)
>    assert(foo(x)>  5);
>
> which x caused the problem?  all I get is the line number for the assert.

When the unittest fails I go and edit the code as such:

     assert(foo(x), text("foo(", x, ") failed"));

Doesn't cost a damn thing, doesn't add to the line count, and when it fires adds a wealth of info.

> BTW, I had this happen in Tango unit tests that used loops (ironically, when I was rewriting Tango's date/time code), I had to instrument the code with printfs which sucked IMO.

See above.

>>> I don't sympathize with you, we have tools to easily do this without
>> much burden.
>>
>> A little while ago you didn't care to start a debugger or use writeln - two simple tools. I cry double standard.
>
> Um.. how are you reading the code if not using an editor?  Using ctrl-F and typing in the function name is several orders of magnitude simpler than adding writelns, recompiling, or recompiling in debug mode (I'm counting the part where you have to dissect the makefile to figure out how to put the thing in debug mode in the first place) and debugging.  It's like saying the burden of using the claw of a hammer that's already in your hand is the same as going to get a reciprocating saw to remove a nail.  It's not even close to a double standard.

How are you debugging code if not using stdio or a debugger?

>>>   If you want to find a function to read, use your editor's find
>> feature.  Some even can just allow you to click on the function and find the definition.  This argument is a complete red herring
>>
>> I find it a valid argument. I and I suspect many just browse code to get a feel of it.
>
> Which you can.  I think you can also get a feel for how well tested the library is also ("wow, look at all the unit tests...").

That's what I said until I got drowned in'em. If the usual D application or library needs such massive unittests and written in such a repetitive manner, D has failed.

>>> Good unit tests are independent and do not affect one another.  Jonathan is
>> right, it's simply a different goal for unit tests, you want easily isolated blocks of code that are simple to understand so when something goes wrong you can work through it in your head instead of figuring out how the unit test works.
>>
>> For me repeated code is worse by all metrics. There is zero advantages that it has, and only troubles.
>
> Then I guess we differ.  I prefer to have unit tests be simple statements that are easily proven mentally without much context.  Complex code is more difficult to reason about, and as spir said, we don't want to have to unit test our unit tests.

A simple loop is simpler than 100 unrolled instances of it.

> I attach to this statement that the unit tests should test *different* things even if they look repetitive.  For example, testing two corner cases might look very repetitive, but they both are looking for weaknesses in different places.

I am certain that the unittests in std.datetime are well beyond 100% code coverage. IMHO they should stop at 100% coverage (which is probably at 20% of their size).

> Having extra unit tests that do the same exact thing is not productive, I agree there.

And I guarantee you std.datetime saturates coverage at 100% with a fraction of the size. I mean it's obvious - how many paths are in e.g. toISOExtendedString?

> Let's look at a different type of repetition -- version statements.  The nature of version statements sometimes forces one to sometimes repeat whole sections of code.  However, understanding the code is much easier than some of the horrific abuses of C preprocessor stuff that I've seen.  At some point, factoring out repetitive code becomes more harmful than the alternative.  I feel like unit tests are one of those cases.

Again, I feel like you do, save the numbers. Let me give you an example. Many would agree that sex is good. Yet few would agree that being required to perform it for 12 hours can be terrible. Eating is good too. Eating seven pounds of even the best food is a horrific experience.

There is a limit to everything. That limit has been passed many times over by the unittests in std.datetime.

BTW, I'm familiar with the version argument (aired originally by Walter) and my stance is as loud and clear as ever. I am proactively trying to refactor code to avoid needless duplication. See e.g. std.file.read, which I refactored to use individual version() statements instead of wholesale version() as was initially in that module.


Andrei

On 2/11/11 1:52 PM, spir wrote:
> On 02/11/2011 08:20 PM, Steve Schveighoffer wrote:
>> At some point, factoring out repetitive code becomes more harmful than the alternative. I feel like unit tests are one of those cases.
>
> Hum, had a similar thought recently. I started to "unfactor out" some
> pieces of code into very basic caller funcs (the kind that constantly
> get called, thus would appear on top of a profiler's output in terms of
> number of executions). The initial reason was lack of knowledge about
> whether the tool funcs would be inlined or not (reason for the other
> thread).
> Then, I realised the result is sometimes easier to understand, provided
> the initially factored out part is properly "titled" by a comment,
> especially when it is non-obvious to identify as a kind of sub-task.

Consider the code below. It is a simplification of 60 lines of code in std.datetime, which I paste at the end of this message. The code is formatted at 80 columns, at the same time showing that you can write compact _and_ good _and_ simple _and_ effective code in 80 columns. (It wraps in the email because email wraps at 75 columns or so.)

The space taken is 24/60 lines = 40%. Less than half of the original - all with generous indentation and narrow columns. Clarity greatly gained, bulk saved, maintenance simplified, failure clarified, and your shoes are shiny. "What else can we do?" Really if there's any debate that this is not a huge improvement I give up. Behead me.

     unittest
     {
         auto badDates =
             [
                 "", "990704", "0100704", "2010070", "2010070 ",
                 "120100704", "-0100704", "+0100704", "2010070a",
"20100a04",
                 "2010a704", "99-07-04", "010-07-04", "2010-07-0",
"2010-07-0 ",
                 "12010-07-04", "-010-07-04", "+010-07-04", "2010-07-0a",
                 "2010-0a-04", "2010-a7-04", "2010/07/04", "2010/7/04",
                 "2010/7/4", "2010/07/4", "2010-7-04", "2010-7-4",
"2010-07-4",
                 "99Jul04", "010Jul04", "2010Jul0", "2010Jul0 ",
"12010Jul04",
                 "-010Jul04", "+010Jul04", "2010Jul0a", "2010Jua04",
"2010aul04",
                 "99-Jul-04", "010-Jul-04", "2010-Jul-0", "2010-Jul-0 ",
                 "12010-Jul-04", "-010-Jul-04", "+010-Jul-04",
"2010-Jul-0a",
                 "2010-Jua-04", "2010-Jal-04", "2010-aul-04", "2010-07-04",
                 "2010-Jul-04",
             ];

         foreach (badDate; badDates)
         {
             assertThrown!DateTimeException(Date.fromISOString(badDate),
                 "Accepts bad date: " ~ badDate);
         }
     }


Andrei

P.S. Original code:

     unittest
     {
         version(testStdDateTime)
         {
             assertThrown!DateTimeException(Date.fromISOString(""));
             assertThrown!DateTimeException(Date.fromISOString("990704"));
             assertThrown!DateTimeException(Date.fromISOString("0100704"));
             assertThrown!DateTimeException(Date.fromISOString("2010070"));
             assertThrown!DateTimeException(Date.fromISOString("2010070 "));

assertThrown!DateTimeException(Date.fromISOString("120100704"));
             assertThrown!DateTimeException(Date.fromISOString("-0100704"));
             assertThrown!DateTimeException(Date.fromISOString("+0100704"));
             assertThrown!DateTimeException(Date.fromISOString("2010070a"));
             assertThrown!DateTimeException(Date.fromISOString("20100a04"));
             assertThrown!DateTimeException(Date.fromISOString("2010a704"));

             assertThrown!DateTimeException(Date.fromISOString("99-07-04"));

assertThrown!DateTimeException(Date.fromISOString("010-07-04"));

assertThrown!DateTimeException(Date.fromISOString("2010-07-0"));

assertThrown!DateTimeException(Date.fromISOString("2010-07-0 "));

assertThrown!DateTimeException(Date.fromISOString("12010-07-04"));

assertThrown!DateTimeException(Date.fromISOString("-010-07-04"));

assertThrown!DateTimeException(Date.fromISOString("+010-07-04"));

assertThrown!DateTimeException(Date.fromISOString("2010-07-0a"));

assertThrown!DateTimeException(Date.fromISOString("2010-0a-04"));

assertThrown!DateTimeException(Date.fromISOString("2010-a7-04"));

assertThrown!DateTimeException(Date.fromISOString("2010/07/04"));

assertThrown!DateTimeException(Date.fromISOString("2010/7/04"));
             assertThrown!DateTimeException(Date.fromISOString("2010/7/4"));

assertThrown!DateTimeException(Date.fromISOString("2010/07/4"));

assertThrown!DateTimeException(Date.fromISOString("2010-7-04"));
             assertThrown!DateTimeException(Date.fromISOString("2010-7-4"));

assertThrown!DateTimeException(Date.fromISOString("2010-07-4"));

             assertThrown!DateTimeException(Date.fromISOString("99Jul04"));
             assertThrown!DateTimeException(Date.fromISOString("010Jul04"));
             assertThrown!DateTimeException(Date.fromISOString("2010Jul0"));
             assertThrown!DateTimeException(Date.fromISOString("2010Jul0
"));

assertThrown!DateTimeException(Date.fromISOString("12010Jul04"));

assertThrown!DateTimeException(Date.fromISOString("-010Jul04"));

assertThrown!DateTimeException(Date.fromISOString("+010Jul04"));

assertThrown!DateTimeException(Date.fromISOString("2010Jul0a"));

assertThrown!DateTimeException(Date.fromISOString("2010Jua04"));

assertThrown!DateTimeException(Date.fromISOString("2010aul04"));


assertThrown!DateTimeException(Date.fromISOString("99-Jul-04"));

assertThrown!DateTimeException(Date.fromISOString("010-Jul-04"));

assertThrown!DateTimeException(Date.fromISOString("2010-Jul-0"));

assertThrown!DateTimeException(Date.fromISOString("2010-Jul-0 "));

assertThrown!DateTimeException(Date.fromISOString("12010-Jul-04"));

assertThrown!DateTimeException(Date.fromISOString("-010-Jul-04"));

assertThrown!DateTimeException(Date.fromISOString("+010-Jul-04"));

assertThrown!DateTimeException(Date.fromISOString("2010-Jul-0a"));

assertThrown!DateTimeException(Date.fromISOString("2010-Jua-04"));

assertThrown!DateTimeException(Date.fromISOString("2010-Jal-04"));

assertThrown!DateTimeException(Date.fromISOString("2010-aul-04"));


assertThrown!DateTimeException(Date.fromISOString("2010-07-04"));

assertThrown!DateTimeException(Date.fromISOString("2010-Jul-04"));
         }
     }

On 2/11/11 3:08 PM, Robert Jacques wrote:
> Problem solved. Personally, I don't usually add the extra clause to
> assert during initial coding; only when they fail do I add something
> like text("\nx:\t",x,"\nfoo:\t",foo(x)); or whatever. (Though enforces
> are a different story)

Exactly so, word for word!

Andrei

On 02/11/2011 10:33 PM, Andrei Alexandrescu wrote:
> On 2/11/11 1:52 PM, spir wrote:
>> On 02/11/2011 08:20 PM, Steve Schveighoffer wrote:
>>> At some point, factoring out repetitive code becomes more harmful than the alternative. I feel like unit tests are one of those cases.
>>
>> Hum, had a similar thought recently. I started to "unfactor out" some
>> pieces of code into very basic caller funcs (the kind that constantly
>> get called, thus would appear on top of a profiler's output in terms of
>> number of executions). The initial reason was lack of knowledge about
>> whether the tool funcs would be inlined or not (reason for the other
>> thread).
>> Then, I realised the result is sometimes easier to understand, provided
>> the initially factored out part is properly "titled" by a comment,
>> especially when it is non-obvious to identify as a kind of sub-task.
>
> Consider the code below. It is a simplification of 60 lines of code in std.datetime, which I paste at the end of this message. The code is formatted at 80 columns, at the same time showing that you can write compact _and_ good _and_ simple _and_ effective code in 80 columns. (It wraps in the email because email wraps at 75 columns or so.)
>
> The space taken is 24/60 lines = 40%. Less than half of the original - all with generous indentation and narrow columns. Clarity greatly gained, bulk saved, maintenance simplified, failure clarified, and your shoes are shiny. "What else can we do?" Really if there's any debate that this is not a huge improvement I give up. Behead me.
>
>      unittest
>      {
>          auto badDates =
>              [
>                  "", "990704", "0100704", "2010070", "2010070 ",
>                  "120100704", "-0100704", "+0100704", "2010070a", "20100a04",
>                  "2010a704", "99-07-04", "010-07-04", "2010-07-0", "2010-07-0 ",
>                  "12010-07-04", "-010-07-04", "+010-07-04", "2010-07-0a",
>                  "2010-0a-04", "2010-a7-04", "2010/07/04", "2010/7/04",
>                  "2010/7/4", "2010/07/4", "2010-7-04", "2010-7-4", "2010-07-4",
>                  "99Jul04", "010Jul04", "2010Jul0", "2010Jul0 ", "12010Jul04",
>                  "-010Jul04", "+010Jul04", "2010Jul0a", "2010Jua04", "2010aul04",
>                  "99-Jul-04", "010-Jul-04", "2010-Jul-0", "2010-Jul-0 ",
>                  "12010-Jul-04", "-010-Jul-04", "+010-Jul-04", "2010-Jul-0a",
>                  "2010-Jua-04", "2010-Jal-04", "2010-aul-04", "2010-07-04",
>                  "2010-Jul-04",
>              ];
>
>          foreach (badDate; badDates)
>          {
>              assertThrown!DateTimeException(Date.fromISOString(badDate),
>                  "Accepts bad date: " ~ badDate);
>          }
>      }
>
>
> Andrei

Agreed :-) (sure)

<ot>
The type of things I had in mind is this kind of little tool funcs we sometimes
write to externalise a difficult point in a dark corner of a useful func (guess
you know what I mean). I often do that. That sort of hard point is usually not
easy just to spot, delimit, and define clearly, let alone naming it in such a
way that months later we have a chance to re-realise what the need for this
func was, how it was designed and how it is supposed to be used, exactly.
Factorising out also sometimes /increases/ complexity, forcing for instance to
add one or two more params, like ref'ed indices or such, which do not belong to
the model, and correspond to data which are "just here" when not externalised.
If, in addition to those problems, and in case of a core functionality (it's
often there that such issues raise) called millions of times, one cannot know
whether that tool func will be inlined or not... I now tend to just let things
in place.
</ot>

Denis
-- 
_________________
vita es estrany
spir.wikidot.com