On Jul 15, 2010, at 5:04 PM, Andrei Alexandrescu wrote:

> On 07/15/2010 06:59 PM, Benjamin Shropshire wrote:
>> Beside, I don't now nor will I ever care about ANYTHING after the first failed assert in any given block.
> 
> Walter, it would be great if you could substantiate some concrete examples by pasting from your tests. Thanks!

I've been thinking about this, and I realized that I'd really like different behaviors for different types of tests.  For testing stateless code, the most convenient thing would be Walter's behavior.  For stateful code, throwing on error is preferable, since once the state is invalid further testing is useless.  I wondered whether one could argue that with testing stateful code, when something fails an exception would likely be thrown from inside the object and abort the test anyway, but I think this isn't true.  For boundary testing, I'd typically want to ensure that AssertErrors were thrown when a contract was violated, etc, so I'm still likely to do something like:

    unittest {
        auto o = MyObject;
        assert(throws!AssertError(o.divide(1,0)));
        ...
    }

It wouldn't be terribly onerous to me to break stateless tests into multiple unittest blocks, but it's probably more readable to simply have:

    unittest {
        verify(sort("bac") == "abc");
        verify(sort("bba") == "abb");
        ...
    }

So I'm really ambivalent beyond thinking that assert should always throw, since this should never result in undesirable behavior (reporting a test failure then a following test segfaulting).

Le 2010-07-15 ? 22:52, Sean Kelly a ?crit :

> It wouldn't be terribly onerous to me to break stateless tests into multiple unittest blocks, but it's probably more readable to simply have:
> 
>    unittest {
>        verify(sort("bac") == "abc");
>        verify(sort("bba") == "abb");
>        ...
>    }

Perhaps the syntax for one-statement unit tests could be relaxed to not require the braces. This:

	unittest { assert(sort("bac") == "abc"); }
	unittest { assert(sort("bba") == "abc"); }

could become this:

	unittest assert(sort("bac") == "abc");
	unittest assert(sort("bba") == "abc");

And it's suddenly more elegant, more readable and easier to write. No need for a new kind of assert (your 'verify') or a non-throwing mode for asserts. It reminds me of what happened to enum.

But of course, that's probably more work to do in the compiler.

-- 
Michel Fortin
michel.fortin at michelf.com
http://michelf.com/

On Thu, 2010-07-15 at 15:00 -0700, Walter Bright wrote:
> 
> Andrei Alexandrescu wrote:
> >
> > 4. DESIRED: assert is NOT hacked, any failing assert ends the current unittest, the failure message is printed, execution continues with the next unittest, program ends with error code if at least one assert failed, everybody's happy.
> >
> 
> Everybody's happy except the guys like me who want to run all the unit tests in one go.

But that is trivial to implement as a library feature!

The biggest problem, as I see it, is that assert(), which is a *built-in* feature of the language, now has radically different semantics depending on where and when it is called.

Besides, the behaviour Andrei and the rest of us want can't be implemented in a library.

-Lars

Michel Fortin, el 15 de julio a las 18:32 me escribiste:
> 
> Le 2010-07-15 ? 18:00, Walter Bright a ?crit :
> 
> >> 4. DESIRED: assert is NOT hacked, any failing assert ends the current unittest, the failure message is printed, execution continues with the next unittest, program ends with error code if at least one assert failed, everybody's happy.
> > 
> > Everybody's happy except the guys like me who want to run all the unit tests in one go.
> 
> I think this is the root of the misunderstanding. Either you consider each assert grouped in a unittest blocks to be a unit test in itself; or you consider each unittest block to be a test with multiple possible points of failure across the path of the test.
> 
> The former is best illustrated this way:
> 
>       unittest {
>               assert(sqrt(16) == 4);
>               assert(sqrt(4) == 2);
>               assert(sqrt(1) == 1);
>       }
> 
> Knowing which assertion fails in the given test might help to solve the problem, so it's legitimate in my view.
> 
> And here's an illustration of a second test modelling a sequence of events. If one of these test fails, all others will fail too because they depend on each other:
> 
>       unittest {
>               S s = new S;
>               s.giveMeAName();
>               assert(s.fileName);
>               s.value = 1;
>               assert(s.value == 1);
>               assert(s.needsToBeSaved);
>               s.save();
>               assert(!s.needsToBeSaved);
>               s.value = 2;
>               assert(s.value == 2);
>               assert(s.needsToBeSaved);
>               s.revert();
>               assert(s.value == 1);
>               assert(!s.needsToBeSaved);
>               ...
>       }
> 
> So here you only need to know about the first error, and the rest is garbage.
> 
> It also highlights another problem about the current behaviour: what happens if one of the functions called in this test throws an exception? My guess is that it would abort unit tests for the whole module... I don't think anyone wants that.

