[GSoC,v5,0/5] t: port reftable/tree_test.c to the unit testing framework

Message

Chandra Pratap July 22, 2024, 5:57 a.m. UTC

The reftable library comes with self tests, which are exercised
as part of the usual end-to-end tests and are designed to
observe the end-user visible effects of Git commands. What it
exercises, however, is a better match for the unit-testing
framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',
2023-12-09), which is designed to observe how low level
implementation details, at the level of sequences of individual
function calls, behave.

Hence, port reftable/tree_test.c to the unit testing framework and
improve upon the ported test. The first patch in the series is
preparatory cleanup, the second patch moves the test to the unit
testing framework, and the rest of the patches improve upon the
ported test.

Mentored-by: Patrick Steinhardt <[e-mail]>
Mentored-by: Christian Couder <[e-mail]>
Signed-off-by: Chandra Pratap <[e-mail]>

---
Changes in v5:
- Rebase the branch on top of the latest master
- Add more explanation in the commit message of patch 2
- Refer to function pointers as 'func' and not '&func'
- Add comments and refactor the test in patch 2 for easier
  comprehension

CI/PR: https://github.com/gitgitgadget/git/pull/1740

Chandra Pratap(5):
reftable: remove unnecessary curly braces in reftable/tree.c
t: move reftable/tree_test.c to the unit testing framework
t-reftable-tree: split test_tree() into two sub-test
t-reftable-tree: add test for non-existent key
t-reftable-tree: improve the test for infix_walk()

Makefile                       |  2 +-
reftable/reftable-tests.h      |  1 -
reftable/tree.c                | 15 +++-------
reftable/tree_test.c           | 60 ----------------------
t/helper/test-reftable.c       |  1 -
t/unit-tests/t-reftable-tree.c | 83 +++++++++++++++++++++++++++++++++++++
6 files changed, 89 insertions(+), 73 deletions(-)

