Re: [PATCH 1/2] strbuf: add tests

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 30, 2016, 11:26 UTC
Message-ID: <alpine.DEB.2.20.1605301323310.4449@virtualbox>
In-Reply-To: <20160530103642.7213-2-william.duclot@ensimag.grenoble-inp.fr>

Hi William,

On Mon, 30 May 2016, William Duclot wrote:

> Test the strbuf API. Being used throughout all Git the API could be
> considered tested, but adding specific tests makes it easier to improve
> and extend the API.
> ---

The commit message makes sense. Please add your sign-off.

Show 32 quoted lines

>  Makefile               |  1 +
>  t/helper/test-strbuf.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  t/t0082-strbuf.sh      | 19 ++++++++++++++
>  3 files changed, 89 insertions(+)
>  create mode 100644 t/helper/test-strbuf.c
>  create mode 100755 t/t0082-strbuf.sh
> 
> diff --git a/Makefile b/Makefile
> index 3f03366..dc84f43 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -613,6 +613,7 @@ TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
>  TEST_PROGRAMS_NEED_X += test-sha1
>  TEST_PROGRAMS_NEED_X += test-sha1-array
>  TEST_PROGRAMS_NEED_X += test-sigchain
> +TEST_PROGRAMS_NEED_X += test-strbuf
>  TEST_PROGRAMS_NEED_X += test-string-list
>  TEST_PROGRAMS_NEED_X += test-submodule-config
>  TEST_PROGRAMS_NEED_X += test-subprocess
> diff --git a/t/helper/test-strbuf.c b/t/helper/test-strbuf.c
> new file mode 100644
> index 0000000..622f627
> --- /dev/null
> +++ b/t/helper/test-strbuf.c
> @@ -0,0 +1,69 @@
> +#include "git-compat-util.h"
> +#include "strbuf.h"
> +
> +/*
> + * Check behavior on usual use cases
> + */
> +int test_usual(struct strbuf *sb)

I have to admit that I would prefer a more concrete name. And since your other tests are more fine-grained, maybe this one could be split into multiple separate ones, too?

> +{
> +	size_t size, old_alloc;
> +	char *res, *old_buf, *str_test = malloc(5*sizeof(char));

Our convention is to list the initialized variables first, the uninitialized ones after that, and for readability an empty line is recommended after the variable declaration block.

Show 27 quoted lines

> +	strbuf_grow(sb, 1);
> +	strcpy(str_test, "test");
> +	old_alloc = sb->alloc;
> +	strbuf_grow(sb, 1000);
> +	if (old_alloc == sb->alloc)
> +		die("strbuf_grow does not realloc the buffer as expected");
> +	old_buf = sb->buf;
> +	res = strbuf_detach(sb, &size);
> +	if (res != old_buf)
> +		die("strbuf_detach does not return the expected buffer");
> +	free(res);
> +
> +	strcpy(str_test, "test");
> +	strbuf_attach(sb, (void *)str_test, strlen(str_test), sizeof(str_test));
> +	res = strbuf_detach(sb, &size);
> +	if (res != str_test)
> +		die("strbuf_detach does not return the expected buffer");
> +	free(res);
> +	strbuf_release(sb);
> +
> +	return 0;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	size_t size = 1;
> +	struct strbuf sb;

The common theme in our source code seems to initialize using STRBUF_INIT... Let's use that paradigm here, too?

Show 5 quoted lines

> +	char str_test[5] = "test";
> +	char str_foo[7] = "foo";
> +
> +	if (argc != 2)
> +		usage("test-strbuf mode");

A nice and convenient way to do command-line parsing is to use the parse-options API, in this case with OPT_CMDMODE. This would also give us a chance to document the command modes in a nice and succinct way: as help strings.

> +
> +	if (!strcmp(argv[1], "basic_grow")) {
> +		/*
> +		 * Check if strbuf_grow(0) allocate a new NUL-terminated buffer

s/allocate/&s/

Show 17 quoted lines

> +		 */
> +		strbuf_init(&sb, 0);
> +		strbuf_grow(&sb, 0);
> +		if (sb.buf == strbuf_slopbuf)
> +			die("strbuf_grow failed to alloc memory");
> +		strbuf_release(&sb);
> +		if (sb.buf != strbuf_slopbuf)
> +			die("strbuf_release does not reinitialize the strbuf");
> +	} else if (!strcmp(argv[1], "strbuf_check_behavior")) {
> +		strbuf_init(&sb, 0);
> +		return test_usual(&sb);
> +	} else if (!strcmp(argv[1], "grow_overflow")) {
> +		/*
> +		 * size_t overflow: should die()
> +		 */
> +		strbuf_init(&sb, 1000);
> +		strbuf_grow(&sb, maximum_unsigned_value_of_type((size_t)1));

A comment "If this does not die(), fall through to returning success, to indicate an error" might be nice here.

Show 16 quoted lines

> +	} else {
> +		usage("test-strbuf mode");
> +	}
> +
> +	return 0;
> +}
> diff --git a/t/t0082-strbuf.sh b/t/t0082-strbuf.sh
> new file mode 100755
> index 0000000..0800d26
> --- /dev/null
> +++ b/t/t0082-strbuf.sh
> @@ -0,0 +1,19 @@
> +#!/bin/sh
> +
> +test_description="Test the strbuf API.
> +"

This description does not need a new-line, and existing one-liner test descriptions seem not to be terminated by a period.

The rest of this patch looks good.

Ciao, Johannes