[phobos] Silent failure of std.container unittests

[Jonathan M Davis]

On Saturday 17 July 2010 05:17:09 Michel Fortin wrote:
> Le 2010-07-17 à 1:07, Walter Bright a écrit :
> > 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 just want to point out that "unittest assert(x);" would just be a
> shortcut for "unittest { assert(x); }", which is quite similar to how you
> can omit the braces for conditional and loop statements in regular code.
> This is only a small shift from the earlier concept of unittest in D:
> instead of having unittest blocks scattered in your module, you have
> unittest statements (which can be blocks) scattered around. It's not much
> of a change, it's just being more permissive.
> 
> 	module abc;
> 
> 	unittest assert(sqrt(4) == 2); // single-statement unit test
> 	unittest assert(sqrt(16) == 4);
> 
> As for nesting unittests, I agree that it might get harder to conceptualize
> how it works. But still, there isn't anything really 'special' about it,
> they have the same semantics as the module-level ones. It's better
> explained with a concrete example. Say a module includes those unit tests:
> 
> 	module abc;
> 
> 	unittest assert(sqrt(4) == 2);
> 	unittest assert(sqrt(16) == 4);
> 
> 	unittest {
> 		int[] result = makeSomeTable(399, 383, 927);
> 		assert(result.length == 3); // guard against invalid state
> 
> 		unittest assert(result[0] == 281);
> 		unittest assert(result[1] == 284);
> 		unittest assert(result[2] == 283);
> 
> 		unittest {
> 			int[] derivedResult = derive(result);
> 			assert(derivedResult.length == 2); // guard against invalid state
> 
> 			unittest assert(derivedResult[0] ==  3);
> 			unittest assert(derivedResult[1] == -1);
> 		}
> 		// ... other tests reusing 'result'
> 	}
> 
> How do you translate this to a module-level unittest function? First define
> this function in the runtime:
> 
> 	void __doUnitTest(void delegate() test) {
> 		try { test(); }
> 		catch (AssertionError e) { __unitTestLogFailure(e); }
> 	}
> 
> Then replace each unittest statement or block with a call to the above
> function, passing the unittest code as the argument. The three
> module-level unittests I wrote above would translate to this module
> unittest function:
> 
> 	void __module_abc_unittests() {
> 		__doUnitTest({  assert(sqrt(4) == 2); });
> 		__doUnitTest({  assert(sqrt(16) == 4); });
> 
> 		__doUnitTest({
> 			int[] result = makeSomeTable(399, 383, 927);
> 			assert(result.length == 3); // guard against invalid state
> 
> 			__doUnitTest({  assert(result[0] == 281);  });
> 			__doUnitTest({  assert(result[1] == 284);  });
> 			__doUnitTest({  assert(result[2] == 283);  });
> 
> 			__doUnitTest({
> 				int[] derivedResult = derive(result);
> 				assert(derivedResult.length == 2); // guard against invalid 
state
> 
> 				__doUnitTest({  assert(derivedResult[0] ==  3);  });
> 				__doUnitTest({  assert(derivedResult[1] == -1);  });
> 			});
> 			// ... other tests reusing 'result'
> 		});
> 	}
> 
> As other mentioned, what I do here with nested unit tests could easily be
> implemented by offering the user a new library function too. I just happen
> to think what I'm proposing here is more elegant.

I'd have to argue with you on that one. Personally, I find it to be far uglier 
than simply replacing assert with expect in cases where you don't want the test 
to stop on failure. A unittest block without braces is a bit odd, but I could 
see that working and not being a big deal. But a unit test block inside another? 
Sure, you could do it, but I think that it would be confusing to new users. It's 
also more verbose than simply changing assert to expect, and I think that it's a 
lot uglier. Obviously, this is at least somewhat subjective, however, since you 
clearly think that nesting unittest is more elegant.

Also, if you went down the road of using expect (verify or check or whatever) 
instead of assert for tests where you didn't want the test to stop, then that 
opens the door to the possibility of adding other such functions later if it's 
deemed appropriate. You could probably even have a library module dedicated to 
additional features for unittests. Many unit test frameworks have functions like 
assertEquals() and assertNull() to allow for better debugging. Now, I'm not 
suggesting that we do that right now, but if we go the route of adding a new 
function to be used instead of assert for testing without stopping the test, 
adding such a library would then be reasonable extension of that which could be 
done at a later date. Using nested unittests does not make things more flexible 
in that manner. It just contorts the current construct so that it does more.

Not to mention, I am a proponent of the idea of named unit tests (e.g. 
unittest(test_name) {}). And they don't really make sense when nested. Sure, you 
could have named external ones and non-named nested ones, but then you'd have to 
worry about whether a test was nested or not when allowing a name on it.

Really, it seems to me that the most straightforward and extensible approach is 
to make assertions function normally (throwing from the unittest block on 
failure and ceasing its execution), make it so that each unittest block is run 
individually regardless of whether any others have succeeded or failed, and add 
a second function which tests its assertion but does not throw on failure 
(though it would still set the appropriate flag so that the overall unittest 
block would count as failed, and main() would not be run). It's straightfoward, 
extensible, easily understood, and I really do think that it's the way to go at 
this point.

- Jonathan M Davis