This is why unit testing frameworks usually have 2 type of checks, one
that exists the current unit test if it fails and one that doesn't.
I think all we need is option 4) and something like:
import std.test;

        unittest {
                check(sqrt(16) == 4);
                check(sqrt(4) == 2);
                check(sqrt(1) == 1);
        }

std.test could have other convenience functions, like something like:
checkOp!"=="(sqrt(16), 4);
assertOp!">"(pow(x, y), 0);

Which prints (when failed because sqrt(16) returned 5 and pow(x, y)
returned -2) something like:
Check failed: 5 == 4
Assert failed: -2 > 0

It's a little uglier to write but provides more useful information.


-- 
Leandro Lucarella (AKA luca)                     http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145  104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
Todo caminante merece una palangana con agua caliente. Del reposos de
sus pies depende la longitud del camino.
        -- Ricardo Vaporeso. Valle de la Muerte, Junio de 1917.

Andrei Alexandrescu, el 15 de julio a las 18:55 me escribiste:
> On 07/15/2010 06:00 PM, Sean Kelly wrote:
> >On Jul 15, 2010, at 3:43 PM, Andrei Alexandrescu wrote:
> >>
> >>I think there's a disadvantage there. FWIW changing the semantics
> >>of assert that way will translate into a disincentive to use it
> >>inside unittests ("Hmm, I better use enforce() here because
> >>assert() is just weird.")
> >
> >Is there a disadvantage in providing a separate routine that reports and doesn't throw?  I know it's another global symbol (assuming it's in object.di), but...
> 
> Good question. expect() comes to mind.

I prefer check(), and boost users will feel at home :)

warn() can be nice to have too, but I think they should not be global.
std.test comes to mind.

See [1]:

Level   Test log content                       Errors          Test
                                               counter       execution
======================================================================
WARN    warning in <test case name>:           not affected  continues
        condition <assertion description>
        is not satisfied
----------------------------------------------------------------------
CHECK   error in <test case name>: test        increased     continues
        <assertion description> failed
----------------------------------------------------------------------
REQUIRE fatal error in <test case name>:       increased     aborts
        critical test <assertion description>
        failed

(replace require with assert... well, and add the counter =P)


[1] http://www.boost.org/doc/libs/1_42_0/libs/test/doc/html/utf/testing-tools.html
    More helper functions for unit tests:
    http://www.boost.org/doc/libs/1_42_0/libs/test/doc/html/utf/testing-tools/reference.html

-- 
Leandro Lucarella (AKA luca)                     http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145  104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
they wrap me up in the back of the trunk
packed with foam and blind drunk
they won't ever take me alive
cause they all drive killer cars

Sean Kelly, el 15 de julio a las 19:52 me escribiste:
> On Jul 15, 2010, at 5:04 PM, Andrei Alexandrescu wrote:
> 
> > On 07/15/2010 06:59 PM, Benjamin Shropshire wrote:
> >> Beside, I don't now nor will I ever care about ANYTHING after the first failed assert in any given block.
> > 
> > Walter, it would be great if you could substantiate some concrete examples by pasting from your tests. Thanks!
> 
> I've been thinking about this, and I realized that I'd really like different behaviors for different types of tests.  For testing stateless code, the most convenient thing would be Walter's behavior.  For stateful code, throwing on error is preferable, since once the state is invalid further testing is useless.  I wondered whether one could argue that with testing stateful code, when something fails an exception would likely be thrown from inside the object and abort the test anyway, but I think this isn't true.  For boundary testing, I'd typically want to ensure that AssertErrors were thrown when a contract was violated, etc, so I'm still likely to do something like:
> 
>     unittest {
>         auto o = MyObject;
>         assert(throws!AssertError(o.divide(1,0)));
>         ...
>     }
> 
> It wouldn't be terribly onerous to me to break stateless tests into multiple unittest blocks, but it's probably more readable to simply have:
> 
>     unittest {
>         verify(sort("bac") == "abc");
>         verify(sort("bba") == "abb");
>         ...
>     }

check() check() check()! =)

