diff mbox series

[v5,8/9] t/unit-tests: convert strvec tests to use clar

Message ID 8f56b4d6264b8e5a85ba73bc8ac541efa066c8a6.1723791831.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series Introduce clar testing framework | expand

Commit Message

Patrick Steinhardt Aug. 16, 2024, 7:04 a.m. UTC
Convert the strvec tests to use the new clar unit testing framework.
This is a first test balloon that demonstrates how the testing infra for
clar-based tests looks like.

The tests are part of the "t/unit-tests/bin/unit-tests" binary. When
running that binary, it generates TAP output:

    # ./t/unit-tests/bin/unit-tests
    TAP version 13
    # start of suite 1: strvec
    ok 1 - strvec::init
    ok 2 - strvec::dynamic_init
    ok 3 - strvec::clear
    ok 4 - strvec::push
    ok 5 - strvec::pushft_pushf
    ok 6 - strvec::pushl
    ok 7 - strvec::pushv
    ok 8 - strvec::replace_at_head
    ok 9 - strvec::replace_at_tail
    ok 10 - strvec::replace_in_between
    ok 11 - strvec::replace_with_substring
    ok 12 - strvec::remove_at_head
    ok 13 - strvec::remove_at_tail
    ok 14 - strvec::remove_in_between
    ok 15 - strvec::pop_empty_array
    ok 16 - strvec::pop_non_empty_array
    ok 17 - strvec::split_empty_string
    ok 18 - strvec::split_single_item
    ok 19 - strvec::split_multiple_items
    ok 20 - strvec::split_whitespace_only
    ok 21 - strvec::split_multiple_consecutive_whitespaces
    ok 22 - strvec::detach
    1..22

The binary also supports some parameters that allow us to run only a
subset of unit tests or alter the output:

    $ ./t/unit-tests/bin/unit-tests -h
    Usage: ./t/unit-tests/bin/unit-tests [options]

    Options:
      -sname        Run only the suite with `name` (can go to individual test name)
      -iname        Include the suite with `name`
      -xname        Exclude the suite with `name`
      -v            Increase verbosity (show suite names)
      -q            Only report tests that had an error
      -Q            Quit as soon as a test fails
      -t            Display results in tap format
      -l            Print suite names
      -r[filename]  Write summary file (to the optional filename)

Furthermore, running `make unit-tests` runs the binary along with all
the other unit tests we have.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Makefile                              |   2 +-
 t/unit-tests/{t-strvec.c => strvec.c} | 119 ++++++++++----------------
 t/unit-tests/unit-test.c              |   3 +-
 3 files changed, 45 insertions(+), 79 deletions(-)
 rename t/unit-tests/{t-strvec.c => strvec.c} (54%)

Comments

Phillip Wood Aug. 16, 2024, 1:38 p.m. UTC | #1
Hi Patrick

On 16/08/2024 08:04, Patrick Steinhardt wrote:
> diff --git a/t/unit-tests/unit-test.c b/t/unit-tests/unit-test.c
> index 32a81299e9..82b7635e6a 100644
> --- a/t/unit-tests/unit-test.c
> +++ b/t/unit-tests/unit-test.c
> @@ -6,10 +6,9 @@ int cmd_main(int argc, const char **argv)
>   	int ret;
>   
>   	/* Append the "-t" flag such that the tests generate TAP output. */
> -	ALLOC_ARRAY(argv_copy, argc + 2);
> +	ALLOC_ARRAY(argv_copy, argc + 1);
>   	COPY_ARRAY(argv_copy, argv, argc);
>   	argv_copy[argc++] = "-t";
> -	argv_copy[argc] = NULL;

I'm confused by this - it looks like it belongs in the previous patch 
but why are we deleting the line that adds the terminating NULL to argv 
in the first place?

Thanks

Phillip
Junio C Hamano Aug. 16, 2024, 4:11 p.m. UTC | #2
Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Patrick
>
> On 16/08/2024 08:04, Patrick Steinhardt wrote:
>> diff --git a/t/unit-tests/unit-test.c b/t/unit-tests/unit-test.c
>> index 32a81299e9..82b7635e6a 100644
>> --- a/t/unit-tests/unit-test.c
>> +++ b/t/unit-tests/unit-test.c
>> @@ -6,10 +6,9 @@ int cmd_main(int argc, const char **argv)
>>   	int ret;
>>     	/* Append the "-t" flag such that the tests generate TAP
>> output. */
>> -	ALLOC_ARRAY(argv_copy, argc + 2);
>> +	ALLOC_ARRAY(argv_copy, argc + 1);
>>   	COPY_ARRAY(argv_copy, argv, argc);
>>   	argv_copy[argc++] = "-t";
>> -	argv_copy[argc] = NULL;
>
> I'm confused by this - it looks like it belongs in the previous patch
> but why are we deleting the line that adds the terminating NULL to
> argv in the first place?

