Re: [PATCH v8 0/3] Add unit test framework and project plan

[Josh Steadmon <steadmon@google.com>]

On 2023.10.16 11:07, phillip.wood123@gmail.com wrote:
> 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.

This is my feeling as well.


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

I like this approach. We'll need to document unit-test best practices,
but I think now that I'll want to do that in a separate series after
this one lands.


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

Thanks for the suggestions! I will make sure these make it into the best
practices doc.


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

Fixed in v9.

> Best Wishes
> 
> Phillip