mbox series

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

Message ID 20240716075641.4264-1-chandrapratap3519@gmail.com (mailing list archive)
Headers show
Series t: port reftable/tree_test.c to the unit testing framework | expand

Message

Chandra Pratap July 16, 2024, 7:48 a.m. UTC
In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
testing framework written entirely in C was introduced to the Git project
aimed at simplifying testing and reducing test run times.
Currently, tests for the reftable refs-backend are performed by a custom
testing framework defined by reftable/test_framework.{c, h}. 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 <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>

---
Changes in v4:
- Rename the tests to be in-line with unit-tests' standards

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 | 80 +++++++++++++++++++++++++++++++++++++
6 files changed, 86 insertions(+), 73 deletions(-)

Range-diff against v3:
1:  ffabd3e411 = 1:  2be2a35b7f reftable: remove unnecessary curly braces in reftable/tree.c
2:  17937233fb < -:  ---------- t: move reftable/tree_test.c to the unit testing framework
-:  ---------- > 2:  de49698ea7 t: move reftable/tree_test.c to the unit testing framework
3:  c3992091db ! 3:  c733776054 t-reftable-tree: split test_tree() into two sub-test functions
    @@ t/unit-tests/t-reftable-tree.c: static void check_increasing(void *arg, void *ke
      	c->last = key;
      }
      
    --static void test_tree(void)
    -+static void test_tree_search(void)
    +-static void t_tree(void)
    ++static void t_tree_search(void)
      {
      	struct tree_node *root = NULL;
      	void *values[11] = { 0 };
    @@ t/unit-tests/t-reftable-tree.c: static void check_increasing(void *arg, void *ke
     -	struct curry c = { 0 };
      
      	do {
    - 		nodes[i] = tree_search(values + i, &root, &test_compare, 1);
    -@@ t/unit-tests/t-reftable-tree.c: static void test_tree(void)
    - 		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
    + 		nodes[i] = tree_search(values + i, &root, &t_compare, 1);
    +@@ 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));
      	}
      
     -	infix_walk(root, check_increasing, &c);
     +	tree_free(root);
     +}
     +
    -+static void test_infix_walk(void)
    ++static void t_infix_walk(void)
     +{
     +	struct tree_node *root = NULL;
     +	void *values[11] = { 0 };
    @@ t/unit-tests/t-reftable-tree.c: static void test_tree(void)
     +	size_t i = 1;
     +
     +	do {
    -+		tree_search(values + i, &root, &test_compare, 1);
    ++		tree_search(values + i, &root, &t_compare, 1);
     +		i = (i * 7) % 11;
     +	} while (i != 1);
     +
    @@ t/unit-tests/t-reftable-tree.c: static void test_tree(void)
      
      int cmd_main(int argc, const char *argv[])
      {
    --	TEST(test_tree(), "tree_search and infix_walk work");
    -+	TEST(test_tree_search(), "tree_search works");
    -+	TEST(test_infix_walk(), "infix_walk works");
    +-	TEST(t_tree(), "tree_search and infix_walk work");
    ++	TEST(t_tree_search(), "tree_search works");
    ++	TEST(t_infix_walk(), "infix_walk works");
      
      	return test_done();
      }
4:  99a0ed484e ! 4:  f1a9325bb3 t-reftable-tree: add test for non-existent key
    @@ Commit message
         Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
     
      ## t/unit-tests/t-reftable-tree.c ##
    -@@ t/unit-tests/t-reftable-tree.c: static void test_tree_search(void)
    - 		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
    +@@ t/unit-tests/t-reftable-tree.c: static void t_tree_search(void)
    + 		check_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));
      	}
      
     +	check(!tree_search(values, &root, &test_compare, 0));
