Le 2010-07-13 ? 21:54, Walter Bright a ?crit :

> I received a litany of complaints about them.

I think you misinterpreted a little. Those complains were about all unit tests aborting at the first failing test. A better thing to do is to abort the current unit test on assert, then proceed with the next one. This way all unit tests are run, but only one error per test is reported.

Reporting more than one error per unit test adds meaningless garbage to the output output because the subsequent assertions are most of the time dependent on the first one succeeding (and thus do not indicate a different problem).

Personally, I'm not against a complete halt after the first assertion: it's simple and it forces you to fix the first bug it finds pronto. I understand that it does not please everyone though: some people just like to have the choice of which problem to fix first, or to get a general overview of the situation.

I think allowing one failed assertion per unit test is a good compromise. But allow more than that and you're lost in the noise of irrelevant assertions.

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

Yeah, this has been suggested before. I may just look into submitting a patch for it.

Sent from my iPhone

On Jul 13, 2010, at 7:28 PM, Michel Fortin <michel.fortin at michelf.com> wrote:

> Le 2010-07-13 ? 21:54, Walter Bright a ?crit :
> 
>> I received a litany of complaints about them.
> 
> I think you misinterpreted a little. Those complains were about all unit tests aborting at the first failing test. A better thing to do is to abort the current unit test on assert, then proceed with the next one. This way all unit tests are run, but only one error per test is reported.
> 
> Reporting more than one error per unit test adds meaningless garbage to the output output because the subsequent assertions are most of the time dependent on the first one succeeding (and thus do not indicate a different problem).
> 
> Personally, I'm not against a complete halt after the first assertion: it's simple and it forces you to fix the first bug it finds pronto. I understand that it does not please everyone though: some people just like to have the choice of which problem to fix first, or to get a general overview of the situation.
> 
> I think allowing one failed assertion per unit test is a good compromise. But allow more than that and you're lost in the noise of irrelevant assertions.
> 
> -- 
> Michel Fortin
> michel.fortin at michelf.com
> http://michelf.com/
> 
> 
> 
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos

Andrei Alexandrescu wrote:
>
> I'm not sure where the discussion leaves us, but at the end of the day we're in the following situation:
>
> 1. I just svn up'ed a minute ago and built dmd. It shows version 2.048.
>
> 2. This program:
>
> unittest
> {
>     int x = printf("inside unittest\n");
>     assert(x == 100000);
> }
>
> void main(string args[])
> {
>     writeln("inside main");
> }
>
> prints this:
>
> inside unittest
> test(7): unittest failure
> inside main
>
> and exits with error code zero.
>
> Please advise.
>

I just tried it (on Windows and Ubuntu) and it does not print "inside main". Also, it needs an "import std.stdio;"

I suggest verifying that druntime/src/core/runtime.d contains the following code:

extern (C) __gshared unittest_errors = false;

extern (C) bool runModuleUnitTests()
{
    static if( __traits( compiles, backtrace ) )
    {
        static extern (C) void unittestSegvHandler( int signum,
siginfo_t* info, void* ptr )
        {
            static enum MAXFRAMES = 128;
            void*[MAXFRAMES]  callstack;
            int               numframes;

            numframes = backtrace( callstack, MAXFRAMES );
            backtrace_symbols_fd( callstack, numframes, 2 );
        }

        sigaction_t action = void;
        sigaction_t oldseg = void;
        sigaction_t oldbus = void;

        (cast(byte*) &action)[0 .. action.sizeof] = 0;
        sigfillset( &action.sa_mask ); // block other signals
        action.sa_flags = SA_SIGINFO | SA_RESETHAND;
        action.sa_sigaction = &unittestSegvHandler;
        sigaction( SIGSEGV, &action, &oldseg );
        sigaction( SIGBUS, &action, &oldbus );
        scope( exit )
        {
            sigaction( SIGSEGV, &oldseg, null );
            sigaction( SIGBUS, &oldbus, null );
        }
    }

    if( Runtime.sm_moduleUnitTester is null )
    {
        foreach( m; ModuleInfo )
        {
            if( m )
            {
                auto fp = m.unitTest;

                if( fp )
                    fp();
            }
        }
        return !unittest_errors;
    }
    return Runtime.sm_moduleUnitTester();
}