I agree with you that it belongs to the previous step.  clar_test(),
unlike the usual convention used by main(), works solely with argc
to tell where the list of argument ends, without requiring argv[] to
be an NULL terminated array.  I do not know if clar _promises_ that
as the calling convention to its users, but if it does so, and if we
want to take advantage of that promise and leave the terminating
NULL out of argv[], we should do so from the beginning.

A NULL-terminated argv[], as long as we make sure argc only counts
the real arguments without counting that terminating NULL, would not
harm the clar_test(argc, argv) call, so I actually do not mind the
way this caller was originally written, with terminating NULL.  It
looks a lot more familiar, even though we may be wasting memory at
the end of argv[] array.
Patrick Steinhardt Aug. 20, 2024, 12:59 p.m. UTC | #3
On Fri, Aug 16, 2024 at 09:11:36AM -0700, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > Hi Patrick
> >
> > On 16/08/2024 08:04, Patrick Steinhardt wrote:
> >> diff --git a/t/unit-tests/unit-test.c b/t/unit-tests/unit-test.c
> >> index 32a81299e9..82b7635e6a 100644
> >> --- a/t/unit-tests/unit-test.c
> >> +++ b/t/unit-tests/unit-test.c
> >> @@ -6,10 +6,9 @@ int cmd_main(int argc, const char **argv)
> >>   	int ret;
> >>     	/* Append the "-t" flag such that the tests generate TAP
> >> output. */
> >> -	ALLOC_ARRAY(argv_copy, argc + 2);
> >> +	ALLOC_ARRAY(argv_copy, argc + 1);
> >>   	COPY_ARRAY(argv_copy, argv, argc);
> >>   	argv_copy[argc++] = "-t";
> >> -	argv_copy[argc] = NULL;
> >
> > I'm confused by this - it looks like it belongs in the previous patch
> > but why are we deleting the line that adds the terminating NULL to
> > argv in the first place?
> 
> I agree with you that it belongs to the previous step.  clar_test(),
> unlike the usual convention used by main(), works solely with argc
> to tell where the list of argument ends, without requiring argv[] to
> be an NULL terminated array.  I do not know if clar _promises_ that
> as the calling convention to its users, but if it does so, and if we
> want to take advantage of that promise and leave the terminating
> NULL out of argv[], we should do so from the beginning.

Agreed.

> A NULL-terminated argv[], as long as we make sure argc only counts
> the real arguments without counting that terminating NULL, would not
> harm the clar_test(argc, argv) call, so I actually do not mind the
> way this caller was originally written, with terminating NULL.  It
> looks a lot more familiar, even though we may be wasting memory at
> the end of argv[] array.

Okay. It's not necessary, but I don't mind it much, either.

Patrick
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index e6206ca8c7..b0c9a79c55 100644
--- a/Makefile
+++ b/Makefile
@@ -1336,6 +1336,7 @@  THIRD_PARTY_SOURCES += sha1dc/%
 THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/%
 THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/clar/%
 
+UNIT_TESTS_SUITES += strvec
 UNIT_TESTS_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
 UNIT_TESTS_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TESTS_SUITES))
 UNIT_TESTS_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
@@ -1353,7 +1354,6 @@  UNIT_TEST_PROGRAMS += t-reftable-merged
 UNIT_TEST_PROGRAMS += t-reftable-record
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
-UNIT_TEST_PROGRAMS += t-strvec
 UNIT_TEST_PROGRAMS += t-trailer
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
diff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/strvec.c
similarity index 54%
rename from t/unit-tests/t-strvec.c
rename to t/unit-tests/strvec.c
index fa1a041469..d11ed0f28d 100644
--- a/t/unit-tests/t-strvec.c
+++ b/t/unit-tests/strvec.c
@@ -1,52 +1,46 @@ 
-#include "test-lib.h"
+#include "unit-test.h"
 #include "strbuf.h"
 #include "strvec.h"
 
 #define check_strvec(vec, ...) \
 	do { \
 		const char *expect[] = { __VA_ARGS__ }; \
-		if (check_uint(ARRAY_SIZE(expect), >, 0) && \
-		    check_pointer_eq(expect[ARRAY_SIZE(expect) - 1], NULL) && \
-		    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \
-		    check_uint((vec)->nr, <=, (vec)->alloc)) { \
-			for (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \
-				if (!check_str((vec)->v[i], expect[i])) { \
-					test_msg("      i: %"PRIuMAX, \
-						 (uintmax_t)i); \
-					break; \
-				} \
-			} \
-		} \
+		cl_assert(ARRAY_SIZE(expect) > 0); \
+		cl_assert_equal_p(expect[ARRAY_SIZE(expect) - 1], NULL); \
+		cl_assert_equal_i((vec)->nr, ARRAY_SIZE(expect) - 1); \
+		cl_assert((vec)->nr <= (vec)->alloc); \
+		for (size_t i = 0; i < ARRAY_SIZE(expect); i++) \
+			cl_assert_equal_s((vec)->v[i], expect[i]); \
 	} while (0)
 
