git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/4] t/unit-tests: adapt example decorate test to clar framework

From
Patrick Steinhardt <ps@pks.im>
Date
Jan 31, 2025, 11:43 UTC
Message-ID
<Z5y3fI7o6ZXaSoas@pks.im>
In-Reply-To
<20250130091334.39922-3-kuforiji98@gmail.com>
On Thu, Jan 30, 2025 at 10:13:32AM +0100, Seyi Kuforiji wrote:
> Adapts example decorate test script to clar framework by using clar
> assertions where necessary. Test functions are created as standalone to
> test different test cases.

Again, I don't think that the second sentence doesn't add much information to the commit message and can probably just be dropped. It would be more interesting to talk about any design decisions that weren't immediately obvious, as these may also make the reviewer wonder.

One thing that would be worth to explain here are the `initialize()` and `cleanup()` functions. Readers not familiar with the clar framework may not know that those get picked up by the framework automatically and that they are executed before and after every single test.

Show 158 quoted lines
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> ---
>  Makefile                          |  2 +-
>  t/meson.build                     |  2 +-
>  t/unit-tests/t-example-decorate.c | 74 ------------------------------
>  t/unit-tests/u-example-decorate.c | 76 +++++++++++++++++++++++++++++++
>  4 files changed, 78 insertions(+), 76 deletions(-)
>  delete mode 100644 t/unit-tests/t-example-decorate.c
>  create mode 100644 t/unit-tests/u-example-decorate.c
> 
> diff --git a/Makefile b/Makefile
> index 2d9dad119a..732d765f1c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1338,6 +1338,7 @@ THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/%
>  THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/clar/%
>  
>  CLAR_TEST_SUITES += u-ctype
> +CLAR_TEST_SUITES += u-example-decorate
>  CLAR_TEST_SUITES += u-hash
>  CLAR_TEST_SUITES += u-hashmap
>  CLAR_TEST_SUITES += u-mem-pool
> @@ -1349,7 +1350,6 @@ CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
>  CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
>  CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
>  
> -UNIT_TEST_PROGRAMS += t-example-decorate
>  UNIT_TEST_PROGRAMS += t-oid-array
>  UNIT_TEST_PROGRAMS += t-oidmap
>  UNIT_TEST_PROGRAMS += t-oidtree
> diff --git a/t/meson.build b/t/meson.build
> index af597f9804..c7e08eca6f 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -1,5 +1,6 @@
>  clar_test_suites = [
>    'unit-tests/u-ctype.c',
> +  'unit-tests/u-example-decorate.c',
>    'unit-tests/u-hash.c',
>    'unit-tests/u-hashmap.c',
>    'unit-tests/u-mem-pool.c',
> @@ -45,7 +46,6 @@ clar_unit_tests = executable('unit-tests',
>  test('unit-tests', clar_unit_tests)
>  
>  unit_test_programs = [
> -  'unit-tests/t-example-decorate.c',
>    'unit-tests/t-oid-array.c',
>    'unit-tests/t-oidmap.c',
>    'unit-tests/t-oidtree.c',
> diff --git a/t/unit-tests/t-example-decorate.c b/t/unit-tests/t-example-decorate.c
> deleted file mode 100644
> index bfc776e223..0000000000
> --- a/t/unit-tests/t-example-decorate.c
> +++ /dev/null
> @@ -1,74 +0,0 @@
> -#define USE_THE_REPOSITORY_VARIABLE
> -
> -#include "test-lib.h"
> -#include "object.h"
> -#include "decorate.h"
> -#include "repository.h"
> -
> -struct test_vars {
> -	struct object *one, *two, *three;
> -	struct decoration n;
> -	int decoration_a, decoration_b;
> -};
> -
> -static void t_add(struct test_vars *vars)
> -{
> -	void *ret = add_decoration(&vars->n, vars->one, &vars->decoration_a);
> -
> -	check(ret == NULL);
> -	ret = add_decoration(&vars->n, vars->two, NULL);
> -	check(ret == NULL);
> -}
> -
> -static void t_readd(struct test_vars *vars)
> -{
> -	void *ret = add_decoration(&vars->n, vars->one, NULL);
> -
> -	check(ret == &vars->decoration_a);
> -	ret = add_decoration(&vars->n, vars->two, &vars->decoration_b);
> -	check(ret == NULL);
> -}
> -
> -static void t_lookup(struct test_vars *vars)
> -{
> -	void *ret = lookup_decoration(&vars->n, vars->one);
> -
> -	check(ret == NULL);
> -	ret = lookup_decoration(&vars->n, vars->two);
> -	check(ret == &vars->decoration_b);
> -	ret = lookup_decoration(&vars->n, vars->three);
> -	check(ret == NULL);
> -}
> -
> -static void t_loop(struct test_vars *vars)
> -{
> -	int objects_noticed = 0;
> -
> -	for (size_t i = 0; i < vars->n.size; i++) {
> -		if (vars->n.entries[i].base)
> -			objects_noticed++;
> -	}
> -	check_int(objects_noticed, ==, 2);
> -}
> -
> -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> -{
> -	struct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };
> -	struct test_vars vars = { 0 };
> -
> -	vars.one = lookup_unknown_object(the_repository, &one_oid);
> -	vars.two = lookup_unknown_object(the_repository, &two_oid);
> -	vars.three = lookup_unknown_object(the_repository, &three_oid);
> -
> -	TEST(t_add(&vars),
> -	     "Add 2 objects, one with a non-NULL decoration and one with a NULL decoration.");
> -	TEST(t_readd(&vars),
> -	     "When re-adding an already existing object, the old decoration is returned.");
> -	TEST(t_lookup(&vars),
> -	     "Lookup returns the added declarations, or NULL if the object was never added.");
> -	TEST(t_loop(&vars), "The user can also loop through all entries.");
> -
> -	clear_decoration(&vars.n, NULL);
> -
> -	return test_done();
> -}
> diff --git a/t/unit-tests/u-example-decorate.c b/t/unit-tests/u-example-decorate.c
> new file mode 100644
> index 0000000000..3a457d41fc
> --- /dev/null
> +++ b/t/unit-tests/u-example-decorate.c
> @@ -0,0 +1,76 @@
> +#define USE_THE_REPOSITORY_VARIABLE
> +
> +#include "unit-test.h"
> +#include "object.h"
> +#include "decorate.h"
> +#include "repository.h"
> +
> +struct test_vars {
> +	struct object *one, *two, *three;
> +	struct decoration n;
> +	int decoration_a, decoration_b;
> +};
> +
> +static struct test_vars vars;
> +
> +void test_example_decorate__add(void)
> +{
> +	void *ret = add_decoration(&vars.n, vars.one, &vars.decoration_a);
> +	cl_assert(ret == NULL);
> +	ret = add_decoration(&vars.n, vars.two, NULL);
> +	cl_assert(ret == NULL);
> +}
We could avoid the separate variable here, e.g. like this:
void test_example_decorate__add(void)
{
	cl_assert_equal_p(add_decoration(&vars.n, vars.one, &vars.decoration_a), NULL);
	cl_assert_equal_p(add_decoration(&vars.n, vars.two, NULL), NULL);
}
The same is true for some of the other test, too.

The test itself could be a bit more thorough in verifying what has been done, but that is not a new issue that your conversion introduces as the old testcase was somewhat light on that, too. So this is nothing that I think you need to change.

Show 6 quoted lines
> +void test_example_decorate__readd(void)
> +{
> +	void *ret;
> +
> +	cl_assert(add_decoration(&vars.n, vars.one, &vars.decoration_a) == NULL);
> +	cl_assert(add_decoration(&vars.n, vars.two, NULL) == NULL);

Okay. This wasn't present before because tests were actually dependent on the data that previous tests wrote. Now that we decouple the tests from one another we have to make sure to recreate the expected setup. Which I think is a good thing, as it makes tests fully self-contained and the setup easier to understand.

I guess this is another thing that you should be explaining in the commit message, as it is not obvious at all.

Show 30 quoted lines
> +	ret = add_decoration(&vars.n, vars.one, NULL);
> +	cl_assert(ret == &vars.decoration_a);
> +	ret = add_decoration(&vars.n, vars.two, &vars.decoration_b);
> +	cl_assert(ret == NULL);
> +}
> +
> +void test_example_decorate__lookup(void)
> +{
> +	void *ret;
> +
> +	add_decoration(&vars.n, vars.two, &vars.decoration_b);
> +	add_decoration(&vars.n, vars.one, NULL);
> +
> +	ret = lookup_decoration(&vars.n, vars.two);
> +	cl_assert(ret == &vars.decoration_b);
> +	ret = lookup_decoration(&vars.n, vars.one);
> +	cl_assert(ret == NULL);
> +}
> +
> +void test_example_decorate__loop(void)
> +{
> +	int objects_noticed = 0;
> +
> +	add_decoration(&vars.n, vars.one, &vars.decoration_a);
> +	add_decoration(&vars.n, vars.two, &vars.decoration_b);
> +
> +	for (size_t i = 0; i < vars.n.size; i++) {
> +		if (vars.n.entries[i].base)
> +			objects_noticed++;
> +	}
Nit: we can get rid of the curly braces here.
Show 16 quoted lines
> +	cl_assert_equal_i(objects_noticed, 2);
> +}
> +
> +void test_example_decorate__initialize(void)
> +{
> +	struct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };
> +
> +	vars.one = lookup_unknown_object(the_repository, &one_oid);
> +	vars.two = lookup_unknown_object(the_repository, &two_oid);
> +	vars.three = lookup_unknown_object(the_repository, &three_oid);
> +}
> +
> +void test_example_decorate__cleanup(void)
> +{
> +	clear_decoration(&vars.n, NULL);
> +}

Let's move both `initialize()` and `cleanup()` to the top so that it becomes easy to see that all tests use this commen setup.

Patrick
Previous: Seyi KuforijiNext: Seyi Kuforiji
Message 5 of 21 in “t/unit-tests: convert unit-tests to use clar”
  1. 0/4 t/unit-tests: convert unit-tests to use clarSeyi Kuforiji, Jan 30, 2025
  2. 1/4 t/unit-tests: convert hashmap test to clar frameworkSeyi Kuforiji, Jan 30, 2025
  3. Patrick SteinhardtJan 31, 2025
  4. 2/4 t/unit-tests: adapt example decorate test to clar frameworkSeyi Kuforiji, Jan 30, 2025
  5. Patrick SteinhardtJan 31, 2025
  6. 3/4 t/unit-tests: convert strbuf test to clar frameworkSeyi Kuforiji, Jan 30, 2025
  7. Patrick SteinhardtJan 31, 2025
  8. 4/4 t/unit-tests: convert strcmp-offset test to clar frameworkSeyi Kuforiji, Jan 30, 2025
  9. Patrick SteinhardtJan 31, 2025
  10. Patrick SteinhardtJan 31, 2025
  11. 0/4 t/unit-tests: convert unit-tests to use clarSeyi Kuforiji, Jan 31, 2025
  12. 1/4 t/unit-tests: convert hashmap test to use clar test frameworkSeyi Kuforiji, Jan 31, 2025
  13. Junio C HamanoJan 31, 2025
  14. phillip.wood123@gmail.comFeb 2, 2025
  15. Patrick SteinhardtFeb 3, 2025
  16. phillip.wood123@gmail.comFeb 3, 2025
  17. 2/4 t/unit-tests: adapt example decorate test to use clar test frameworkSeyi Kuforiji, Jan 31, 2025
  18. 3/4 t/unit-tests: convert strbuf test to use clar test frameworkSeyi Kuforiji, Jan 31, 2025
  19. Phillip WoodFeb 2, 2025
  20. 4/4 t/unit-tests: convert strcmp-offset test to use clar test frameworkSeyi Kuforiji, Jan 31, 2025
  21. Junio C HamanoJan 31, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.