and druntime/src/core/exception.d contains the following code:

extern (C) extern __gshared bool unittest_errors;

extern (C) void onUnittestErrorMsg( string file, size_t line, string msg )
{
    static char[] intToString( char[] buf, uint val )
    {
        assert( buf.length > 9 );
        auto p = buf.ptr + buf.length;

        do
        {
            *--p = cast(char)(val % 10 + '0');
        } while( val /= 10 );

        return buf[p - buf.ptr .. $];
    }

    static struct Console
    {
        Console opCall( in char[] val )
        {
            version( Windows )
            {
                uint count = void;
                WriteFile( GetStdHandle( 0xfffffff5 ), val.ptr,
val.length, &count, null );
            }
            else version( Posix )
            {
                write( 2, val.ptr, val.length );
            }
            return this;
        }


        Console opCall( uint val )
        {
            char[10] tmp = void;
            return opCall( intToString( tmp, val ) );
        }
    }

    static __gshared Console console;

    unittest_errors = true;
    console( file )( "(" )( line )( "): " )( msg )( "\n" );
}

If they do, then make sure you really did delete the druntime lib and rebuild it and you're not linking with an old one.

Walter's change fixed the issue where non-throwing asserts weren't triggering a unittest failure, but it also reverted my change to trap unhandled exceptions, set a "fail" flag, and continue with the next test.  I'd fix this, but what I'm confused about is why the support-non-throwing-asserts change was re-added in the first place.  So instead I'd like to ask what the eventual/intended/desired/whatever behavior is for unit tests so I can work towards that.  I'd like throwing asserts and for each unittest in a module to be run separately so recovery from an AssertError (which I'd like for unit tests) is at the granularity of the unittest block, not the module.  The best I can do without a compiler change is module-level granularity like we have now, but asserts could all throw again, etc.  Should this be an intermediate step?  Should we keep non-throwing asserts?  Halp plzz.

Sean Kelly wrote:
> Walter's change fixed the issue where non-throwing asserts weren't triggering a unittest failure, but it also reverted my change to trap unhandled exceptions, set a "fail" flag, and continue with the next test.  I'd fix this, but what I'm confused about is why the support-non-throwing-asserts change was re-added in the first place.  So instead I'd like to ask what the eventual/intended/desired/whatever behavior is for unit tests so I can work towards that.  I'd like throwing asserts and for each unittest in a module to be run separately so recovery from an AssertError (which I'd like for unit tests) is at the granularity of the unittest block, not the module.  The best I can do without a compiler change is module-level granularity like we have now, but asserts could all throw again, etc.  Should this be an intermediate step?  Should we keep non-throwing asserts?  Halp plzz.
>
> 

Perhaps what will help is examining the output of the compiler. For the program:

import std.c.stdio;

unittest
{
    int x = printf("inside unittest\n");
    assert(x == 100000);
}

void main(string args[])
{
    printf("inside main\n");
}

The output is:

_TEXT    segment dword use32 public 'CODE'    ;size is 0
_TEXT    ends
_DATA    segment para use32 public 'DATA'    ;size is 64
_DATA    ends
CONST    segment para use32 public 'CONST'    ;size is 0
CONST    ends
_BSS    segment para use32 public 'BSS'    ;size is 0
_BSS    ends
FLAT    group
    extrn    _D3foo11__unittest1FZv
includelib phobos.lib
    extrn    _main
    extrn    __acrtused_con
    extrn    __Dmain
    extrn    __D3foo9__modtestFZv
FMB    segment dword use32 public 'DATA'    ;size is 0
FMB    ends
FM    segment dword use32 public 'DATA'    ;size is 4
FM    ends
FME    segment dword use32 public 'DATA'    ;size is 0
FME    ends
    extrn    _D3foo15__unittest_failFiZv
    extrn    __d_unittestm
    extrn    _printf

    public    _D3foo12__ModuleInfoZ
_D3foo11__unittest1FZv    COMDAT flags=x0 attr=x0 align=x0
__Dmain    COMDAT flags=x0 attr=x0 align=x0
__D3foo9__modtestFZv    COMDAT flags=x0 attr=x0 align=x0
_D3foo15__unittest_failFiZv    COMDAT flags=x0 attr=x0 align=x0

_TEXT    segment
    assume    CS:_TEXT
_TEXT    ends
_DATA    segment
    db    069h,06eh,073h,069h,064h,065h,020h,075h    ;inside u
    db    06eh,069h,074h,074h,065h,073h,074h,00ah    ;nittest.
    db    000h,000h,000h,000h,069h,06eh,073h,069h    ;....insi
    db    064h,065h,020h,06dh,061h,069h,06eh,00ah    ;de main.
    db    000h,000h,000h,000h,000h,000h,000h,000h    ;........
    db    000h,000h,000h,000h,000h,000h,000h,000h    ;........
_D3foo12__ModuleInfoZ:
    db    004h,002h,000h,0ffffff80h,000h,000h,000h,000h    ;........
    dd    offset FLAT:__D3foo9__modtestFZv
    db    066h,06fh,06fh,000h    ;foo.
_DATA    ends
CONST    segment
CONST    ends
_BSS    segment
_BSS    ends
FMB    segment
FMB    ends
FM    segment
    dd    offset FLAT:_D3foo12__ModuleInfoZ
FM    ends
FME    segment
FME    ends
_D3foo11__unittest1FZv    comdat
    assume    CS:_D3foo11__unittest1FZv
L0:     mov    EAX,offset FLAT:_DATA
        push    EAX
        call    near ptr _printf
        cmp    EAX,0186A0h
        je    L1C
        mov    EAX,7
        call    near ptr _D3foo15__unittest_failFiZv
L1C:        add    ESP,4
        ret
_D3foo11__unittest1FZv    ends
__Dmain    comdat
    assume    CS:__Dmain
L0:     mov    EAX,offset FLAT:_DATA[014h]
        push    EAX
        call    near ptr _printf
        xor    EAX,EAX
        add    ESP,4
        ret
__Dmain    ends
__D3foo9__modtestFZv    comdat
    assume    CS:__D3foo9__modtestFZv
L0:     call    near ptr _D3foo11__unittest1FZv
        ret
__D3foo9__modtestFZv    ends
_D3foo15__unittest_failFiZv    comdat
    assume    CS:_D3foo15__unittest_failFiZv
L0:     enter    4,0
        push    EAX
        mov    ECX,offset FLAT:_D3foo12__ModuleInfoZ
        push    ECX
        call    near ptr __d_unittestm
        add    ESP,8
        leave
        ret
_D3foo15__unittest_failFiZv    ends
    end

What is happening is that the compiler inserts the address of __D3foo9__modtestFZv into the ModuleInfo record. The druntime calls this function. This function, in turn, runs the unittest code for that module. When a unittest fails, the function __d_unittestm(&__ModuleInfoZ, __LINE__) is called. This function is in the druntime.

Note that the compiler generated code does not throw any exceptions. What happens when __d_unittestm() is called is ENTIRELY up to druntime, which can be:

1. nothing
2. print a message
3. abort
4. throw an exception chosen by druntime
5. whatever druntime wants to

The important thing to note is that this is up to druntime, not the compiler. The part that is up to the compiler is the granularity of the unittests, which is set at the module, not the individual unittest blocks.

Thanks, Sean, that was the clue I needed.  I reran the unittests, compiling against an earlier version of druntime, and it turns out that the failing assert is in line 1255 of container.d.  That's the postblit constructor of Array.Payload, which is defined simply as

  this(this)
  {
      assert(0);
  }

So, firstly:  Why is the constructor defined like this?  If it is meant to disable copying of Array.Payload, isn't that what @disable is for?

Moving on, I did try to @disable the postblit constructor in order to track down why it's called.  Then the error changes to

  std/exception.d(407): Error: struct
  std.container.Array!(uint).Array.Payload is not copyable
  because it is annotated with @disable

Line 407 of exception.d is inside pointsTo(), and contains

  foreach (i, subobj; source.tupleof)

typeof(source) is Tuple!(Payload,"_backend",ulong,"_length"), and it
would seem that taking .tupleof on this type attempts a copy of Payload.

-Lars



On Tue, 2010-07-13 at 07:23 -0700, Sean Kelly wrote:
> If a unittest throws an exception the test is marked as failed and you'll see an errorlevel 1 after the tests complete. I didn't report the exception because unittest output is often parsed to accumulate results, but this could easily be changed.
> 
> Sent from my iPhone
> 
> On Jul 12, 2010, at 6:55 AM, Lars Tandle Kyllingstad <lars at kyllingen.net> wrote:
> 
> > When running 'make unittest' on the latest revision of Phobos, it just fails on/after std.container, without any sensible error message:
> > 
> > Testing generated/posix/debug/unittest/std/container
> > make[1]: *** [generated/posix/debug/unittest/std/container] Error 1
> > make: *** [unittest] Error 2
> > 
> > Anyone else seeing this?
> > 
> > -Lars
> > 
> > _______________________________________________
> > phobos mailing list
> > phobos at puremagic.com
> > http://lists.puremagic.com/mailman/listinfo/phobos
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos

On Jul 13, 2010, at 11:06 PM, Walter Bright wrote:

> 
> 
> Sean Kelly wrote:
>> Walter's change fixed the issue where non-throwing asserts weren't triggering a unittest failure, but it also reverted my change to trap unhandled exceptions, set a "fail" flag, and continue with the next test.  I'd fix this, but what I'm confused about is why the support-non-throwing-asserts change was re-added in the first place.  So instead I'd like to ask what the eventual/intended/desired/whatever behavior is for unit tests so I can work towards that.  I'd like throwing asserts and for each unittest in a module to be run separately so recovery from an AssertError (which I'd like for unit tests) is at the granularity of the unittest block, not the module.  The best I can do without a compiler change is module-level granularity like we have now, but asserts could all throw again, etc.  Should this be an intermediate step?  Should we keep non-throwing asserts?  Halp plzz.
>> 
>> 
> 
> Perhaps what will help is examining the output of the compiler. For the program:

...

> What is happening is that the compiler inserts the address of __D3foo9__modtestFZv into the ModuleInfo record. The druntime calls this function. This function, in turn, runs the unittest code for that module. When a unittest fails, the function __d_unittestm(&__ModuleInfoZ, __LINE__) is called. This function is in the druntime.
> 
> Note that the compiler generated code does not throw any exceptions. What happens when __d_unittestm() is called is ENTIRELY up to druntime, which can be:
> 
> 1. nothing
> 2. print a message
> 3. abort
> 4. throw an exception chosen by druntime
> 5. whatever druntime wants to
> 
> The important thing to note is that this is up to druntime, not the compiler. The part that is up to the compiler is the granularity of the unittests, which is set at the module, not the individual unittest blocks.

Right.  I was proposing that ModuleInfo have a list of unittest modules instead of a reference to a single function which calls them each in turn (ie. finer granularity).  I've already taken care of the assert behavior on my local machine, but I don't want to commit the change unless it's what people want.  I thought that since you reverted my unittest changes yesterday that non-throwing asserts might actually be desired.

Sean Kelly wrote:
>
>
> Right.  I was proposing that ModuleInfo have a list of unittest modules instead of a reference to a single function which calls them each in turn (ie. finer granularity).  I've already taken care of the assert behavior on my local machine, but I don't want to commit the change unless it's what people want.  I thought that since you reverted my unittest changes yesterday that non-throwing asserts might actually be desired.
> _______________________________________________
>
> 

There isn't a right answer. The overwhelming feedback I received prior to 2.44 was that all the unittests should run, and then the program should exit if any of them failed, and that's what I implemented. I think that also makes a lot of sense - if there are a lot, simply redirect them to a file and then fix the ones you want to, all in one go. I don't think it is a big problem to ignore messages from tests that are redundant.

The other tenable option is to allow at most one unittest failure per module. Or one per unittest block. Either way, we'll get complaints. I suggest we go with the "run them all".

On Wednesday, July 14, 2010 14:43:26 Walter Bright wrote:
> Sean Kelly wrote:
> > Right.  I was proposing that ModuleInfo have a list of unittest modules instead of a reference to a single function which calls them each in turn (ie. finer granularity).  I've already taken care of the assert behavior on my local machine, but I don't want to commit the change unless it's what people want.  I thought that since you reverted my unittest changes yesterday that non-throwing asserts might actually be desired. _______________________________________________
> 
> There isn't a right answer. The overwhelming feedback I received prior to 2.44 was that all the unittests should run, and then the program should exit if any of them failed, and that's what I implemented. I think that also makes a lot of sense - if there are a lot, simply redirect them to a file and then fix the ones you want to, all in one go. I don't think it is a big problem to ignore messages from tests that are redundant.
> 
> The other tenable option is to allow at most one unittest failure per
> module. Or one per unittest block. Either way, we'll get complaints. I
> suggest we go with the "run them all".
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos

I would have thought that when people were saying "let all the unit tests run," they were referring to each unittest block rather than each assertion within the unittest blocks. If frameworks like JUnit, each unit test (in JUnit's case, each unit test is a function) will be run and sucess or failure will be indicated for each test with failure meaning that _one_ of the assertions failed - a test does not continue if an assertion fails.

I think that it makes sense to treat a unittest block as a single unit test and treat a single assertion failure within a unittest block as failure for that test but have that have no effect on whether the other unittest blocks are run. If we ever moved towards named unit tests, this makes even more sense. e.g.

unittest(equality_test)
{
  ... //whatever tests we do for equality for whatever this test is for
}

Treating a unittest block as a single unit test makes a lot of sense IMO, and it lends itself quite well to good granularity. If you want to test a bunch of stuff in a single test and have it fail on a single assertion, then put it all in one unittest block. If you want to test a bunch of stuff individually without each test affecting the others, then put each test in a separate in a separate unittest block.

That would be the typical way that unit test tools work (in my experience at least), and I think that's more or less what most people are looking for. It also makes it work better to move towards named unittests in the future like a number of people have requested. It would even make it possible to make it so that you could simply print out success or failure for each test if you wanted to do set things up that way.

Having everything halt because of a single assertion failure makes for slow testing. On the other hand, never halting on assertion failures makes for a lot of cruft in your results. Treating each unittest block as a single unittest which succeeds or fails, stopping a particular block when one of its assertions fails and then moving on to the next block seems by far the best solution to me.

- Jonathan M Davis

On 07/14/2010 04:43 PM, Walter Bright wrote:
> There isn't a right answer. The overwhelming feedback I received prior to 2.44 was that all the unittests should run, and then the program should exit if any of them failed, and that's what I implemented. I think that also makes a lot of sense - if there are a lot, simply redirect them to a file and then fix the ones you want to, all in one go. I don't think it is a big problem to ignore messages from tests that are redundant.
>
> The other tenable option is to allow at most one unittest failure per module. Or one per unittest block. Either way, we'll get complaints. I suggest we go with the "run them all".

Sounds good to me. I got things to "fail successfully" now. Never been so happy about something failing.

Andrei