Am 02.07.24 um 17:13 schrieb phillip.wood123@xxxxxxxxx:
> Hi René
>
> On 29/06/2024 16:43, René Scharfe wrote:
>> The macro TEST only allows defining a test that consists of a
>> single expression. Add the new macro, TEST_RUN, which provides a
>> way to define unit tests that are made up of one or more
>> statements. A test started with it implicitly ends when the next
>> test is started or test_done() is called.
>>
>> TEST_RUN allows defining self-contained tests en bloc, a bit like
>> test_expect_success does for regular tests. Unlike TEST it does
>> not require defining wrapper functions for test statements.
>
> There are pros and cons to not requiring one function per test. It
> can be a pain to have to write separate functions but it keeps each
> test self contained which hopefully makes it harder to have
> accidental dependencies between tests. Having separate functions for
> each test makes it easy to initialize and free resources for every
> test by writing a setup() function that initializes the resources,
> calls the test function and then frees the resources.
Right. We should use TEST and TEST_RUN when appropriate.
> The changes in patch 6 to use TEST_RUN() mean that each test now has
> more boilerplate to initialize and free the strbuf.
This makes them more similar to strbuf usage in the wild. Using
the API idiomatically just makes more sense to me. Not hiding
initialization and release makes the tests visibly independent.
This is not enforced by TEST_RUN, but made possible by it.
> Having each test in its own function also makes main() shorter and
> which means can quickly get an overview of all the test cases from
> it.
That's true, now you need to grep for TEST_RUN to get such an
overview.
On the other hand I find the start of the description in TEST
invocations somewhat hard to locate, as they are not vertically
aligned due to the preceding variable-length function name. Just
saying..
>> +int test__run(const char *location, const char *format, ...)
>> +{
>> + va_list ap;
>> + char *desc;
>> +
>> + test__run_maybe_end();
>> +
>> + va_start(ap, format);
>> + desc = xstrvfmt(format, ap);
>
> This uses an strbuf under the hood. So far we've avoided doing that
> as we want to be able to test the strbuf implementation with this
> test framework. We don't need to support arbitrary length strings
> here so we could use a fixed array and xsnprinf() instead.
Fair point. xsnprinf() might be a bit too strict, as it doesn't
handle short buffers gracefully. Perhaps that's OK; a developer
getting hit by that could simply increase the buffer size.
We could also let xstrvfmt() call vsnprintf(3) directly. The code
duplication would be a bit grating, but perhaps there's some good
base function hidden in there somewhere.
> Looking ahead the plan seems to be to change most instances of TEST()
> to TEST_RUN(). If we are going to go that way perhaps we should steal
> TEST() for this macro and rename the existing TEST() macro.
Not my plan, at least -- I'm content with just having the *ability*
to keep all parts of a test together.
If we wanted to do that, though, then TEST_RUN would have to be
complemented with a blessed way to check ctx.result in order to handle
the t_static_init test of t-strbuf, which I mentioned in my earlier
reply to Josh.
Err, no, we can simply check the check_* results, like check_strvec_loc
does:
diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c
index c8e39ddda7..c765fab53a 100644
--- a/t/unit-tests/t-strbuf.c
+++ b/t/unit-tests/t-strbuf.c
@@ -19,15 +19,6 @@ static int assert_sane_strbuf(struct strbuf *buf)
return check_uint(buf->len, <, buf->alloc);
}
-static void t_static_init(void)
-{
- struct strbuf buf = STRBUF_INIT;
-
- check_uint(buf.len, ==, 0);
- check_uint(buf.alloc, ==, 0);
- check_char(buf.buf[0], ==, '\0');
-}
-
static void t_dynamic_init(void)
{
struct strbuf buf;
@@ -85,8 +76,14 @@ static void t_release(struct strbuf *sb)
int cmd_main(int argc, const char **argv)
{
- if (!TEST(t_static_init(), "static initialization works"))
- test_skip_all("STRBUF_INIT is broken");
+ if (TEST_RUN("static initialization works")) {
+ struct strbuf buf = STRBUF_INIT;
+ if (!check_uint(buf.len, ==, 0) ||
+ !check_uint(buf.alloc, ==, 0) ||
+ !check_char(buf.buf[0], ==, '\0'))
+ test_skip_all("STRBUF_INIT is broken");
+ }
+
TEST(t_dynamic_init(), "dynamic initialization works");
if (TEST_RUN("strbuf_addch adds char")) {
> I'm not very enthusiastic about requiring the test author to wrap
> TEST_RUN() in an if() statement rather than just doing that for them.
> It makes it explicit but from the test author's point of view it just
> feels like pointless boilerplate.
Hmm. We can add more magic, but I suspect that it might confuse
developers and editors.
René