Range-diff against v4:
<rebase commits>
1:  2be2a35b7f = 45:  1d637f7686 reftable: remove unnecessary curly braces in reftable/tree.c
2:  de49698ea7 ! 46:  7401e2409f t: move reftable/tree_test.c to the unit testing framework
   @@ Commit message
        the unit testing framework instead of reftable's test framework and
        renaming the tests to align with unit-tests' standards.

   +    Also add a comment to help understand the test routine.
   +
   +    Note that this commit mostly moves the test from reftable/ to
   +    t/unit-tests/ and most of the refactoring is performed by the
   +    trailing commits.
   +
        Mentored-by: Patrick Steinhardt <[e-mail]>
        Mentored-by: Christian Couder <[e-mail]>
        Signed-off-by: Chandra Pratap <[e-mail]>

     ## Makefile ##
   -@@ Makefile: UNIT_TEST_PROGRAMS += t-mem-pool
   - UNIT_TEST_PROGRAMS += t-oidtree
   +@@ Makefile: UNIT_TEST_PROGRAMS += t-oidtree
     UNIT_TEST_PROGRAMS += t-prio-queue
     UNIT_TEST_PROGRAMS += t-reftable-basics
   + UNIT_TEST_PROGRAMS += t-reftable-record
    +UNIT_TEST_PROGRAMS += t-reftable-tree
     UNIT_TEST_PROGRAMS += t-strbuf
     UNIT_TEST_PROGRAMS += t-strcmp-offset
     UNIT_TEST_PROGRAMS += t-strvec
   -@@ Makefile: REFTABLE_TEST_OBJS += reftable/record_test.o
   +@@ Makefile: REFTABLE_TEST_OBJS += reftable/pq_test.o
     REFTABLE_TEST_OBJS += reftable/readwrite_test.o
     REFTABLE_TEST_OBJS += reftable/stack_test.o
     REFTABLE_TEST_OBJS += reftable/test_framework.o
    @@ reftable/tree_test.c (deleted)

     ## t/helper/test-reftable.c ##
    @@ t/helper/test-reftable.c: int cmd__reftable(int argc, const char **argv)
   + {
    	/* test from simple to complex. */
   - 	record_test_main(argc, argv);
     	block_test_main(argc, argv);
    -	tree_test_main(argc, argv);
    	pq_test_main(argc, argv);
    @@ t/unit-tests/t-reftable-tree.c (new)
     +	size_t i = 1;
     +	struct curry c = { 0 };
     +
    ++	/* pseudo-randomly insert the pointers for elements between
    ++	 * values[1] and values[10] (included) in the tree.
    ++	 */
     +	do {
    -+		nodes[i] = tree_search(values + i, &root, &t_compare, 1);
    ++		nodes[i] = tree_search(&values[i], &root, &t_compare, 1);
     +		i = (i * 7) % 11;
     +	} while (i != 1);
     +
     +	for (i = 1; i < ARRAY_SIZE(nodes); i++) {
    -+		check_pointer_eq(values + i, nodes[i]->key);
    -+		check_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));
    ++		check_pointer_eq(&values[i], nodes[i]->key);
    ++		check_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));
     +	}
     +
     +	infix_walk(root, check_increasing, &c);
 3:  c733776054 ! 47:  59d5c17d5e t-reftable-tree: split test_tree() into two sub-test functions
    @@ Commit message
         'int insert' which when set, inserts the key if it is not found
         in the tree. Otherwise, the function returns NULL for such cases.

    +    While at it, use 'func' to pass function pointers and not '&func'.
    +
         Mentored-by: Patrick Steinhardt <[e-mail]>
         Mentored-by: Christian Couder <[e-mail]>
         Signed-off-by: Chandra Pratap <[e-mail]>
    @@ t/unit-tests/t-reftable-tree.c: static void check_increasing(void *arg, void *ke
      	size_t i = 1;
     -	struct curry c = { 0 };

    - 	do {
    - 		nodes[i] = tree_search(values + i, &root, &t_compare, 1);
    + 	/* pseudo-randomly insert the pointers for elements between
    + 	 * values[1] and values[10] (included) in the tree.
     @@ t/unit-tests/t-reftable-tree.c: static void t_tree(void)
    - 		check_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));
    + 		check_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));
      	}

     -	infix_walk(root, check_increasing, &c);
    @@ t/unit-tests/t-reftable-tree.c: static void t_tree(void)
     +	size_t i = 1;
     +
     +	do {
    -+		tree_search(values + i, &root, &t_compare, 1);
    ++		tree_search(&values[i], &root, t_compare, 1);
     +		i = (i * 7) % 11;
     +	} while (i != 1);
     +
 4:  f1a9325bb3 <  -:  ---------- t-reftable-tree: add test for non-existent key
 -:  ---------- > 48:  c1ce79916b t-reftable-tree: add test for non-existent key
 5:  c6b7a3d646 ! 49:  d1a5ced526 t-reftable-tree: improve the test for infix_walk()
    @@ t/unit-tests/t-reftable-tree.c: static void t_infix_walk(void)
     +	size_t count = 0;

      	do {
    - 		tree_search(values + i, &root, &t_compare, 1);
    + 		tree_search(&values[i], &root, t_compare, 1);
      		i = (i * 7) % 11;
     +		count++;
      	} while (i != 1);
    @@ t/unit-tests/t-reftable-tree.c: static void t_infix_walk(void)
     -	infix_walk(root, &check_increasing, &c);
     +	infix_walk(root, &store, &c);
     +	for (i = 1; i < ARRAY_SIZE(values); i++)
    -+		check_pointer_eq(values + i, out[i - 1]);
    ++		check_pointer_eq(&values[i], out[i - 1]);
     +	check(!out[i - 1]);
     +	check_int(c.len, ==, count);
      	tree_free(root);

Comments

Chandra Pratap Aug. 1, 2024, 11:21 a.m. UTC | #1

A reminder for reviews/acks.

Patrick Steinhardt Aug. 1, 2024, 11:45 a.m. UTC | #2

On Thu, Aug 01, 2024 at 04:51:44PM +0530, Chandra Pratap wrote:
> A reminder for reviews/acks.

There was one style nit from Junio already that you should probably
address. Other than that I didn't really have anything else to add to
this series.

Patrick