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

> 
> 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)?  

> > +	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?

> > +	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.
>