Re: [PATCH v7 00/14] Introduce clar testing framework

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Patrick

On 03/09/2024 10:14, Patrick Steinhardt wrote:
> Hi,
> 
> this is the seventh version fo my patch series that introduces the clar
> unit testing framework.
> 
> Changes compared to v6:
> 
>    - Add some explanations for why we introduce clar in the first place
>      to the second commit message.
> 
>    - Split out implementation of the test driver, that is the main
>      function, into a separate commit.
> 
>    - Reimplement argument parsing ourselves. This allows more flexibility
>      and gets rid of some options that do not make sense for us.
> 
>    - Add an empty line between declarations and code.
> 
>    - Improve the test messages in ctype tests to also mention our
>      expectations.
> 
>    - Adapt `cl_failf()` to not use `xstrfmt()`, but `snprintf()` with a
>      static buffer instead.

I've left some comments on patch 11 which is new. Apart from that 
range-diff looks fine to me.

Thanks for working on this

Phillip

> Thanks!
> 
> Patrick
> 
> Johannes Schindelin (4):
>    clar: avoid compile error with mingw-w64
>    clar(win32): avoid compile error due to unused `fs_copy()`
>    clar: stop including `shellapi.h` unnecessarily
>    clar: add CMake support
> 
> Patrick Steinhardt (10):
>    t: do not pass GIT_TEST_OPTS to unit tests with prove
>    t: import the clar unit testing framework
>    t/clar: fix compatibility with NonStop
>    Makefile: fix sparse dependency on GENERATED_H
>    Makefile: make hdr-check depend on generated headers
>    Makefile: do not use sparse on third-party sources
>    Makefile: wire up the clar unit testing framework
>    t/unit-tests: implement test driver
>    t/unit-tests: convert strvec tests to use clar
>    t/unit-tests: convert ctype tests to use clar
> 
>   .gitignore                                 |   1 +
>   Documentation/technical/unit-tests.txt     |   2 +
>   Makefile                                   |  53 +-
>   contrib/buildsystems/CMakeLists.txt        |  53 ++
>   t/Makefile                                 |   4 +-
>   t/run-test.sh                              |   2 +-
>   t/unit-tests/.gitignore                    |   2 +
>   t/unit-tests/clar-generate.awk             |  50 ++
>   t/unit-tests/clar/.github/workflows/ci.yml |  23 +
>   t/unit-tests/clar/COPYING                  |  15 +
>   t/unit-tests/clar/README.md                | 329 ++++++++
>   t/unit-tests/clar/clar.c                   | 842 +++++++++++++++++++++
>   t/unit-tests/clar/clar.h                   | 173 +++++
>   t/unit-tests/clar/clar/fixtures.h          |  50 ++
>   t/unit-tests/clar/clar/fs.h                | 524 +++++++++++++
>   t/unit-tests/clar/clar/print.h             | 211 ++++++
>   t/unit-tests/clar/clar/sandbox.h           | 159 ++++
>   t/unit-tests/clar/clar/summary.h           | 143 ++++
>   t/unit-tests/clar/generate.py              | 266 +++++++
>   t/unit-tests/clar/test/.gitignore          |   4 +
>   t/unit-tests/clar/test/Makefile            |  39 +
>   t/unit-tests/clar/test/clar_test.h         |  16 +
>   t/unit-tests/clar/test/main.c              |  40 +
>   t/unit-tests/clar/test/main.c.sample       |  27 +
>   t/unit-tests/clar/test/resources/test/file |   1 +
>   t/unit-tests/clar/test/sample.c            |  84 ++
>   t/unit-tests/{t-ctype.c => ctype.c}        |  71 +-
>   t/unit-tests/strvec.c                      | 241 ++++++
>   t/unit-tests/t-strvec.c                    | 211 ------
>   t/unit-tests/unit-test.c                   |  45 ++
>   t/unit-tests/unit-test.h                   |  10 +
>   31 files changed, 3457 insertions(+), 234 deletions(-)
>   create mode 100644 t/unit-tests/clar-generate.awk
>   create mode 100644 t/unit-tests/clar/.github/workflows/ci.yml
>   create mode 100644 t/unit-tests/clar/COPYING
>   create mode 100644 t/unit-tests/clar/README.md
>   create mode 100644 t/unit-tests/clar/clar.c
>   create mode 100644 t/unit-tests/clar/clar.h
>   create mode 100644 t/unit-tests/clar/clar/fixtures.h
>   create mode 100644 t/unit-tests/clar/clar/fs.h
>   create mode 100644 t/unit-tests/clar/clar/print.h
>   create mode 100644 t/unit-tests/clar/clar/sandbox.h
>   create mode 100644 t/unit-tests/clar/clar/summary.h
>   create mode 100755 t/unit-tests/clar/generate.py
>   create mode 100644 t/unit-tests/clar/test/.gitignore
>   create mode 100644 t/unit-tests/clar/test/Makefile
>   create mode 100644 t/unit-tests/clar/test/clar_test.h
>   create mode 100644 t/unit-tests/clar/test/main.c
>   create mode 100644 t/unit-tests/clar/test/main.c.sample
>   create mode 100644 t/unit-tests/clar/test/resources/test/file
>   create mode 100644 t/unit-tests/clar/test/sample.c
>   rename t/unit-tests/{t-ctype.c => ctype.c} (68%)
>   create mode 100644 t/unit-tests/strvec.c
>   delete mode 100644 t/unit-tests/t-strvec.c
>   create mode 100644 t/unit-tests/unit-test.c
>   create mode 100644 t/unit-tests/unit-test.h
> 
> Range-diff against v6:
>   1:  e48a6461137 =  1:  b67f10ec0b0 t: do not pass GIT_TEST_OPTS to unit tests with prove
>   2:  1710e9f9ff7 !  2:  55a9b46e65f t: import the clar unit testing framework
>      @@ Metadata
>        ## Commit message ##
>           t: import the clar unit testing framework
>       
>      +    Our unit testing framework is a homegrown solution. While it supports
>      +    most of our needs, it is likely that the volume of unit tests will grow
>      +    quite a bit in the future such that we can exercise low-level subsystems
>      +    directly. This surfaces several shortcomings that the current solution
>      +    has:
>      +
>      +      - There is no way to run only one specific tests. While some of our
>      +        unit tests wire this up manually, others don't. In general, it
>      +        requires quite a bit of boilerplate to get this set up correctly.
>      +
>      +      - Failures do not cause a test to stop execution directly. Instead,
>      +        the test author needs to return manually whenever an assertion
>      +        fails. This is rather verbose and is not done correctly in most of
>      +        our unit tests.
>      +
>      +      - Wiring up a new testcase requires both implementing the test
>      +        function and calling it in the respective test suite's main
>      +        function, which is creating code duplication.
>      +
>      +    We can of course fix all of these issues ourselves, but that feels
>      +    rather pointless when there are already so many unit testing frameworks
>      +    out there that have those features.
>      +
>      +    We line out some requirements for any unit testing framework in
>      +    "Documentation/technical/unit-tests.txt". The "clar" unit testing
>      +    framework, which isn't listed in that table yet, ticks many of the
>      +    boxes:
>      +
>      +      - It is licensed under ISC, which is compatible.
>      +
>      +      - It is easily vendorable because it is rather tiny at around 1200
>      +        lines of code.
>      +
>      +      - It is easily hackable due to the same reason.
>      +
>      +      - It has TAP support.
>      +
>      +      - It has skippable tests.
>      +
>      +      - It preprocesses test files in order to extract test functions, which
>      +        then get wired up automatically.
>      +
>      +    While it's not perfect, the fact that clar originates from the libgit2
>      +    project means that it should be rather easy for us to collaborate with
>      +    upstream to plug any gaps.
>      +
>           Import the clar unit testing framework at commit 1516124 (Merge pull
>           request #97 from pks-t/pks-whitespace-fixes, 2024-08-15). The framework
>           will be wired up in subsequent commits.
>   3:  5c21aa87aa2 =  3:  f24401f0a87 t/clar: fix compatibility with NonStop
>   4:  06d2bce0d82 =  4:  658a601c541 clar: avoid compile error with mingw-w64
>   5:  f88b3421a09 =  5:  0b8a6ac5fed clar(win32): avoid compile error due to unused `fs_copy()`
>   6:  5fb4c55be33 =  6:  c50e7a0ea68 clar: stop including `shellapi.h` unnecessarily
>   7:  e0dcbd5ca83 =  7:  b8f3f16dd27 Makefile: fix sparse dependency on GENERATED_H
>   8:  77a03f8df70 =  8:  3d3fe443b9a Makefile: make hdr-check depend on generated headers
>   9:  c91dd7327e3 =  9:  7d0f494850a Makefile: do not use sparse on third-party sources
> 10:  115c15aa9ae ! 10:  9c74c5ae019 Makefile: wire up the clar unit testing framework
>      @@ Commit message
>           anything. Thus, it would cause a compiler error if a function name was
>           mistyped and thus not picked up by "generate.py".
>       
>      +    The test driver "unit-test.c" is an empty stub for now. It will get
>      +    implemented in the next commit.
>      +
>           Signed-off-by: Patrick Steinhardt <ps@pks.im>
>       
>        ## .gitignore ##
>      @@ t/unit-tests/unit-test.c (new)
>       @@
>       +#include "unit-test.h"
>       +
>      -+int cmd_main(int argc, const char **argv)
>      ++int cmd_main(int argc UNUSED, const char **argv UNUSED)
>       +{
>      -+	const char **argv_copy;
>      -+	int ret;
>      -+
>      -+	/* Append the "-t" flag such that the tests generate TAP output. */
>      -+	ALLOC_ARRAY(argv_copy, argc + 2);
>      -+	COPY_ARRAY(argv_copy, argv, argc);
>      -+	argv_copy[argc++] = "-t";
>      -+	argv_copy[argc] = NULL;
>      -+
>      -+	ret = clar_test(argc, (char **) argv_copy);
>      -+
>      -+	free(argv_copy);
>      -+	return ret;
>      ++	return 0;
>       +}
>       
>        ## t/unit-tests/unit-test.h (new) ##
>   -:  ----------- > 11:  8bd5b3e2b29 t/unit-tests: implement test driver
> 11:  b3b8df04872 ! 12:  3c3b9eacdfb t/unit-tests: convert strvec tests to use clar
>      @@ Commit message
>           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:
>      +    running that binary with an injected error, it generates TAP output:
>       
>               # ./t/unit-tests/bin/unit-tests
>               TAP version 13
>      @@ Commit message
>               ok 1 - strvec::init
>               ok 2 - strvec::dynamic_init
>               ok 3 - strvec::clear
>      -        ok 4 - strvec::push
>      -        ok 5 - strvec::pushft_pushf
>      +        not ok 4 - strvec::push
>      +            ---
>      +            reason: |
>      +              String mismatch: (&vec)->v[i] != expect[i]
>      +              'foo' != 'fo' (at byte 2)
>      +            at:
>      +              file: 't/unit-tests/strvec.c'
>      +              line: 48
>      +              function: 'test_strvec__push'
>      +            ---
>      +        ok 5 - strvec::pushf
>               ok 6 - strvec::pushl
>               ok 7 - strvec::pushv
>               ok 8 - strvec::replace_at_head
>      @@ t/unit-tests/strvec.c (new)
>       +#define check_strvec(vec, ...) \
>       +	do { \
>       +		const char *expect[] = { __VA_ARGS__ }; \
>      -+		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); \
>      ++		size_t expect_len = ARRAY_SIZE(expect); \
>      ++		cl_assert(expect_len > 0); \
>      ++		cl_assert_equal_p(expect[expect_len - 1], NULL); \
>      ++		cl_assert_equal_i((vec)->nr, expect_len - 1); \
>       +		cl_assert((vec)->nr <= (vec)->alloc); \
>      -+		for (size_t i = 0; i < ARRAY_SIZE(expect); i++) \
>      ++		for (size_t i = 0; i < expect_len; i++) \
>       +			cl_assert_equal_s((vec)->v[i], expect[i]); \
>       +	} while (0)
>       +
>       +void test_strvec__init(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	cl_assert_equal_p(vec.v, empty_strvec);
>       +	cl_assert_equal_i(vec.nr, 0);
>       +	cl_assert_equal_i(vec.alloc, 0);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__dynamic_init(void)
>       +{
>       +	struct strvec vec;
>      ++
>       +	strvec_init(&vec);
>       +	cl_assert_equal_p(vec.v, empty_strvec);
>       +	cl_assert_equal_i(vec.nr, 0);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__clear(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_push(&vec, "foo");
>       +	strvec_clear(&vec);
>       +	cl_assert_equal_p(vec.v, empty_strvec);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__pushf(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_pushf(&vec, "foo: %d", 1);
>       +	check_strvec(&vec, "foo: 1", NULL);
>       +	strvec_clear(&vec);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__pushl(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
>       +	check_strvec(&vec, "foo", "bar", "baz", NULL);
>       +	strvec_clear(&vec);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__replace_at_head(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
>       +	strvec_replace(&vec, 0, "replaced");
>       +	check_strvec(&vec, "replaced", "bar", "baz", NULL);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__replace_in_between(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
>       +	strvec_replace(&vec, 1, "replaced");
>       +	check_strvec(&vec, "foo", "replaced", "baz", NULL);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__replace_with_substring(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_pushl(&vec, "foo", NULL);
>       +	strvec_replace(&vec, 0, vec.v[0] + 1);
>       +	check_strvec(&vec, "oo", NULL);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__remove_at_head(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
>       +	strvec_remove(&vec, 0);
>       +	check_strvec(&vec, "bar", "baz", NULL);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__remove_at_tail(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
>       +	strvec_remove(&vec, 2);
>       +	check_strvec(&vec, "foo", "bar", NULL);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__remove_in_between(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
>       +	strvec_remove(&vec, 1);
>       +	check_strvec(&vec, "foo", "baz", NULL);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__pop_empty_array(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_pop(&vec);
>       +	check_strvec(&vec, NULL);
>       +	strvec_clear(&vec);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__pop_non_empty_array(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
>       +	strvec_pop(&vec);
>       +	check_strvec(&vec, "foo", "bar", NULL);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__split_empty_string(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_split(&vec, "");
>       +	check_strvec(&vec, NULL);
>       +	strvec_clear(&vec);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__split_single_item(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_split(&vec, "foo");
>       +	check_strvec(&vec, "foo", NULL);
>       +	strvec_clear(&vec);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__split_multiple_items(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_split(&vec, "foo bar baz");
>       +	check_strvec(&vec, "foo", "bar", "baz", NULL);
>       +	strvec_clear(&vec);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__split_whitespace_only(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_split(&vec, " \t\n");
>       +	check_strvec(&vec, NULL);
>       +	strvec_clear(&vec);
>      @@ t/unit-tests/strvec.c (new)
>       +void test_strvec__split_multiple_consecutive_whitespaces(void)
>       +{
>       +	struct strvec vec = STRVEC_INIT;
>      ++
>       +	strvec_split(&vec, "foo\n\t bar");
>       +	check_strvec(&vec, "foo", "bar", NULL);
>       +	strvec_clear(&vec);
> 12:  1ac2e48a7f2 ! 13:  c8360db2f86 t/unit-tests: convert ctype tests to use clar
>      @@ Commit message
>           on failure:
>       
>               # start of suite 1: ctype
>      -        ok 1 - ctype::isspace
>      -        not ok 2 - ctype::isdigit
>      +        not ok 1 - ctype::isspace
>                   ---
>                   reason: |
>                     Test failed.
>      -              0x61 is classified incorrectly
>      +              0x0d is classified incorrectly: expected 0, got 1
>                   at:
>                     file: 't/unit-tests/ctype.c'
>      -              line: 38
>      -              function: 'test_ctype__isdigit'
>      +              line: 36
>      +              function: 'test_ctype__isspace'
>                   ---
>      +        ok 2 - ctype::isdigit
>               ok 3 - ctype::isalpha
>               ok 4 - ctype::isalnum
>               ok 5 - ctype::is_glob_special
>      @@ t/unit-tests/t-ctype.c => t/unit-tests/ctype.c
>       -				test_msg("      i: 0x%02x", i); \
>       -		} \
>       -		check(!class(EOF)); \
>      --	} \
>      -+	for (int i = 0; i < 256; i++) \
>      -+		if (class(i) != !!memchr(string, i, len)) \
>      -+			cl_failf("0x%02x is classified incorrectly", i); \
>      ++	for (int i = 0; i < 256; i++) { \
>      ++		int actual = class(i), expect = !!memchr(string, i, len); \
>      ++		if (actual != expect) \
>      ++			cl_failf("0x%02x is classified incorrectly: expected %d, got %d", \
>      ++				 i, expect, actual); \
>      + 	} \
>       +	cl_assert(!class(EOF)); \
>        } while (0)
>        
>      @@ t/unit-tests/unit-test.h
>       +#include "strbuf.h"
>       +
>       +#define cl_failf(fmt, ...) do { \
>      -+	char *desc = xstrfmt(fmt, __VA_ARGS__); \
>      ++	char desc[4096]; \
>      ++	snprintf(desc, sizeof(desc), fmt, __VA_ARGS__); \
>       +	clar__fail(__FILE__, __func__, __LINE__, "Test failed.", desc, 1); \
>      -+	free(desc); \
>       +} while (0)
> 13:  131036c398e = 14:  d51c146cd9d clar: add CMake support