Today's checkout of phobos breaks the unit tests on Windows:

..\dmd -O -release -nofloat -d -L/co -unittest unittest.d std\math.d
std\stdio.d std\dateparse.d std
\date.d std\uni.d std\string.d  std\base64.d std\md5.d std\xml.d
std\bigint.d std\regexp.d  std\comp
iler.d std\cpuid.d std\format.d std\demangle.d  std\path.d std\file.d
std\outbuffer.d std\utf.d std\
uri.d  std\ctype.d std\random.d std\mmfile.d  std\algorithm.d
std\array.d std\numeric.d std\function
al.d  std\range.d std\stdiobase.d std\concurrency.d  std\metastrings.d
std\contracts.d std\getopt.d
 std\signals.d std\typetuple.d std\traits.d std\bind.d  std\bitmanip.d
std\typecons.d  std\boxer.d
std\complex.d  std\exception.d  std\process.d  std\system.d
std\encoding.d std\variant.d  std\strea
m.d std\socket.d std\socketstream.d  std\perf.d std\container.d
std\conv.d  std\zip.d std\cstream.d
std\loader.d  std\__fileinit.d  std\datebase.d  std\regex.d
std\stdarg.d  std\stdint.d  std\json.d
 std\gregorian.d  std\internal\math\biguintcore.d
std\internal\math\biguintnoasm.d std\internal\mat
h\biguintx86.d  crc32.d  std\c\process.d  std\c\stdarg.d
std\c\stddef.d  std\c\stdlib.d  std\c\stri
ng.d  std\c\time.d  std\c\math.d  std\c\windows\com.d
std\c\windows\stat.d  std\c\windows\windows.d
  std\c\windows\winsock.d  std\windows\charset.d
std\windows\iunknown.d  std\windows\registry.d  st
d\windows\syserror.d phobos.lib
std.contracts has been scheduled for deprecation. Please use
std.exception instead.
 --- std.numeric(543) CustomFloat broken test ---
Warning: AutoImplement!(C_6) ignored variadic arguments to the
constructor C_6(...)
 --- std.socket(314) broken test ---
 --- std.regex(3348) broken test ---

unittest
std.xml(379): unittest failure
std.complex(514): unittest failure
std.complex(519): unittest failure
The syntax of the command is incorrect.
std.exception.ErrnoException at std\process.d(327):  (No error)

--- errorlevel 1

Yup, that has a lot to do with the changes to assert semantics. Prior to those, it was embarrassing for people to check in stuff that broke the build. Now, failing unittests only issue one like to stdout - who cares?

Andrei

Walter Bright wrote:
> Today's checkout of phobos breaks the unit tests on Windows:
> 
> ..\dmd -O -release -nofloat -d -L/co -unittest unittest.d std\math.d
> std\stdio.d std\dateparse.d std
> \date.d std\uni.d std\string.d  std\base64.d std\md5.d std\xml.d
> std\bigint.d std\regexp.d  std\comp
> iler.d std\cpuid.d std\format.d std\demangle.d  std\path.d std\file.d
> std\outbuffer.d std\utf.d std\
> uri.d  std\ctype.d std\random.d std\mmfile.d  std\algorithm.d
> std\array.d std\numeric.d std\function
> al.d  std\range.d std\stdiobase.d std\concurrency.d  std\metastrings.d
> std\contracts.d std\getopt.d
> std\signals.d std\typetuple.d std\traits.d std\bind.d  std\bitmanip.d
> std\typecons.d  std\boxer.d
> std\complex.d  std\exception.d  std\process.d  std\system.d
> std\encoding.d std\variant.d  std\strea
> m.d std\socket.d std\socketstream.d  std\perf.d std\container.d
> std\conv.d  std\zip.d std\cstream.d
> std\loader.d  std\__fileinit.d  std\datebase.d  std\regex.d
> std\stdarg.d  std\stdint.d  std\json.d
> std\gregorian.d  std\internal\math\biguintcore.d
> std\internal\math\biguintnoasm.d std\internal\mat
> h\biguintx86.d  crc32.d  std\c\process.d  std\c\stdarg.d
> std\c\stddef.d  std\c\stdlib.d  std\c\stri
> ng.d  std\c\time.d  std\c\math.d  std\c\windows\com.d
> std\c\windows\stat.d  std\c\windows\windows.d
>  std\c\windows\winsock.d  std\windows\charset.d  std\windows\iunknown.d
> std\windows\registry.d  st
> d\windows\syserror.d phobos.lib
> std.contracts has been scheduled for deprecation. Please use
> std.exception instead.
> --- std.numeric(543) CustomFloat broken test ---
> Warning: AutoImplement!(C_6) ignored variadic arguments to the constructor C_6(...)
> --- std.socket(314) broken test ---
> --- std.regex(3348) broken test ---
> 
> unittest
> std.xml(379): unittest failure
> std.complex(514): unittest failure
> std.complex(519): unittest failure
> The syntax of the command is incorrect.
> std.exception.ErrnoException at std\process.d(327):  (No error)
> 
> --- errorlevel 1
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos

The failing unittests still exit with an errorlevel. The change was to not exit immediately on the assert error, but to remember that it failed, run all the unittests, then exit with errorlevel set to 1 if any of them had failed.

In other words, the unit tests are not being run by the people checking in phobos changes.

I also find the ErrnoException saying "(No error)" is very strange.

BTW, the std.complex failures are possibly due to changes in toString().

Andrei Alexandrescu wrote:
> Yup, that has a lot to do with the changes to assert semantics. Prior to those, it was embarrassing for people to check in stuff that broke the build. Now, failing unittests only issue one like to stdout - who cares?
>
> Andrei
>
> Walter Bright wrote:
>>
>> unittest
>> std.xml(379): unittest failure
>> std.complex(514): unittest failure
>> std.complex(519): unittest failure
>> The syntax of the command is incorrect.
>> std.exception.ErrnoException at std\process.d(327):  (No error)
>>
>> --- errorlevel 1
>>
>

On Mon, 2010-07-05 at 12:44 -0700, Walter Bright wrote:
> BTW, the std.complex failures are possibly due to changes in toString().

The last change to std.complex was over a month ago, and this bug is completely new, so that's not it.  Rather, I suspect the unittests fail because of Andrei's changes to std.array.Appender (changeset 1713), or possibly the changes to std.format (changeset 1719).  I'll look into it tomorrow.

-Lars

On 07/05/2010 02:44 PM, Walter Bright wrote:
> The failing unittests still exit with an errorlevel. The change was to not exit immediately on the assert error, but to remember that it failed, run all the unittests, then exit with errorlevel set to 1 if any of them had failed.

I'm glad we see eye to eye on that! In fact there is a bug in your implementation of Linux - it doesn't do as you mention above. Look at this:

$ generated/posix/debug/unittest/std/xml
std.xml(379): unittest failure
$ echo $?
0
$ _

So something somewhere makes the program return success even though it fails.

> I also find the ErrnoException saying "(No error)" is very strange.

errnoEnforce() enforces its argument and assumes that failure will set errno. Somewhere there's an errnoEnforce() against an exception that does NOT set errno.

> BTW, the std.complex failures are possibly due to changes in toString().

I figured what's happening.

The problem stems again from the lack of a default constructor (or the inability to disable it effectively). Previously, Appender was passed around by reference, which made it possible for it itself to hold a null-initialized payload. But that was a hack - one should be able to pass it by value. Once I changed Appender to support pass-by-value, it started having that problem we discussed in conjunction with built-in hashtables: it has reference semantics if not null, and crappy semantics if null.


Andrei

On Jul 5, 2010, at 9:13 PM, Andrei Alexandrescu wrote:

