From: phillip.wood123@gmail.com
Date: Mon, 16 Oct 2023 10:07:02 GMT
Subject: Re: [PATCH v8 0/3] Add unit test framework and project plan
Message-ID: <93a18989-bf05-4318-8d85-cf23c0f32170@gmail.com>
In-Reply-To: <cover.1696889529.git.steadmon@google.com>

Hi Josh

Thanks for the update

On 09/10/2023 23:21, Josh Steadmon wrote:
> In addition to reviewing the patches in this series, reviewers can help
> this series progress by chiming in on these remaining TODOs:
> - Figure out if we should split t-basic.c into multiple meta-tests, to
>    avoid merge conflicts and changes to expected text in
>    t0080-unit-test-output.sh.

I think it depends on how many new tests we think we're going to want to 
add here. I can see us adding a few more check_* macros (comparing 
object ids and arrays of bytes spring to mind) and wanting to test them 
here, but (perhaps naïvely) I don't expect huge amounts of churn here.

> - Figure out if we should de-duplicate assertions in t-strbuf.c at the
>    cost of making tests less self-contained and diagnostic output less
>    helpful.

In principle we could pass the location information along to any helper 
function, I'm not sure how easy that is at the moment. We can get 
reasonable error messages by using the check*() macros in the helper and 
wrapping the call to the helper with check() as well. For example

static int assert_sane_strbuf(struct strbuf *buf)
{
	/* Initialized strbufs should always have a non-NULL buffer */
	if (!check(!!buf->buf))
		return 0;
	/* Buffers should always be NUL-terminated */
	if (!check_char(buf->buf[buf->len], ==, '\0'))
		return 0;
	/*
	 * Freshly-initialized strbufs may not have a dynamically allocated
	 * buffer
	 */
	if (buf->len == 0 && buf->alloc == 0)
		return 1;
	/* alloc must be at least one byte larger than len */
	return check_uint(buf->len, <, buf->alloc);
}

and in the test function call it as

	check(assert_sane_strbuf(buf));

which gives error messages like

# check "buf->len < buf->alloc" failed at t/unit-tests/t-strbuf.c:43
#    left: 5
#   right: 0
# check "assert_sane_strbuf(&buf)" failed at t/unit-tests/t-strbuf.c:60

So we can see where assert_sane_strbuf() was called and which assertion 
in assert_sane_strbuf() failed.

> - Figure out if we should collect unit tests statistics similar to the
>    "counts" files for shell tests

Unless someone has an immediate need for that I'd be tempted to leave it 
wait until someone requests that data.

> - Decide if it's OK to wait on sharding unit tests across "sliced" CI
>    instances

Hopefully the unit tests will run fast enough that we don't need to 
worry about that in the early stages.

> - Provide guidelines for writing new unit tests

This is not a comprehensive list but we should recommend that

- tests avoid leaking resources so the leak sanitizer see if the code
   being tested has a resource leak.

- tests check that pointers are not NULL before deferencing them to
   avoid the whole program being taken down with SIGSEGV.

- tests are written with easy debugging in mind - i.e. good diagnostic
   messages. Hopefully the check* macros make that easy to do.

> Changes in v8:
> - Flipped return values for TEST, TEST_TODO, and check_* macros &
>    functions. This makes it easier to reason about control flow for
>    patterns like:
>      if (check(some_condition)) { ... } > - Moved unit test binaries to t/unit-tests/bin to simplify .gitignore
>    patterns.

Thanks for the updates to the test library, the range diff looks good to me.

 > - Removed testing of some strbuf implementation details in t-strbuf.c

I agree that makes sense. I think it would be good to update 
assert_sane_strbuf() to use the check* macros as suggest above.

Best Wishes

Phillip