5:  4fce9a8bd8 ! 5:  c6b7a3d646 t-reftable-tree: improve the test for infix_walk()
    @@ Commit message
         Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
     
      ## t/unit-tests/t-reftable-tree.c ##
    -@@ t/unit-tests/t-reftable-tree.c: static int test_compare(const void *a, const void *b)
    +@@ t/unit-tests/t-reftable-tree.c: static int t_compare(const void *a, const void *b)
      }
      
      struct curry {
    @@ t/unit-tests/t-reftable-tree.c: static int test_compare(const void *a, const voi
      {
      	struct curry *c = arg;
     -	if (c->last)
    --		check_int(test_compare(c->last, key), <, 0);
    +-		check_int(t_compare(c->last, key), <, 0);
     -	c->last = key;
     +	c->arr[c->len++] = key;
      }
      
    - static void test_tree_search(void)
    -@@ t/unit-tests/t-reftable-tree.c: static void test_infix_walk(void)
    + static void t_tree_search(void)
    +@@ t/unit-tests/t-reftable-tree.c: static void t_infix_walk(void)
      {
      	struct tree_node *root = NULL;
      	void *values[11] = { 0 };
    @@ t/unit-tests/t-reftable-tree.c: static void test_infix_walk(void)
     +	size_t count = 0;
      
      	do {
    - 		tree_search(values + i, &root, &test_compare, 1);
    + 		tree_search(values + i, &root, &t_compare, 1);
      		i = (i * 7) % 11;
     +		count++;
      	} while (i != 1);

Comments

Junio C Hamano July 16, 2024, 7:52 p.m. UTC | #1
Chandra Pratap <chandrapratap3519@gmail.com> writes:

> In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit

Consistently refer to an existing commit with --format=reference.

	$ git show -s --format=reference 8bf6fbd
	8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)

> testing framework written entirely in C was introduced to the Git project
> aimed at simplifying testing and reducing test run times.

I doubt that the reason why "unit-tests" written entirely in C was
introduced is because we wanted to simplify testing and to reduce
test run times to begin with.  The traditional tests to observe the
effect visible to end-users by actually running commands that would
be run by end-users and unit tests serve two separate purposes.  The
latter does not "simplify", or "reduce time"---you cannot write a
test "entirely in C" to make sure, say, that "git push --force"
allows a non-fast-forward update to happen using unit-test
framework.  The unit-tests cannot replace end-to-end tests.  They
are complementary.

The statement may need to be rethought.

Or just stop at saying something like

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

which already covers the next paragraph while at it.

> Currently, tests for the reftable refs-backend are performed by a custom
> testing framework defined by reftable/test_framework.{c, h}. 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 <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
> ---
> Changes in v4:
> - Rename the tests to be in-line with unit-tests' standards
>
> CI/PR: https://github.com/gitgitgadget/git/pull/1740

By the way, did you rebase the patches?  On which commit is this
iteration based?  Judging from the second patch, it seems to assume
that Makefile does not yet have t-reftable-record in it, and it
applies cleanly to 'master' before 9118e46e (Merge branch
'cp/unit-test-reftable-record', 2024-07-15).  Newer 'master' has
textual conflicts (nothing that I cannot resolve, but it shows that
apparently that anything newer than 9118e46e are not commits that
you developed this series on).

Has this series even been compile-tested?  I do not think that even
the unit test added by this series has been run (or compiled).

$ make -j32 t/unit-tests/t-reftable-tree.o
GIT_VERSION = 2.46.0.rc0.27.g0c2075a7c5
    * new build flags
    CC t/unit-tests/t-reftable-tree.o
In file included from t/unit-tests/t-reftable-tree.c:9:
t/unit-tests/t-reftable-tree.c: In function ‘t_tree_search’:
t/unit-tests/t-reftable-tree.c:45:44: error: ‘test_compare’ undeclared (first use in this function); did you mean ‘t_compare’?
   45 |         check(!tree_search(values, &root, &test_compare, 0));
      |                                            ^~~~~~~~~~~~
t/unit-tests/test-lib.h:75:45: note: in definition of macro ‘check’
   75 |         check_bool_loc(TEST_LOCATION(), #x, x)
      |                                             ^
t/unit-tests/t-reftable-tree.c:45:44: note: each undeclared identifier is reported only once for each function it appears in
   45 |         check(!tree_search(values, &root, &test_compare, 0));
      |                                            ^~~~~~~~~~~~
t/unit-tests/test-lib.h:75:45: note: in definition of macro ‘check’
   75 |         check_bool_loc(TEST_LOCATION(), #x, x)
      |                                             ^
make: *** [Makefile:2754: t/unit-tests/t-reftable-tree.o] Error 1





Let's concentrate on quality, not quantity; too many topics with the
same prefix cp/unit-test* seem to be left unreviewed on the list.

In the meantime, I'll queue the following fix-up on top.  In this
codebase, it is preferred to write a pointer to a function whose
name is "func" as just "func", not "&func".


 t/unit-tests/t-reftable-tree.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index b3d4008e5c..107f1f69bf 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -33,16 +33,16 @@ static void t_tree_search(void)
 	size_t i = 1;
 
 	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(nodes[i], tree_search(values + i, &root, t_compare, 0));
 	}
 
-	check(!tree_search(values, &root, &test_compare, 0));
+	check(!tree_search(values, &root, t_compare, 0));
 	tree_free(root);
 }
 
@@ -58,7 +58,7 @@ 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);