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

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

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.