-- 
Leandro Lucarella (AKA luca)                     http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145  104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
No existir?a el sonido del mar si faltara en la vida oreja y caracol.
        -- Ricardo Vaporeso. Cosqu?n, 1908.

----- Original Message ----

> From: Walter Bright <walter at digitalmars.com>
> To: Discuss the phobos library for D <phobos at puremagic.com>
> Sent: Thu, July 15, 2010 7:11:28 PM
> Subject: Re: [phobos] Silent failure of std.container unittests
> 
> 
> 
> Sean Kelly wrote:
> > On Jul 15, 2010, at 3:00 PM, Walter Bright  wrote:
> > 
> >>> NOT YET:
> >>>
> >>>  4. DESIRED: assert is NOT hacked, any failing assert ends the current
>unittest,  the failure message is printed, execution continues with the next unittest,  program ends with error code if at least one assert failed, everybody's  happy.
> >>>
> >>> 
> >>  Everybody's happy except the guys like me who want to run all the unit
>tests in  one go.
> >> 
> >
> > All the unit tests or all the  asserts?
> 
> All the asserts.
> 
> >   All the unit tests are run  with this approach:
> >
> >     module  A;
> >
> >     unittest {
> >          assert(false, "test 1a failed");
> >          assert(false, "test 1b failed");
> >     }
> >
> >      unittest {
> >         assert(false, "test 2  failed");
> >     }
> >
> > This should  print:
> >
> >     test 1a failed
> >     test 2  failed
> >
> > I'm not sure I understand the problem of breaking unit  tests into a
>collection of atomic blocks.
> 
> Extra work.

But then assert doesn't do what assert is supposed to do.

The problem I see here is, inside unit tests, assert means two different things:

1. Display an error if a value isn't what it's supposed to be.
2. Make sure my code is in a sane state before it goes any further.

During unit tests, assert is used for 1 mostly, but many use it for 1 and 2 combined.

A sample from dcollections, with each assert marked as whether it is 1 and/or 2:

    static if(doUnittest) unittest
    {
        auto hs = new HashSet;
        hs.add([1, 2, 3, 4, 5]);
        auto r = hs[];
        assert(rangeEqual(r, cast(V[])[1, 2, 3, 4, 5])); // 1, 2
        assert(r.front == hs.begin.front); // 1
        assert(r.back != r.front); // 1
        auto oldfront = r.front;
        auto oldback = r.back;
        r.popFront();
        r.popBack();
        assert(r.front != r.back); // 1, 2
        assert(r.front != oldfront); // 1, 2
        assert(r.back != oldback); // 1, 2

        auto b = r.begin;
        assert(!b.empty); // 1
        assert(b.front == r.front); // 1
        auto e = r.end;
        assert(e.empty); // 1
    }


I'm almost thinking we need to dump assert as "the way to fail unit tests" and go with something more configurable.  Leave assert the way it is.

Then, what we need is two functions, one that "exits the current unit test", and one that "just prints out a failure".

