Adam Ruppe wrote:
> On 7/15/10, Walter Bright <walter at digitalmars.com> wrote:
> 
>>> Assertion failure should abort the current unittest block.
>>>
>>> Do we all agree about the above?
>>> 
>> Not me.
>> 
>
> That's madness!
>
> unittest {
>     auto a = something();
>     assert(a !is null);
>      a.stuff(); // if a is null, this is meaningless
> }
> 

Yes, and I could easily rewrite that as:

unittest {
    auto a = something();
    if (a)
        a.stuff();
    else
        assert(0);

Is that worse than your rewrite of the following?

> 
>> Everybody's happy except the guys like me who want to run all the unit
>> tests in one go.
>> 
>
> What you could do is:
>
> unittest { assert(my test); }
> unittest { assert(my other test); }
> unittest {
>   // complex test
> }
>
> It is a bit verbose, but you'd get all the results you want without breaking cases like my first null assert one.
>
> 

Yeah, but try it if you want to go through a table of inputs and compare the results to a table of outputs. Of course, that can be done, too, but it's extra code as well.

I'd guess this code is pre- at disable.  Not sure what the correct fix is for std.container though, since it seems reasonable for .tupleof to perform a copy.

On Jul 13, 2010, at 11:59 PM, Lars Tandle Kyllingstad wrote:

> 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
> 
> 
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos

That's a bug in the compiler that I reported. I'll get back to you guys with std.container.

Andrei

Sean Kelly wrote:
> I'd guess this code is pre- at disable.  Not sure what the correct fix is for std.container though, since it seems reasonable for .tupleof to perform a copy.
> 
> On Jul 13, 2010, at 11:59 PM, Lars Tandle Kyllingstad wrote:
> 
>> 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
>>
>> _______________________________________________
>> 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

Walter Bright, el 16 de julio a las 12:13 me escribiste:
> 
> 
> Adam Ruppe wrote:
> >On 7/15/10, Walter Bright <walter-UwkSZrAjFffxfQ63PejmQw at public.gmane.org> wrote:
> >>>Assertion failure should abort the current unittest block.
> >>>
> >>>Do we all agree about the above?
> >>Not me.
> >
> >That's madness!
> >
> >unittest {
> >    auto a = something();
> >    assert(a !is null);
> >     a.stuff(); // if a is null, this is meaningless
> >}
> 
> Yes, and I could easily rewrite that as:
> 
> unittest {
>    auto a = something();
>    if (a)
>        a.stuff();
>    else
>        assert(0);
> 
> Is that worse than your rewrite of the following?

Yes :)

> >>Everybody's happy except the guys like me who want to run all the unit tests in one go.
> >
> >What you could do is:
> >
> >unittest { assert(my test); }
> >unittest { assert(my other test); }
> >unittest {
> >  // complex test
> >}
> >
> >It is a bit verbose, but you'd get all the results you want without breaking cases like my first null assert one.
> >
> 
> Yeah, but try it if you want to go through a table of inputs and compare the results to a table of outputs. Of course, that can be done, too, but it's extra code as well.

import std.test: check;

unittest {
	check(my test); // use expect() if it makes Andrei happier =P
	check(my other test);
}

Done! Really Walter, what's the point of changing assert semantics for something that can be naturally solved by a new function. Everybody is happy then, and there are no odd behaviour or backward-incompatible changes.

-- 
Leandro Lucarella (AKA luca)                     http://llucax.com.ar/
----------------------------------------------------------------------
GPG Key: 5F5A8D05 (F8CD F9A7 BF00 5431 4145  104C 949E BFB6 5F5A 8D05)
----------------------------------------------------------------------
In Minnesota there's a law
That prevents men from having sex with living fish

Walter Bright wrote:
>
>
> Adam Ruppe wrote:
>> On 7/15/10, Walter Bright <walter at digitalmars.com> wrote:
>> 
>>>> Assertion failure should abort the current unittest block.
>>>>
>>>> Do we all agree about the above?
>>>> 
>>> Not me.
>>> 
>>
>> That's madness!
>>
>> unittest {
>>     auto a = something();
>>     assert(a !is null);
>>      a.stuff(); // if a is null, this is meaningless
>> }
>> 
>
> Yes, and I could easily rewrite that as:
>
> unittest {
>    auto a = something();
>    if (a)
>        a.stuff();
>    else
>        assert(0);
>
> Is that worse than your rewrite of the following?
>
>> 
>>> Everybody's happy except the guys like me who want to run all the unit
>>> tests in one go.
>>> 
>>
>> What you could do is:
>>
>> unittest { assert(my test); }
>> unittest { assert(my other test); }
>> unittest {
>>   // complex test
>> }
>>
>> It is a bit verbose, but you'd get all the results you want without breaking cases like my first null assert one.
>>
>> 
>
> Yeah, but try it if you want to go through a table of inputs and compare the results to a table of outputs. Of course, that can be done, too, but it's extra code as well.

Can we all agree that  a failed assert should end a "Unit test" ("Unit test" != unittest)?

My thought is, that we might be running into issues because the idea of a unit test is being conflated with a function. What we really want is a collection of blocks of code that that are all run and if any of them trigger an assert, that block stops and the tests as a whole are considered to have failed.

As a point to start from, how about allow unittest as a statement type?

foreach(item; collection) unittest { asssert(foobar(item)); }

> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos
>

On Friday, July 16, 2010 17:47:33 Benjamin Shropshire wrote:
> 
> Can we all agree that  a failed assert should end a "Unit test" ("Unit
> test" != unittest)?

In any unit test framework that I've used and certainly any place that I've worked at which used unit tests, a unit test _is_ a function. A single assertion is just part of the test.

> 
> My thought is, that we might be running into issues because the idea of a unit test is being conflated with a function. What we really want is a collection of blocks of code that that are all run and if any of them trigger an assert, that block stops and the tests as a whole are considered to have failed.
> 
> As a point to start from, how about allow unittest as a statement type?
> 
> foreach(item; collection) unittest { asssert(foobar(item)); }

Regardless of whether you consider the assertion or the unittest block as the unit test (and I would certainly choose the latter), I don't see any point to this. What would this do? A unittest block is a block of code which is run before main when your program is built with -unittest. How would that jive with a foreach loop? Do you want code within functions to suddenly be compiled in or not depending on whether the -unittest flag was used?

I think that what we have in terms of code structure is quite good. The only suggestion that I would make to that would be to add optional names (e.g. unittest(test_name) {}) so that it would be possible at a later date to make it so that tests could be run individually (particularly by 3rd party tools like IDEs). But that could be added at pretty much any time, since it's not a breaking change.

Regardless, I don't understand what your foreach loop is really trying to do here. Are you trying to get it to get it to run the test on each item in the collection and report errors at each failure without affecting the other tests? If that's the case, I don't see what this buys us over just have a special function - like expect() or verify() or check() or whatever - which prints out the error and continues, having set a global flag to indicate that one of the unit tests failed so that main() won't be run.

- Jonathan M Davis

Le 2010-07-16 ? 21:03, Jonathan M Davis a ?crit :

>> My thought is, that we might be running into issues because the idea of a unit test is being conflated with a function. What we really want is a collection of blocks of code that that are all run and if any of them trigger an assert, that block stops and the tests as a whole are considered to have failed.
>> 
>> As a point to start from, how about allow unittest as a statement type?
>> 
>> foreach(item; collection) unittest { asssert(foobar(item)); }
> 
> Regardless of whether you consider the assertion or the unittest block as the unit test (and I would certainly choose the latter), I don't see any point to this.

Walter had a point... what if you want to test multiple outputs of the same function on one go? Here's an example:

	unittest {
		int[3] result = makeSomeTable(399, 383, 927);
		assert(result[0] == 281);
		assert(result[1] == 281);
		assert(result[2] == 281);
	}

Here, each assert is independent one from another and knowing that the first and the third fails but not the second might help diagnose the problem.

I think Jonathan's idea is quite on the spot. It'd allow you to write this:

	unittest {
		int[3] result = makeSomeTable(399, 383, 927);
		unittest { assert(result[0] == 281); }
		unittest { assert(result[1] == 281); }
		unittest { assert(result[2] == 281); }
	}

and each "unittest assert" becomes an independent test that may fail but without interrupting the flow of the outside scope. The compiler could expand it like this:

	unittest {
		int[3] result = makeSomeTable(399, 383, 927);

		try
			assert(result[0] == 281);
		catch (AssertionError e)
			__unitTestLogFailure(e);

		try
			assert(result[1] == 281);
		catch (AssertionError e)
			__unitTestLogFailure(e);

		try
			assert(result[2] == 281);
		catch (AssertionError e)
			__unitTestLogFailure(e);
	}

And now you have a log entry for each failed assertion, because they're independent unit tests.

It'd also help reduce verbosity to not require the braces for one-statement unit tests.


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

Michel Fortin wrote:
> Walter had a point... what if you want to test multiple outputs of the same function on one go? Here's an example:
>
> 	unittest {
> 		int[3] result = makeSomeTable(399, 383, 927);
> 		assert(result[0] == 281);
> 		assert(result[1] == 281);
> 		assert(result[2] == 281);
> 	}
>
> Here, each assert is independent one from another and knowing that the first and the third fails but not the second might help diagnose the problem.
>
> I think Jonathan's idea is quite on the spot. It'd allow you to write this:
>
> 	unittest {
> 		int[3] result = makeSomeTable(399, 383, 927);
> 		unittest { assert(result[0] == 281); }
> 		unittest { assert(result[1] == 281); }
> 		unittest { assert(result[2] == 281); }
> 	}
>
> and each "unittest assert" becomes an independent test that may fail but without interrupting the flow of the outside scope.

That the use mode I was thinking of. Right after I hit send, I realized that what I had said would include allowing unittests in main.

Michel Fortin wrote:
>
> But of course, that's probably more work to do in the compiler.
>
> 

Yes, but it's not hard. For me, the issue is making things for the user more complicated. D is already a very large language, and adding more and more stuff like this makes it larger, buggier, and less approachable. For example, nothing stands out to say what "unittest assert" does vs "assert". I'd have to continuously recheck the manual. One thing I like about the current unittest setup is how utterly trivial it is to use effectively.

Is it really a good idea to have so much customizable behavior wedded into the grammar, whereas it can be achieved using existing D constructs (albeit with a little more typing)? I think the default behavior should be the simplest, "git 'er done" that covers 90% of the usage needs. The other 10% can be done with conventional D constructs.

Michel Fortin wrote:
>
> 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.
> 

Yes, for that sequence, the rest are irrelevant. But what I don't understand is why is it so bad that they also generate messages, which you just ignore looking over the log file? Me, I'd rather see a few extra cascaded messages, fix the ones that matter and ignore the extra, than be short messages and have to rebuild/rerun the tests just to get dinged again.


> It also highlights another problem about the current behaviour: what happens if one of the functions called in this test throws an exception?

The special behavior of assert() inside unittest blocks is only for the
assert()s that are lexically enclosed by the unittest block. I.e. any
others will have the usual behavior.