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