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
