Unit tests, asserts and contradictions in the spec

[H. S. Teoh]

On Wed, Feb 06, 2019 at 10:33:27PM +0000, Paul Backus via Digitalmars-d wrote:
> On Wednesday, 6 February 2019 at 21:40:38 UTC, H. S. Teoh wrote:
> > [...]
> > In unittests, however, we've had the convention of using asserts for
> > test failures since the early days.  Generally, UB caused by asserts
> > in this context probably wouldn't have the same risk as above, but
> > it does bring into question the wisdom of continuing to run after a
> > unittest has failed.  The trouble is that the assert may have
> > happened deep inside the call stack, and since it throws
> > AssertError, that means stack unwinding code is potentially never
> > run [...]
> > 
> > [...]
> > But this means that if the contract failed because of some
> > deeply-nested assert while evaluating the contract. E.g., the
> > contract calls some function which in turns calls a bunch of stuff,
> > and somewhere in there an assert fails. AssertError is thrown [...]
> 
> It seems like the fundamental problem here is the failure distinguish
> between a failed assertion *in the actual body* of a contract or unit
> test, and one that occurs deeper in the stack, in code called by the
> contract or unit test. Only assertions that are part of the unit
> test/contract body itself should have their failure treated specially,
> since they're the only ones written with the special unit
> test/contract assertion semantics in mind.
> 
> Probably the easiest way to distinguish different types of assertions
> would be to have them throw different AssertError subclasses (e.g.,
> UnittestAssertError, ContractAssertError), which the runtime could
> then check for specifically. This would allow existing code that
> expects AssertError, like custom test runners, to continue to "work",
> with the aforementioned caveats.

Hmm. A common practice when writing complex contracts that are
frequently used is to factor them out into common functions:

	auto myComplexFunc(Args...)(Args args)
	in {
		checkArgs(args);
	}
	do { ... }

	auto anotherComplexFunc(Args...)(Args args)
	in {
		checkArgs(args);
	}
	do { ... }

	void checkArgs(Args...)(Args args) {
		foreach (arg; args) {
			assert(someCondition(arg));
		}
	}

Since checkArgs is outside the contract scope, the compiler wouldn't
know to emit code to throw ContractAssertError rather than AssertError.

Of course, one could argue that checkArgs ought to return bool, and the
assert moved into the contract itself.  That's probably a reasonable
thing to do, but existing code may not follow such a practice.


Furthermore, the resource leak problem is still not really fixed:

	nothrow unittest {
		auto res = acquireResources();
		scope(exit) res.release();

		void runTest(Resource r) nothrow {
			doSomething(r);
			assert(r.success);
		}

		// Presumably run through all test cases
		foreach (r; res) {
			runTest(r);
		}
	}

Since the nested helper function is marked nothrow, a failed assert
throwing an AssertError or UnittestAssertError may bypass the unittest's
unwinding code (the compiler assumes runTest never throws, so never
installs stack unwinding code around it), which means the scope(exit)
never triggers and the resource is leaked.

This is a relatively benign example; imagine if you have a unittest that
calls alloca() and relies on cleanup code to adjust the stack pointer
properly.  A failed assert bypassing stack unwinding will then corrupt
your stack. (Of course, this is a problem with throwing Errors in
general, not just for unittests. But it shows that throwing a
UnittestAssertError instead of AssertError does not necessarily solve
the problem.)


T

-- 
Try to keep an open mind, but not so open your brain falls out. -- theboz