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

[Josh Steadmon <steadmon@google.com>]

On 2023.08.17 17:12, Junio C Hamano wrote:
> 
> 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)?  

I am unsure whether or not this is an improvement. While it would
certainly help readability and reduce duplication if this were
production code, in test code it can often be more valuable to be
verbose and explicit, so that individual broken test cases can be
quickly understood without having to do a lot of cross referencing.

I'll hold off on adding any more utility functions in t-strbuf for V8,
but if you or other folks feel strongly about it we can address it in
V9.


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

Yes, I think this is too much of an internal detail. None of the users
of strbuf ever reference it directly. Presumably for library-ish code,
we should stick to testing just the user-observable parts, not the
implementation.


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

Yeah, in general I think this is a good improvement, but again I'm not
sure if it's worth adding additional helpers. I'll try to rework this a
bit in V8.


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