-static void t_static_init(void)
+void test_strvec__init(void)
 {
 	struct strvec vec = STRVEC_INIT;
-	check_pointer_eq(vec.v, empty_strvec);
-	check_uint(vec.nr, ==, 0);
-	check_uint(vec.alloc, ==, 0);
+	cl_assert_equal_p(vec.v, empty_strvec);
+	cl_assert_equal_i(vec.nr, 0);
+	cl_assert_equal_i(vec.alloc, 0);
 }
 
-static void t_dynamic_init(void)
+void test_strvec__dynamic_init(void)
 {
 	struct strvec vec;
 	strvec_init(&vec);
-	check_pointer_eq(vec.v, empty_strvec);
-	check_uint(vec.nr, ==, 0);
-	check_uint(vec.alloc, ==, 0);
+	cl_assert_equal_p(vec.v, empty_strvec);
+	cl_assert_equal_i(vec.nr, 0);
+	cl_assert_equal_i(vec.alloc, 0);
 }
 
-static void t_clear(void)
+void test_strvec__clear(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_push(&vec, "foo");
 	strvec_clear(&vec);
-	check_pointer_eq(vec.v, empty_strvec);
-	check_uint(vec.nr, ==, 0);
-	check_uint(vec.alloc, ==, 0);
+	cl_assert_equal_p(vec.v, empty_strvec);
+	cl_assert_equal_i(vec.nr, 0);
+	cl_assert_equal_i(vec.alloc, 0);
 }
 
-static void t_push(void)
+void test_strvec__push(void)
 {
 	struct strvec vec = STRVEC_INIT;
 
@@ -59,7 +53,7 @@  static void t_push(void)
 	strvec_clear(&vec);
 }
 
-static void t_pushf(void)
+void test_strvec__pushf(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_pushf(&vec, "foo: %d", 1);
@@ -67,7 +61,7 @@  static void t_pushf(void)
 	strvec_clear(&vec);
 }
 
-static void t_pushl(void)
+void test_strvec__pushl(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
@@ -75,7 +69,7 @@  static void t_pushl(void)
 	strvec_clear(&vec);
 }
 
