Re: [PATCH v7 2/3] unit tests: add TAP unit test framework

[Junio C Hamano <gitster@pobox.com>]

It seems this got stuck during Josh's absense and I didn't ping it
further, but I should have noticed that you are the author of this
patch, and pinged you in the meantime.

Any thought on the "polarity" of the return values from the
assertion?  I still find it confusing and hard to follow.

Thanks.

> Josh Steadmon <steadmon@google.com> writes:
>
>> +test_expect_success 'TAP output from unit tests' '
>> +	cat >expect <<-EOF &&
>> +	ok 1 - passing test
>> +	ok 2 - passing test and assertion return 0
>> +	# check "1 == 2" failed at t/unit-tests/t-basic.c:76
>> +	#    left: 1
>> +	#   right: 2
>> +	not ok 3 - failing test
>> +	ok 4 - failing test and assertion return -1
>> +	not ok 5 - passing TEST_TODO() # TODO
>> +	ok 6 - passing TEST_TODO() returns 0
>> +	# todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
>> +	not ok 7 - failing TEST_TODO()
>> +	ok 8 - failing TEST_TODO() returns -1
>> +	# check "0" failed at t/unit-tests/t-basic.c:30
>> +	# skipping test - missing prerequisite
>> +	# skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
>> +	ok 9 - test_skip() # SKIP
>> +	ok 10 - skipped test returns 0
>> +	# skipping test - missing prerequisite
>> +	ok 11 - test_skip() inside TEST_TODO() # SKIP
>> +	ok 12 - test_skip() inside TEST_TODO() returns 0
>> +	# check "0" failed at t/unit-tests/t-basic.c:48
>> +	not ok 13 - TEST_TODO() after failing check
>> +	ok 14 - TEST_TODO() after failing check returns -1
>> +	# check "0" failed at t/unit-tests/t-basic.c:56
>> +	not ok 15 - failing check after TEST_TODO()
>> +	ok 16 - failing check after TEST_TODO() returns -1
>> +	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
>> +	#    left: "\011hello\\\\"
>> +	#   right: "there\"\012"
>> +	# check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
>> +	#    left: "NULL"
>> +	#   right: NULL
>> +	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
>> +	#    left: ${SQ}a${SQ}
>> +	#   right: ${SQ}\012${SQ}
>> +	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
>> +	#    left: ${SQ}\\\\${SQ}
>> +	#   right: ${SQ}\\${SQ}${SQ}
>> +	not ok 17 - messages from failing string and char comparison
>> +	# BUG: test has no checks at t/unit-tests/t-basic.c:91
>> +	not ok 18 - test with no checks
>> +	ok 19 - test with no checks returns -1
>> +	1..19
>> +	EOF
>
> Presumably t-basic will serve as a catalog of check_* functions and
> the test binary, together with this test piece, will keep growing as
> we gain features in the unit tests infrastructure.  I wonder how
> maintainable the above is, though.  When we acquire new test, we
> would need to renumber.  What if multiple developers add new
> features to the catalog at the same time?
>
>> diff --git a/t/unit-tests/.gitignore b/t/unit-tests/.gitignore
>> new file mode 100644
>> index 0000000000..e292d58348
>> --- /dev/null
>> +++ b/t/unit-tests/.gitignore
>> @@ -0,0 +1,2 @@
>> +/t-basic
>> +/t-strbuf
>
> Also, can we come up with some naming convention so that we do not
> have to keep adding to this file every time we add a new test
> script?
>
>> diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c
>> new file mode 100644
>> index 0000000000..561611e242
>> --- /dev/null
>> +++ b/t/unit-tests/t-strbuf.c
>> @@ -0,0 +1,75 @@
>> +#include "test-lib.h"
>> +#include "strbuf.h"
>> +
>> +/* wrapper that supplies tests with an initialized strbuf */
>> +static void setup(void (*f)(struct strbuf*, void*), void *data)
>> +{
>> +	struct strbuf buf = STRBUF_INIT;
>> +
>> +	f(&buf, data);
>> +	strbuf_release(&buf);
>> +	check_uint(buf.len, ==, 0);
>> +	check_uint(buf.alloc, ==, 0);
>> +	check(buf.buf == strbuf_slopbuf);
>> +	check_char(buf.buf[0], ==, '\0');
>> +}
>
> What I am going to utter from here on are not complaints but purely
> meant as questions.  
>
> Would the resulting output and maintainability of the tests change
> (improve, or worsen) if we introduce
>
> 	static void assert_empty_strbuf(struct strbuf *buf)
> 	{
> 		check_uint(buf->len, ==, 0);
>                 check_uint(buf->alloc, ==, 0);
> 		check(buf.buf == strbuf_slopbuf);
> 		check_char(buf.buf[0], ==, '\0');
> 	}
>
> and call it from the setup() function to ensure that
> strbuf_release(&buf) it calls after running customer test f() brings
> the buffer in a reasonably initialized state?  The t_static_init()
> test should be able to say
>
> 	static void t_static_init(void)
> 	{
> 		struct strbuf buf = STRBUF_INIT;
> 		assert_empty_strbuf(&buf);
> 	}
>
> if we did so, but is that a good thing or a bad thing (e.g. it may
> make it harder to figure out where the real error came from, because
> of the "line number" thing will not easily capture the caller of the
> caller, perhaps)?  
>
>> +static void t_static_init(void)
>> +{
>> +	struct strbuf buf = STRBUF_INIT;
>> +
>> +	check_uint(buf.len, ==, 0);
>> +	check_uint(buf.alloc, ==, 0);
>> +	if (check(buf.buf == strbuf_slopbuf))
>> +		return; /* avoid de-referencing buf.buf */
>
> strbuf_slopbuf[0] is designed to be readable.  Do check() assertions
> return their parameter negated?
>
> In other words, if "we expect buf.buf to point at the slopbuf, but
> if that expectation does not hold, check() returns true and we
> refrain from doing check_char() on the next line because we cannot
> trust what buf.buf points at" is what is going on here, I find it
> very confusing.  Perhaps my intuition is failing me, but somehow I
> would have expected that passing check_foo() would return true while
> failing ones would return false.
>
> IOW I would expect
>
> 	if (check(buf.buf == strbuf_slopbuf))
> 		return;
>
> to work very similarly to
>
> 	if (buf.buf == strbuf_slopbuf)
> 		return;
>
> in expressing the control flow, simply because they are visually
> similar.  But of course, if we early-return because buf.buf that
> does not point at strbuf_slopbuf is a sign of trouble, then the
> control flow we want is
>
> 	if (buf.buf != strbuf_slopbuf)
> 		return;
>
> or
>
> 	if (!(buf.buf == strbuf_slopbuf))
> 		return;
>
> The latter is easier to translate to check_foo(), because what is
> inside the inner parentheses is the condition we expect, and we
> would like check_foo() to complain when the condition does not hold.
>
> For the "check_foo()" thing to work in a similar way, while having
> the side effect of reporting any failed expectations, we would want
> to write
>
> 	if (!check(buf.buf == strbuf_slopbuf))
> 		return;
>
> And for that similarity to work, check_foo() must return false when
> its expectation fails, and return true when its expectation holds.
>
> I think that is where my "I find it very confusing" comes from.
>
>> +	check_char(buf.buf[0], ==, '\0');
>> +}
>
>> +static void t_dynamic_init(void)
>> +{
>> +	struct strbuf buf;
>> +
>> +	strbuf_init(&buf, 1024);
>> +	check_uint(buf.len, ==, 0);
>> +	check_uint(buf.alloc, >=, 1024);
>> +	check_char(buf.buf[0], ==, '\0');
>
> Is it sensible to check buf.buf is not slopbuf at this point, or
> does it make the test TOO intimate with the current implementation
> detail?
>
>> +	strbuf_release(&buf);
>> +}
>> +
>> +static void t_addch(struct strbuf *buf, void *data)
>> +{
>> +	const char *p_ch = data;
>> +	const char ch = *p_ch;
>> +
>> +	strbuf_addch(buf, ch);
>> +	if (check_uint(buf->len, ==, 1) ||
>> +	    check_uint(buf->alloc, >, 1))
>> +		return; /* avoid de-referencing buf->buf */
>
> Again, I find the return values from these check_uint() calls highly
> confusing, if this is saying "if len is 1 and alloc is more than 1,
> then we are in an expected state and can further validate that buf[0]
> is ch and buf[1] is NULL, but otherwise we should punt".  The polarity
> looks screwy.  Perhaps it is just me?
>
>> +	check_char(buf->buf[0], ==, ch);
>> +	check_char(buf->buf[1], ==, '\0');
>> +}
>
> In any case, this t_addch() REQUIRES that incoming buf is empty,
> doesn't it?  I do not think it is sensible.  I would have expected
> that it would be more like
>
> 	t_addch(struct strbuf *buf, void *data)
> 	{
> 		char ch = *(char *)data;
> 		size_t orig_alloc = buf->alloc;
> 		size_t orig_len = buf->len;
>
> 		if (!assert_sane_strbuf(buf))
> 			return;
>                 strbuf_addch(buf, ch);
> 		if (!assert_sane_strbuf(buf))
> 			return;
> 		check_uint(buf->len, ==, orig_len + 1);
> 		check_uint(buf->alloc, >=, orig_alloc);
>                 check_char(buf->buf[buf->len - 1], ==, ch);
>                 check_char(buf->buf[buf->len], ==, '\0');
> 	}
>
> to ensure that we can add a ch to a strbuf with any existing
> contents and get a one-byte longer contents than before, with the
> last byte of the buffer becoming 'ch' and still NUL terminated.
>
> And we protect ourselves with a helper that checks if the given
> strbuf looks *sane*.
>
> 	static int assert_sane_strbuf(struct strbuf *buf)
> 	{
>         	/* can use slopbuf only when the length is 0 */
> 		if (buf->buf == strbuf_slopbuf)
>                 	return (buf->len == 0);
> 		/* everybody else must have non-NULL buffer */
> 		if (buf->buf == NULL)
> 			return 0;
>                 /* 
> 		 * alloc must be at least 1 byte larger than len
> 		 * for the terminating NUL at the end.
> 		 */
> 		return ((buf->len + 1 <= buf->alloc) &&
> 		    	(buf->buf[buf->len] == '\0'));
> 	}
>
> You can obviously use your check_foo() for the individual checks
> done in this function to get a more detailed diagnosis, but because
> I have confused myself enough by thinking about their polarity, I
> wrote this in barebones comparison instead.