Am 01.07.24 um 21:58 schrieb Josh Steadmon:
> On 2024.06.29 17:47, René Scharfe wrote:
>> The macro TEST takes a single expression. If a test requires multiple
>> statements then they need to be placed in a function that's called in
>> the TEST expression. The functions setup() and setup_populated() here
>> are used for that purpose and take another function as an argument,
>> making the control flow hard to follow.
>>
>> Remove the overhead of these functions by using TEST_RUN instead. Move
>> their duplicate post-condition checks into a new helper, t_release(),
>> and let t_addch() and t_addstr() accept properly typed input parameters
>> instead of void pointers.
>>
>> Use the fully checking t_addstr() for adding initial values instead of
>> only doing only a length comparison -- there's no need for skipping the
>> other checks.
>>
>> This results in test cases that look much more like strbuf usage in
>> production code, only with checked strbuf functions replaced by checking
>> wrappers.
>>
>> Signed-off-by: René Scharfe <l.s.r@xxxxxx>
>> ---
>> t/unit-tests/t-strbuf.c | 79 +++++++++++++++++++++--------------------
>> 1 file changed, 41 insertions(+), 38 deletions(-)
>>
>> diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c
>> index 6027dafef7..c8e39ddda7 100644
>> --- a/t/unit-tests/t-strbuf.c
>> +++ b/t/unit-tests/t-strbuf.c
>> @@ -1,32 +1,6 @@
>> #include "test-lib.h"
>> #include "strbuf.h"
>>
>> -/* wrapper that supplies tests with an empty, initialized strbuf */
>> -static void setup(void (*f)(struct strbuf*, const void*),
>> - const void *data)
>> -{
>> - struct strbuf buf = STRBUF_INIT;
>> -
>> - f(&buf, data);
>> - strbuf_release(&buf);
>> - check_uint(buf.len, ==, 0);
>> - check_uint(buf.alloc, ==, 0);
>> -}
>> -
>> -/* wrapper that supplies tests with a populated, initialized strbuf */
>> -static void setup_populated(void (*f)(struct strbuf*, const void*),
>> - const char *init_str, const void *data)
>> -{
>> - struct strbuf buf = STRBUF_INIT;
>> -
>> - strbuf_addstr(&buf, init_str);
>> - check_uint(buf.len, ==, strlen(init_str));
>> - f(&buf, data);
>> - strbuf_release(&buf);
>> - check_uint(buf.len, ==, 0);
>> - check_uint(buf.alloc, ==, 0);
>> -}
>> -
>> static int assert_sane_strbuf(struct strbuf *buf)
>> {
>> /* Initialized strbufs should always have a non-NULL buffer */
>> @@ -66,10 +40,8 @@ static void t_dynamic_init(void)
>> strbuf_release(&buf);
>> }
>>
>> -static void t_addch(struct strbuf *buf, const void *data)
>> +static void t_addch(struct strbuf *buf, int ch)
>> {
>> - const char *p_ch = data;
>> - const char ch = *p_ch;
>> size_t orig_alloc = buf->alloc;
>> size_t orig_len = buf->len;
>>
>> @@ -85,9 +57,8 @@ static void t_addch(struct strbuf *buf, const void *data)
>> check_char(buf->buf[buf->len], ==, '\0');
>> }
>>
>> -static void t_addstr(struct strbuf *buf, const void *data)
>> +static void t_addstr(struct strbuf *buf, const char *text)
>> {
>> - const char *text = data;
>> size_t len = strlen(text);
>> size_t orig_alloc = buf->alloc;
>> size_t orig_len = buf->len;
>> @@ -105,18 +76,50 @@ static void t_addstr(struct strbuf *buf, const void *data)
>> check_str(buf->buf + orig_len, text);
>> }
>>
>> +static void t_release(struct strbuf *sb)
>> +{
>> + strbuf_release(sb);
>> + check_uint(sb->len, ==, 0);
>> + check_uint(sb->alloc, ==, 0);
>> +}
>> +
>> int cmd_main(int argc, const char **argv)
>> {
>> if (!TEST(t_static_init(), "static initialization works"))
>> test_skip_all("STRBUF_INIT is broken");
>> TEST(t_dynamic_init(), "dynamic initialization works");
>
> IIUC you're leaving t_static_init() as-is so that we can determine
> whether or not to skip the rest of the tests,
Right. And that place might be a use case for an official version of
test__run_end(), but it's probably better to gather a few more such
candidates before drawing conclusions.
> but is there a reason you
> didn't convert t_dynamic_init() here?
Good question, I don't remember. Perhaps an oversight or laziness.
Or just a general feeling to leave the init tests alone since the
first one doesn't map to TEST_RUN easily. So, in a word: No.
>> - TEST(setup(t_addch, "a"), "strbuf_addch adds char");
>> - TEST(setup(t_addch, ""), "strbuf_addch adds NUL char");
>> - TEST(setup_populated(t_addch, "initial value", "a"),
>> - "strbuf_addch appends to initial value");
>> - TEST(setup(t_addstr, "hello there"), "strbuf_addstr adds string");
>> - TEST(setup_populated(t_addstr, "initial value", "hello there"),
>> - "strbuf_addstr appends string to initial value");
>> +
>> + if (TEST_RUN("strbuf_addch adds char")) {
>> + struct strbuf sb = STRBUF_INIT;
>> + t_addch(&sb, 'a');
>> + t_release(&sb);
>> + }
>> +
>> + if (TEST_RUN("strbuf_addch adds NUL char")) {
>> + struct strbuf sb = STRBUF_INIT;
>> + t_addch(&sb, '\0');
>> + t_release(&sb);
>> + }
>> +
>> + if (TEST_RUN("strbuf_addch appends to initial value")) {
>> + struct strbuf sb = STRBUF_INIT;
>> + t_addstr(&sb, "initial value");
>> + t_addch(&sb, 'a');
>> + t_release(&sb);
>> + }
>> +
>> + if (TEST_RUN("strbuf_addstr adds string")) {
>> + struct strbuf sb = STRBUF_INIT;
>> + t_addstr(&sb, "hello there");
>> + t_release(&sb);
>> + }
>> +
>> + if (TEST_RUN("strbuf_addstr appends string to initial value")) {
>> + struct strbuf sb = STRBUF_INIT;
>> + t_addstr(&sb, "initial value");
>> + t_addstr(&sb, "hello there");
>> + t_release(&sb);
>> + }
>>
>> return test_done();
>> }
>> --
>> 2.45.2
>
> I think this commit in particular shows how TEST_RUN() is more
> convenient than TEST(). (Although, arguably we shouldn't have allowed
> the setup() + callback situation to start with.)
Great, thanks!
René