-static void t_pushv(void)
+void test_strvec__pushv(void)
 {
 	const char *strings[] = {
 		"foo", "bar", "baz", NULL,
@@ -88,7 +82,7 @@  static void t_pushv(void)
 	strvec_clear(&vec);
 }
 
-static void t_replace_at_head(void)
+void test_strvec__replace_at_head(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
@@ -97,7 +91,7 @@  static void t_replace_at_head(void)
 	strvec_clear(&vec);
 }
 
-static void t_replace_at_tail(void)
+void test_strvec__replace_at_tail(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
@@ -106,7 +100,7 @@  static void t_replace_at_tail(void)
 	strvec_clear(&vec);
 }
 
-static void t_replace_in_between(void)
+void test_strvec__replace_in_between(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
@@ -115,7 +109,7 @@  static void t_replace_in_between(void)
 	strvec_clear(&vec);
 }
 
-static void t_replace_with_substring(void)
+void test_strvec__replace_with_substring(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_pushl(&vec, "foo", NULL);
@@ -124,7 +118,7 @@  static void t_replace_with_substring(void)
 	strvec_clear(&vec);
 }
 
-static void t_remove_at_head(void)
+void test_strvec__remove_at_head(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
@@ -133,7 +127,7 @@  static void t_remove_at_head(void)
 	strvec_clear(&vec);
 }
 
-static void t_remove_at_tail(void)
+void test_strvec__remove_at_tail(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
@@ -142,7 +136,7 @@  static void t_remove_at_tail(void)
 	strvec_clear(&vec);
 }
 
-static void t_remove_in_between(void)
+void test_strvec__remove_in_between(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
@@ -151,7 +145,7 @@  static void t_remove_in_between(void)
 	strvec_clear(&vec);
 }
 
-static void t_pop_empty_array(void)
+void test_strvec__pop_empty_array(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_pop(&vec);
@@ -159,7 +153,7 @@  static void t_pop_empty_array(void)
 	strvec_clear(&vec);
 }
 
-static void t_pop_non_empty_array(void)
+void test_strvec__pop_non_empty_array(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
@@ -168,7 +162,7 @@  static void t_pop_non_empty_array(void)
 	strvec_clear(&vec);
 }
 
-static void t_split_empty_string(void)
+void test_strvec__split_empty_string(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_split(&vec, "");
@@ -176,7 +170,7 @@  static void t_split_empty_string(void)
 	strvec_clear(&vec);
 }
 
-static void t_split_single_item(void)
+void test_strvec__split_single_item(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_split(&vec, "foo");
@@ -184,7 +178,7 @@  static void t_split_single_item(void)
 	strvec_clear(&vec);
 }
 
-static void t_split_multiple_items(void)
+void test_strvec__split_multiple_items(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_split(&vec, "foo bar baz");
@@ -192,7 +186,7 @@  static void t_split_multiple_items(void)
 	strvec_clear(&vec);
 }
 
-static void t_split_whitespace_only(void)
+void test_strvec__split_whitespace_only(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_split(&vec, " \t\n");
@@ -200,7 +194,7 @@  static void t_split_whitespace_only(void)
 	strvec_clear(&vec);
 }
 
-static void t_split_multiple_consecutive_whitespaces(void)
+void test_strvec__split_multiple_consecutive_whitespaces(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	strvec_split(&vec, "foo\n\t bar");
@@ -208,7 +202,7 @@  static void t_split_multiple_consecutive_whitespaces(void)
 	strvec_clear(&vec);
 }
 
-static void t_detach(void)
+void test_strvec__detach(void)
 {
 	struct strvec vec = STRVEC_INIT;
 	const char **detached;
@@ -216,40 +210,13 @@  static void t_detach(void)
 	strvec_push(&vec, "foo");
 
 	detached = strvec_detach(&vec);
-	check_str(detached[0], "foo");
-	check_pointer_eq(detached[1], NULL);
+	cl_assert_equal_s(detached[0], "foo");
+	cl_assert_equal_p(detached[1], NULL);
 
-	check_pointer_eq(vec.v, empty_strvec);
-	check_uint(vec.nr, ==, 0);
-	check_uint(vec.alloc, ==, 0);
+	cl_assert_equal_p(vec.v, empty_strvec);
+	cl_assert_equal_i(vec.nr, 0);
+	cl_assert_equal_i(vec.alloc, 0);
 
 	free((char *) detached[0]);
 	free(detached);
 }
-
-int cmd_main(int argc, const char **argv)
-{
-	TEST(t_static_init(), "static initialization");
-	TEST(t_dynamic_init(), "dynamic initialization");
-	TEST(t_clear(), "clear");
-	TEST(t_push(), "push");
-	TEST(t_pushf(), "pushf");
-	TEST(t_pushl(), "pushl");
-	TEST(t_pushv(), "pushv");
-	TEST(t_replace_at_head(), "replace at head");
-	TEST(t_replace_in_between(), "replace in between");
-	TEST(t_replace_at_tail(), "replace at tail");
-	TEST(t_replace_with_substring(), "replace with substring");
-	TEST(t_remove_at_head(), "remove at head");
-	TEST(t_remove_in_between(), "remove in between");
-	TEST(t_remove_at_tail(), "remove at tail");
-	TEST(t_pop_empty_array(), "pop with empty array");
-	TEST(t_pop_non_empty_array(), "pop with non-empty array");
-	TEST(t_split_empty_string(), "split empty string");
-	TEST(t_split_single_item(), "split single item");
-	TEST(t_split_multiple_items(), "split multiple items");
-	TEST(t_split_whitespace_only(), "split whitespace only");
-	TEST(t_split_multiple_consecutive_whitespaces(), "split multiple consecutive whitespaces");
-	TEST(t_detach(), "detach");
-	return test_done();
-}
diff --git a/t/unit-tests/unit-test.c b/t/unit-tests/unit-test.c
index 32a81299e9..82b7635e6a 100644
--- a/t/unit-tests/unit-test.c
+++ b/t/unit-tests/unit-test.c
@@ -6,10 +6,9 @@  int cmd_main(int argc, const char **argv)
 	int ret;
 
 	/* Append the "-t" flag such that the tests generate TAP output. */
-	ALLOC_ARRAY(argv_copy, argc + 2);
+	ALLOC_ARRAY(argv_copy, argc + 1);
 	COPY_ARRAY(argv_copy, argv, argc);
 	argv_copy[argc++] = "-t";
-	argv_copy[argc] = NULL;
 
 	ret = clar_test(argc, (char **) argv_copy);