> On 07/05/2010 02:44 PM, Walter Bright wrote:
>> The failing unittests still exit with an errorlevel. The change was to not exit immediately on the assert error, but to remember that it failed, run all the unittests, then exit with errorlevel set to 1 if any of them had failed.
> 
> I'm glad we see eye to eye on that! In fact there is a bug in your implementation of Linux - it doesn't do as you mention above. Look at this:
> 
> $ generated/posix/debug/unittest/std/xml
> std.xml(379): unittest failure
> $ echo $?
> 0
> $ _
> 
> So something somewhere makes the program return success even though it fails.

That's probably my fault.  The unittest handler returns a boolean to indicate whether the app should run when tests complete.  I believe Walter repurposed it to have the unittest handler indicate success/failure and I changed it back.  We really need to be able to do both--set a result and allow only tests to be run.  It may be worth adding a Runtime.result property that the app returns on exit, but it could be weird dealing with both this and the result from Dmain if they conflict.  The easiest alternative would be to either change the calling convention for the unittest handler, or add some kind of global flag to indicate whether Dmain should be run.

Thanks for looking into this. A fix would be awesome!

Andrei

On 07/06/2010 12:07 AM, Sean Kelly wrote:
> On Jul 5, 2010, at 9:13 PM, Andrei Alexandrescu wrote:
>
>> On 07/05/2010 02:44 PM, Walter Bright wrote:
>>> The failing unittests still exit with an errorlevel. The change was to not exit immediately on the assert error, but to remember that it failed, run all the unittests, then exit with errorlevel set to 1 if any of them had failed.
>>
>> I'm glad we see eye to eye on that! In fact there is a bug in your implementation of Linux - it doesn't do as you mention above. Look at this:
>>
>> $ generated/posix/debug/unittest/std/xml
>> std.xml(379): unittest failure
>> $ echo $?
>> 0
>> $ _
>>
>> So something somewhere makes the program return success even though it fails.
>
> That's probably my fault.  The unittest handler returns a boolean to indicate whether the app should run when tests complete.  I believe Walter repurposed it to have the unittest handler indicate success/failure and I changed it back.  We really need to be able to do both--set a result and allow only tests to be run.  It may be worth adding a Runtime.result property that the app returns on exit, but it could be weird dealing with both this and the result from Dmain if they conflict.  The easiest alternative would be to either change the calling convention for the unittest handler, or add some kind of global flag to indicate whether Dmain should be run.
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos

Sean Kelly wrote:
> That's probably my fault.

Every team needs one of those people. Thanks for filling that role.

On Jul 5, 2010, at 11:35 PM, Walter Bright wrote:
> 
> Sean Kelly wrote:
>> That's probably my fault.
> 
> Every team needs one of those people. Thanks for filling that role.

I like to maintain a steady level of failure to keep things positive ;-)

How about this... the unittest handler only returns an error code and if it's nonzero then the app won't run.  That's consistent with the pre-assert change behavior.  The difference being that even if the unittests are successful, to prevent the app from running, a non-zero error should be returned.  It's been so long since I originally implemented this (it was done for Gregor when I was working on Tango) that I don't recall the exact use case he presented, but it seems reasonable that if you run an app and it runs some tests but doesn't actually execute then the return value should be nonzero, even if all the tests succeed.  What do you think?

Le 2010-07-06 ? 11:28, Sean Kelly a ?crit :

> On Jul 5, 2010, at 11:35 PM, Walter Bright wrote:
>> 
>> Sean Kelly wrote:
>>> That's probably my fault.
>> 
>> Every team needs one of those people. Thanks for filling that role.
> 
> I like to maintain a steady level of failure to keep things positive ;-)
> 
> How about this... the unittest handler only returns an error code and if it's nonzero then the app won't run.

This is what I was expecting the change would do originally. Running the program should be conditional to the unit tests being successful, otherwise it's too easy to ignore them.

Another way to put it is: do you want your failed unit tests to behave as warnings or as errors? I choose errors.


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