I'd suggest require() and check().

Note that we do not need the special traits of assert, it should only be used during unit tests.

FWIW, the current mode of unit testing (assert doesn't throw) didn't hinder much the testing of dcollections.

-Steve

On 07/16/2010 08:31 AM, Leandro Lucarella wrote:
> Andrei Alexandrescu, el 15 de julio a las 18:55 me escribiste:
>> Good question. expect() comes to mind.
>
> I prefer check(), and boost users will feel at home :)

Yah but with expect() Google Test users will feel at home :o).

http://code.google.com/p/googletest/wiki/GoogleTestPrimer

Anyway, I think it's reasonable to say that all other things being equal, messing with the meaning of assert() depending on where it occurs is a strong argument against the current stance.

Walter, again, you may want to think of pleasing the core and potential contributors to Phobos in particular and D in general as those that currently take the brunt of unittest usage. As one of them, I can say that I'd much enjoy it if you kindly agreed to have assert() end the current unittest.


Thanks,

Andrei

I think this is the right line to pursue.  The user can select the behavior of a failure then.


----- Original Message ----
> From: Leandro Lucarella <luca at llucax.com.ar>
> To: phobos at puremagic.com
> Sent: Fri, July 16, 2010 6:31:05 AM
> Subject: Re: [phobos] Silent failure of std.container unittests
> 
> Andrei Alexandrescu, el 15 de julio a las 18:55 me escribiste:
> > On  07/15/2010 06:00 PM, Sean Kelly wrote:
> > >On Jul 15, 2010, at 3:43 PM,  Andrei Alexandrescu wrote:
> > >>
> > >>I think there's a  disadvantage there. FWIW changing the semantics
> > >>of assert that  way will translate into a disincentive to use it
> > >>inside  unittests ("Hmm, I better use enforce() here because
> > >>assert() is  just weird.")
> > >
> > >Is there a disadvantage in providing a  separate routine that reports and doesn't throw?  I know it's  another global symbol (assuming it's in object.di), but...
> > 
> > Good question. expect() comes to mind.
> 
> I prefer check(), and  boost users will feel at home :)
> 
> warn() can be nice to have too, but I  think they should not be global.
> std.test comes to mind.
> 
> See  [1]:
> 
> Level   Test log content                        Errors           Test
>                                                  counter        execution
> ======================================================================
> WARN     warning in <test case name>:            not affected  continues
>         condition  <assertion description>
>         is not  satisfied
> ----------------------------------------------------------------------
> CHECK    error in <test case name>: test         increased     continues
>         <assertion  description>  failed
> ----------------------------------------------------------------------
> REQUIRE  fatal error in <test case name>:       increased      aborts
>         critical test <assertion  description>
>         failed
> 
> (replace require  with assert... well, and add the counter =P)
> 
> 
> [1]
>http://www.boost.org/doc/libs/1_42_0/libs/test/doc/html/utf/testing-tools.html
>      More helper functions for unit tests:
> 
>http://www.boost.org/doc/libs/1_42_0/libs/test/doc/html/utf/testing-tools/reference.html
>
> 
> -- 
> Leandro Lucarella (AKA luca)                       http://llucax.com.ar/
> ----------------------------------------------------------------------
> GPG  Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145  104C 949E BFB6 5F5A  8D05)
> ----------------------------------------------------------------------
> they  wrap me up in the back of the trunk
> packed with foam and blind drunk
> they  won't ever take me alive
> cause they all drive killer  cars
> _______________________________________________
> phobos mailing  list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos
> 

On Jul 16, 2010, at 8:09 AM, Andrei Alexandrescu wrote:
> 
> Walter, again, you may want to think of pleasing the core and potential contributors to Phobos in particular and D in general as those that currently take the brunt of unittest usage. As one of them, I can say that I'd much enjoy it if you kindly agreed to have assert() end the current unittest.

As of my commit yesterday, this is the behavior.  We really need to increase the granularity of tests though.