threads / patch / 61614

patch, 4 partst: port reftable/tree_test.c to the unit testing framework

Subject: [GSoC][PATCH 0/4] t: port reftable/tree_test.c to the unit testing framework

## tl;dr

66 messages between Jun 10, 2024 and Aug 6, 2024. Diffs are folded; open one to read it.

replies: 65people: 5as markdown or json

Chandra Pratap· Jun 10, 2024, 13:01 UTC · lore

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>

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

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

Makefile | 2 +- reftable/tree.c | 15 +++-------- reftable/tree_test.c | 60 ----------------------------------- t/helper/test-reftable.c | 1 - t/unit-tests/t-reftable-tree.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 79 insertions(+), 72 deletions(-)

Chandra Pratap· Jun 10, 2024, 13:01 UTC · re: Chandra Pratap · lore

[PATCH 1/4] reftable: remove unnecessary curly braces in reftable/tree.c

According to Documentation/CodingGuidelines, single-line control-flow statements must omit curly braces (except for some special cases). Make reftable/tree.c adhere to this guideline.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 reftable/tree.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
Show changes to reftable/tree.c +5 −10
diff --git a/reftable/tree.c b/reftable/tree.c
index 528f33ae38..5ffb2e0d69 100644
--- a/reftable/tree.c
+++ b/reftable/tree.c
@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,
 void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),
 		void *arg)
 {
-	if (t->left) {
+	if (t->left)
 		infix_walk(t->left, action, arg);
-	}
 	action(arg, t->key);
-	if (t->right) {
+	if (t->right)
 		infix_walk(t->right, action, arg);
-	}
 }
 
 void tree_free(struct tree_node *t)
 {
-	if (!t) {
+	if (!t)
 		return;
-	}
-	if (t->left) {
+	if (t->left)
 		tree_free(t->left);
-	}
-	if (t->right) {
+	if (t->right)
 		tree_free(t->right);
-	}
 	reftable_free(t);
 }
-- 
2.45.2.404.g9eaef5822c
Chandra Pratap· Jun 10, 2024, 13:01 UTC · re: Chandra Pratap · lore

[PATCH 2/4] t: move reftable/tree_test.c to the unit testing framework

reftable/tree_test.c exercises the functions defined in reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit testing framework. Migration involves refactoring the tests to use the unit testing framework instead of reftable's test framework.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                                      |  2 +-
 t/helper/test-reftable.c                      |  1 -
 .../unit-tests/t-reftable-tree.c              | 32 ++++++++-----------
 3 files changed, 15 insertions(+), 20 deletions(-)
 rename reftable/tree_test.c => t/unit-tests/t-reftable-tree.c (59%)
Show changes to 4 files +15 −20

Makefile, t/helper/test-reftable.c, reftable/tree_test.c, t/unit-tests/t-reftable-tree.c

diff --git a/Makefile b/Makefile
index 2f5f16847a..d736b2f8bd 100644
--- a/Makefile
+++ b/Makefile
@@ -1336,6 +1336,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
 UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-prio-queue
+UNIT_TEST_PROGRAMS += t-reftable-tree
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
 UNIT_TEST_PROGRAMS += t-strvec
@@ -2681,7 +2682,6 @@ REFTABLE_TEST_OBJS += reftable/record_test.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
 REFTABLE_TEST_OBJS += reftable/stack_test.o
 REFTABLE_TEST_OBJS += reftable/test_framework.o
-REFTABLE_TEST_OBJS += reftable/tree_test.o
 
 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index bae731669c..9475db2f76 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -8,7 +8,6 @@ int cmd__reftable(int argc, const char **argv)
 	basics_test_main(argc, argv);
 	record_test_main(argc, argv);
 	block_test_main(argc, argv);
-	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
 	merged_test_main(argc, argv);
diff --git a/reftable/tree_test.c b/t/unit-tests/t-reftable-tree.c
similarity index 59%
rename from reftable/tree_test.c
rename to t/unit-tests/t-reftable-tree.c
index 6961a657ad..208e7b7874 100644
--- a/reftable/tree_test.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -6,11 +6,8 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "system.h"
-#include "tree.h"
-
-#include "test_framework.h"
-#include "reftable-tests.h"
+#include "test-lib.h"
+#include "reftable/tree.h"
 
 static int test_compare(const void *a, const void *b)
 {
@@ -24,37 +21,36 @@ struct curry {
 static void check_increasing(void *arg, void *key)
 {
 	struct curry *c = arg;
-	if (c->last) {
-		EXPECT(test_compare(c->last, key) < 0);
-	}
+	if (c->last)
+		check_int(test_compare(c->last, key), <, 0);
 	c->last = key;
 }
 
 static void test_tree(void)
 {
 	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct tree_node *nodes[11] = { 0 };
+	size_t i = 1;
+	struct curry c = { 0 };
 
-	void *values[11] = { NULL };
-	struct tree_node *nodes[11] = { NULL };
-	int i = 1;
-	struct curry c = { NULL };
 	do {
 		nodes[i] = tree_search(values + i, &root, &test_compare, 1);
 		i = (i * 7) % 11;
 	} while (i != 1);
 
 	for (i = 1; i < ARRAY_SIZE(nodes); i++) {
-		EXPECT(values + i == nodes[i]->key);
-		EXPECT(nodes[i] ==
-		       tree_search(values + i, &root, &test_compare, 0));
+		check_pointer_eq(values + i, nodes[i]->key);
+		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
 	}
 
 	infix_walk(root, check_increasing, &c);
 	tree_free(root);
 }
 
-int tree_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_tree);
-	return 0;
+	TEST(test_tree(), "tree_search and infix_walk work");
+
+	return test_done();
 }
-- 
2.45.2.404.g9eaef5822c
Chandra Pratap· Jun 10, 2024, 13:01 UTC · re: Chandra Pratap · lore

[PATCH 3/4] t-reftable-tree: split test_tree() into two sub-test functions

In the current testing setup, tests for both tree_search() and infix_walk() defined by reftable/tree.{c, h} are performed by a single test function, test_tree(). Split tree_test() into test_tree_search() and test_infix_walk() responsible for independently testing tree_search() and infix_walk() respectively. This improves the overall readability of the test file as well as simplifies debugging.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +19 −3
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 208e7b7874..78d5caafbe 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -26,7 +26,7 @@ static void check_increasing(void *arg, void *key)
 	c->last = key;
 }
 
-static void test_tree(void)
+static void test_tree_search(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
@@ -44,13 +44,29 @@ static void test_tree(void)
 		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
 	}
 
-	infix_walk(root, check_increasing, &c);
+	tree_free(root);
+}
+
+static void test_infix_walk(void)
+{
+	struct tree_node *root = NULL;
+	void *values[13] = { 0 };
+	struct curry c = { 0 };
+	size_t i = 1;
+
+	do {
+		tree_search(values + i, &root, &test_compare, 1);
+		i = (i * 5) % 13;
+	} while (i != 1);
+
+	infix_walk(root, &check_increasing, &c);
 	tree_free(root);
 }
 
 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");
 
 	return test_done();
 }
-- 
2.45.2.404.g9eaef5822c
Patrick Steinhardt· Jun 10, 2024, 13:49 UTC · re: Chandra Pratap · lore

Re: [PATCH 3/4] t-reftable-tree: split test_tree() into two sub-test functions

On Mon, Jun 10, 2024 at 06:31:30PM +0530, Chandra Pratap wrote:
Show 12 quoted lines
> @@ -44,13 +44,29 @@ static void test_tree(void)
>  		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
>  	}
>  
> -	infix_walk(root, check_increasing, &c);
> +	tree_free(root);
> +}
> +
> +static void test_infix_walk(void)
> +{
> +	struct tree_node *root = NULL;
> +	void *values[13] = { 0 };

Is there a reason why we have 13 values here while we had 11 values in the test this was split out from?

Show 7 quoted lines
> +	struct curry c = { 0 };
> +	size_t i = 1;
> +
> +	do {
> +		tree_search(values + i, &root, &test_compare, 1);
> +		i = (i * 5) % 13;
> +	} while (i != 1);

It's completely non-obvious that `tree_search()` ends up _inserting_ nodes into the tree when the entry we're searching for wasn't found (and if the last parameter is `1`. I feel like this interface could really use a complete makeover and split up its concerns. In any case, that does not need to happen as part of this patch seriesr

What I think would help though is if the commit message itself mentioned this unorthodox way of inserting values into the tree.

> +	infix_walk(root, &check_increasing, &c);

Not a fault of this commit, but this test certainly isn't great. It would succeed even if `infix_walk()` didn't do anything as we do not verify at all whether all nodes have been traversed (and traversed once, exactly).

Patrick
Chandra Pratap· Jun 11, 2024, 06:44 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 3/4] t-reftable-tree: split test_tree() into two sub-test functions

On Mon, 10 Jun 2024 at 19:19, Patrick Steinhardt <ps@pks.im> wrote:
Show 17 quoted lines
>
> On Mon, Jun 10, 2024 at 06:31:30PM +0530, Chandra Pratap wrote:
> > @@ -44,13 +44,29 @@ static void test_tree(void)
> >               check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
> >       }
> >
> > -     infix_walk(root, check_increasing, &c);
> > +     tree_free(root);
> > +}
> > +
> > +static void test_infix_walk(void)
> > +{
> > +     struct tree_node *root = NULL;
> > +     void *values[13] = { 0 };
>
> Is there a reason why we have 13 values here while we had 11 values in
> the test this was split out from?

I did that to introduce some variety between the test cases, but now that you mention it, this change doesn't go well with the objective of this patch.

Show 13 quoted lines
> > +     struct curry c = { 0 };
> > +     size_t i = 1;
> > +
> > +     do {
> > +             tree_search(values + i, &root, &test_compare, 1);
> > +             i = (i * 5) % 13;
> > +     } while (i != 1);
>
> It's completely non-obvious that `tree_search()` ends up _inserting_
> nodes into the tree when the entry we're searching for wasn't found (and
> if the last parameter is `1`. I feel like this interface could really
> use a complete makeover and split up its concerns. In any case, that
> does not need to happen as part of this patch seriesr

I don't really mind it because all tree operations are only used in reftable/writer.c and only in a couple of places, so it would make sense for the original authors to focus their efforts on other parts of the codebase. Still, I do agree that the readability takes a hit 'cause of that.

> What I think would help though is if the commit message itself mentioned
> this unorthodox way of inserting values into the tree.
Sure thing.
Show 6 quoted lines
> > +     infix_walk(root, &check_increasing, &c);
>
> Not a fault of this commit, but this test certainly isn't great. It
> would succeed even if `infix_walk()` didn't do anything as we do not
> verify at all whether all nodes have been traversed (and traversed once,
> exactly).

I'll try to modify the test to check for these properties of an infix walk as well.

Chandra Pratap· Jun 10, 2024, 13:01 UTC · re: Chandra Pratap · lore

[PATCH 4/4] t-reftable-tree: add test for non-existent key

In the current testing setup for tree_search(), the case for non-existent key is not exercised. Improve this by adding a test-case for the same.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/unit-tests/t-reftable-tree.c +1 −0
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 78d5caafbe..b2fca0cb5e 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -44,6 +44,7 @@ static void test_tree_search(void)
 		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
 	}
 
+	check(!tree_search(values, &root, &test_compare, 0));
 	tree_free(root);
 }
 
-- 
2.45.2.404.g9eaef5822c
Chandra Pratap· Jun 12, 2024, 05:38 UTC · re: Chandra Pratap · lore

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

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 v2:
- Add more context in the commit message of the third patch
- Add an improvement patch for test_infix_walk()
- Small refactor changes
CI/PR: https://github.com/gitgitgadget/git/pull/1740

Chandra Pratap(5): reftable: remove unnecessary curly braces in t: move reftable/tree_test.c to the unit testing 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/tree.c | 15 +++-------- reftable/tree_test.c | 60 ------------------------------- t/helper/test-reftable.c | 1 - t/unit-tests/t-reftable-tree.c | 76 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 82 insertions(+), 72 deletions(-)

Range-diff against v1:
1:  161d8892d6 ! 1:  542e497334 t-reftable-tree: split test_tree() into two sub-test functions
    @@ Commit message
         This improves the overall readability of the test file as well as
         simplifies debugging.
     
    +    Note that the last parameter in the tree_search() functiom is
    +    'int insert' which when set, inserts the key if it is not found
    +    in the tree. Otherwise, the function returns NULL for such cases.
    +
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Mentored-by: Christian Couder <chriscool@tuxfamily.org>
         Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
    @@ t/unit-tests/t-reftable-tree.c: static void check_increasing(void *arg, void *ke
      {
      	struct tree_node *root = NULL;
      	void *values[11] = { 0 };
    + 	struct tree_node *nodes[11] = { 0 };
    + 	size_t i = 1;
    +-	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));
      	}
    @@ t/unit-tests/t-reftable-tree.c: static void test_tree(void)
     +static void test_infix_walk(void)
     +{
     +	struct tree_node *root = NULL;
    -+	void *values[13] = { 0 };
    ++	void *values[11] = { 0 };
     +	struct curry c = { 0 };
     +	size_t i = 1;
     +
     +	do {
     +		tree_search(values + i, &root, &test_compare, 1);
    -+		i = (i * 5) % 13;
    ++		i = (i * 7) % 11;
     +	} while (i != 1);
     +
     +	infix_walk(root, &check_increasing, &c);
2:  d649c4a193 = 2:  c976a37cbc t-reftable-tree: add test for non-existent key
-:  ---------- > 3:  3010c8f01a t-reftable-tree: improve the test for infix_walk()
Chandra Pratap· Jun 12, 2024, 05:38 UTC · re: Chandra Pratap · lore

[PATCH v2 1/5] reftable: remove unnecessary curly braces in reftable/tree.c

According to Documentation/CodingGuidelines, single-line control-flow statements must omit curly braces (except for some special cases). Make reftable/tree.c adhere to this guideline.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 reftable/tree.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
Show changes to reftable/tree.c +5 −10
diff --git a/reftable/tree.c b/reftable/tree.c
index 528f33ae38..5ffb2e0d69 100644
--- a/reftable/tree.c
+++ b/reftable/tree.c
@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,
 void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),
 		void *arg)
 {
-	if (t->left) {
+	if (t->left)
 		infix_walk(t->left, action, arg);
-	}
 	action(arg, t->key);
-	if (t->right) {
+	if (t->right)
 		infix_walk(t->right, action, arg);
-	}
 }
 
 void tree_free(struct tree_node *t)
 {
-	if (!t) {
+	if (!t)
 		return;
-	}
-	if (t->left) {
+	if (t->left)
 		tree_free(t->left);
-	}
-	if (t->right) {
+	if (t->right)
 		tree_free(t->right);
-	}
 	reftable_free(t);
 }
-- 
2.45.2.404.g9eaef5822c
Chandra Pratap· Jun 12, 2024, 05:38 UTC · re: Chandra Pratap · lore

[PATCH v2 2/5] t: move reftable/tree_test.c to the unit testing framework

reftable/tree_test.c exercises the functions defined in reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit testing framework. Migration involves refactoring the tests to use the unit testing framework instead of reftable's test framework.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                                      |  2 +-
 t/helper/test-reftable.c                      |  1 -
 .../unit-tests/t-reftable-tree.c              | 32 ++++++++-----------
 3 files changed, 15 insertions(+), 20 deletions(-)
 rename reftable/tree_test.c => t/unit-tests/t-reftable-tree.c (59%)
Show changes to 4 files +15 −20

Makefile, t/helper/test-reftable.c, reftable/tree_test.c, t/unit-tests/t-reftable-tree.c

diff --git a/Makefile b/Makefile
index 2f5f16847a..d736b2f8bd 100644
--- a/Makefile
+++ b/Makefile
@@ -1336,6 +1336,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
 UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-prio-queue
+UNIT_TEST_PROGRAMS += t-reftable-tree
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
 UNIT_TEST_PROGRAMS += t-strvec
@@ -2681,7 +2682,6 @@ REFTABLE_TEST_OBJS += reftable/record_test.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
 REFTABLE_TEST_OBJS += reftable/stack_test.o
 REFTABLE_TEST_OBJS += reftable/test_framework.o
-REFTABLE_TEST_OBJS += reftable/tree_test.o
 
 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index bae731669c..9475db2f76 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -8,7 +8,6 @@ int cmd__reftable(int argc, const char **argv)
 	basics_test_main(argc, argv);
 	record_test_main(argc, argv);
 	block_test_main(argc, argv);
-	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
 	merged_test_main(argc, argv);
diff --git a/reftable/tree_test.c b/t/unit-tests/t-reftable-tree.c
similarity index 59%
rename from reftable/tree_test.c
rename to t/unit-tests/t-reftable-tree.c
index 6961a657ad..208e7b7874 100644
--- a/reftable/tree_test.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -6,11 +6,8 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "system.h"
-#include "tree.h"
-
-#include "test_framework.h"
-#include "reftable-tests.h"
+#include "test-lib.h"
+#include "reftable/tree.h"
 
 static int test_compare(const void *a, const void *b)
 {
@@ -24,37 +21,36 @@ struct curry {
 static void check_increasing(void *arg, void *key)
 {
 	struct curry *c = arg;
-	if (c->last) {
-		EXPECT(test_compare(c->last, key) < 0);
-	}
+	if (c->last)
+		check_int(test_compare(c->last, key), <, 0);
 	c->last = key;
 }
 
 static void test_tree(void)
 {
 	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct tree_node *nodes[11] = { 0 };
+	size_t i = 1;
+	struct curry c = { 0 };
 
-	void *values[11] = { NULL };
-	struct tree_node *nodes[11] = { NULL };
-	int i = 1;
-	struct curry c = { NULL };
 	do {
 		nodes[i] = tree_search(values + i, &root, &test_compare, 1);
 		i = (i * 7) % 11;
 	} while (i != 1);
 
 	for (i = 1; i < ARRAY_SIZE(nodes); i++) {
-		EXPECT(values + i == nodes[i]->key);
-		EXPECT(nodes[i] ==
-		       tree_search(values + i, &root, &test_compare, 0));
+		check_pointer_eq(values + i, nodes[i]->key);
+		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
 	}
 
 	infix_walk(root, check_increasing, &c);
 	tree_free(root);
 }
 
-int tree_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_tree);
-	return 0;
+	TEST(test_tree(), "tree_search and infix_walk work");
+
+	return test_done();
 }
-- 
2.45.2.404.g9eaef5822c
Chandra Pratap· Jun 12, 2024, 05:38 UTC · re: Chandra Pratap · lore

[PATCH v2 3/5] t-reftable-tree: split test_tree() into two sub-test functions

In the current testing setup, tests for both tree_search() and infix_walk() defined by reftable/tree.{c, h} are performed by a single test function, test_tree(). Split tree_test() into test_tree_search() and test_infix_walk() responsible for independently testing tree_search() and infix_walk() respectively. This improves the overall readability of the test file as well as simplifies debugging.

Note that the last parameter in the tree_search() functiom is 'int insert' which when set, inserts the key if it is not found in the tree. Otherwise, the function returns NULL for such cases.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +19 −4
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 208e7b7874..cb721b377a 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -26,13 +26,12 @@ static void check_increasing(void *arg, void *key)
 	c->last = key;
 }
 
-static void test_tree(void)
+static void test_tree_search(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
 	struct tree_node *nodes[11] = { 0 };
 	size_t i = 1;
-	struct curry c = { 0 };
 
 	do {
 		nodes[i] = tree_search(values + i, &root, &test_compare, 1);
@@ -44,13 +43,29 @@ static void test_tree(void)
 		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
 	}
 
-	infix_walk(root, check_increasing, &c);
+	tree_free(root);
+}
+
+static void test_infix_walk(void)
+{
+	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct curry c = { 0 };
+	size_t i = 1;
+
+	do {
+		tree_search(values + i, &root, &test_compare, 1);
+		i = (i * 7) % 11;
+	} while (i != 1);
+
+	infix_walk(root, &check_increasing, &c);
 	tree_free(root);
 }
 
 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");
 
 	return test_done();
 }
-- 
2.45.2.404.g9eaef5822c
Chandra Pratap· Jun 12, 2024, 05:38 UTC · re: Chandra Pratap · lore

[PATCH v2 4/5] t-reftable-tree: add test for non-existent key

In the current testing setup for tree_search(), the case for non-existent key is not exercised. Improve this by adding a test-case for the same.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/unit-tests/t-reftable-tree.c +1 −0
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index cb721b377a..f1adab4458 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -43,6 +43,7 @@ static void test_tree_search(void)
 		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
 	}
 
+	check(!tree_search(values, &root, &test_compare, 0));
 	tree_free(root);
 }
 
-- 
2.45.2.404.g9eaef5822c
Chandra Pratap· Jun 12, 2024, 05:38 UTC · re: Chandra Pratap · lore

[PATCH v2 5/5] t-reftable-tree: improve the test for infix_walk()

In the current testing setup for infix_walk(), the following
properties of an infix traversal of a tree remain untested:
- every node of the tree must be visited
- every node must be visited exactly only
and only the property 'traversal in increasing order' is tested.
Modify test_infix_walk() to check for all the properties above.

This can be achieved by storing the nodes' keys linearly, in a nullified buffer, as we visit them and then checking the input keys against this buffer in increasing order. By checking that the element just after the last input key is 'NULL' in the output buffer, we ensure that every node is traversed exactly once.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +10 −6
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index f1adab4458..917a64a7d1 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -15,15 +15,14 @@ static int test_compare(const void *a, const void *b)
 }
 
 struct curry {
-	void *last;
+	void **arr;
+	size_t i;
 };
 
-static void check_increasing(void *arg, void *key)
+static void store(void *arg, void *key)
 {
 	struct curry *c = arg;
-	if (c->last)
-		check_int(test_compare(c->last, key), <, 0);
-	c->last = key;
+	c->arr[c->i++] = key;
 }
 
 static void test_tree_search(void)
@@ -51,6 +50,7 @@ static void test_infix_walk(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
+	void *out[20] = { 0 };
 	struct curry c = { 0 };
 	size_t i = 1;
 
@@ -59,7 +59,11 @@ static void test_infix_walk(void)
 		i = (i * 7) % 11;
 	} while (i != 1);
 
-	infix_walk(root, &check_increasing, &c);
+	c.arr = (void **) &out;
+	infix_walk(root, &store, &c);
+	for (i = 1; i < ARRAY_SIZE(values); i++)
+		check_pointer_eq(values + i, out[i - 1]);
+	check(!out[i]);
 	tree_free(root);
 }
 
-- 
2.45.2.404.g9eaef5822c
Patrick Steinhardt· Jun 12, 2024, 06:59 UTC · re: Chandra Pratap · lore

Re: [PATCH v2 5/5] t-reftable-tree: improve the test for infix_walk()

On Wed, Jun 12, 2024 at 11:08:14AM +0530, Chandra Pratap wrote:
> In the current testing setup for infix_walk(), the following
> properties of an infix traversal of a tree remain untested:
> - every node of the tree must be visited
> - every node must be visited exactly only
s/only/once
> and only the property 'traversal in increasing order' is tested.
Nit: this reads a bit awkward. How about "In fact, we only verify that
the traversal happens in increasing order."
Show 5 quoted lines
> @@ -51,6 +50,7 @@ static void test_infix_walk(void)
>  {
>  	struct tree_node *root = NULL;
>  	void *values[11] = { 0 };
> +	void *out[20] = { 0 };

From the test below it looks like we only expect 11 values to be added to `out`. Why does this array have length 20?

We could of course also use something like `ALLOC_GROW()` to grow the array dynamically. But that'd likely be overkill.

Show 9 quoted lines
>  	struct curry c = { 0 };
>  	size_t i = 1;
>  
> @@ -59,7 +59,11 @@ static void test_infix_walk(void)
>  		i = (i * 7) % 11;
>  	} while (i != 1);
>  
> -	infix_walk(root, &check_increasing, &c);
> +	c.arr = (void **) &out;
We can initialize this variable directly when declaring `c`:
    struct curry c = {
        .arr = &out;
    };

Also, is the cast necessary? This is the only site where we use `struct curry` if I'm not mistaken, so I'd expect that the type of `arr` should match our expectations.

> +	infix_walk(root, &store, &c);
> +	for (i = 1; i < ARRAY_SIZE(values); i++)
> +		check_pointer_eq(values + i, out[i - 1]);

Let's also verify that `c.len` matches the expected number of nodes visited.

Patrick
> +	check(!out[i]);
>  	tree_free(root);
>  }
Chandra Pratap· Jun 12, 2024, 09:05 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 5/5] t-reftable-tree: improve the test for infix_walk()

On Wed, 12 Jun 2024 at 12:29, Patrick Steinhardt <ps@pks.im> wrote:
Show 22 quoted lines
>
> On Wed, Jun 12, 2024 at 11:08:14AM +0530, Chandra Pratap wrote:
> > In the current testing setup for infix_walk(), the following
> > properties of an infix traversal of a tree remain untested:
> > - every node of the tree must be visited
> > - every node must be visited exactly only
>
> s/only/once
>
> > and only the property 'traversal in increasing order' is tested.
>
> Nit: this reads a bit awkward. How about "In fact, we only verify that
> the traversal happens in increasing order."
>
> > @@ -51,6 +50,7 @@ static void test_infix_walk(void)
> >  {
> >       struct tree_node *root = NULL;
> >       void *values[11] = { 0 };
> > +     void *out[20] = { 0 };
>
> From the test below it looks like we only expect 11 values to be added
> to `out`. Why does this array have length 20?
That's an error. I'll correct it in the next iteration.
Show 18 quoted lines
> We could of course also use something like `ALLOC_GROW()` to grow the
> array dynamically. But that'd likely be overkill.
>
> >       struct curry c = { 0 };
> >       size_t i = 1;
> >
> > @@ -59,7 +59,11 @@ static void test_infix_walk(void)
> >               i = (i * 7) % 11;
> >       } while (i != 1);
> >
> > -     infix_walk(root, &check_increasing, &c);
> > +     c.arr = (void **) &out;
>
> We can initialize this variable directly when declaring `c`:
>
>     struct curry c = {
>         .arr = &out;
>     };
Right, this seems more concise.
> Also, is the cast necessary? This is the only site where we use `struct
> curry` if I'm not mistaken, so I'd expect that the type of `arr` should
> match our expectations.

Trying to do this without a cast produces the following compilation error for me: initialization of ‘void **’ from incompatible pointer type ‘void * (*)[11]’

Show 6 quoted lines
> > +     infix_walk(root, &store, &c);
> > +     for (i = 1; i < ARRAY_SIZE(values); i++)
> > +             check_pointer_eq(values + i, out[i - 1]);
>
> Let's also verify that `c.len` matches the expected number of nodes
> visited.
Sure.
Chandra Pratap· Jun 12, 2024, 12:52 UTC · re: Chandra Pratap · lore

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

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 v3:
- Fix a typo in the commit message of the fifth patch
- Add a check for number of input & output elements in the fifth patch
- Small refactor changes
CI/PR: https://github.com/gitgitgadget/git/pull/1740

Chandra Pratap(5): reftable: remove unnecessary curly braces in t: move reftable/tree_test.c to the unit testing 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/tree.c | 15 +++-------- reftable/tree_test.c | 60 ------------------------------- t/helper/test-reftable.c | 1 - t/unit-tests/t-reftable-tree.c | 76 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 86 insertions(+), 72 deletions(-)

Range-diff against v2:
1:  3010c8f01a ! 1:  44800ad205 t-reftable-tree: improve the test for infix_walk()
    @@ Commit message
         In the current testing setup for infix_walk(), the following
         properties of an infix traversal of a tree remain untested:
         - every node of the tree must be visited
    -    - every node must be visited exactly only
    -    and only the property 'traversal in increasing order' is tested.
    +    - every node must be visited exactly once
    +    In fact, only the property 'traversal in increasing order' is tested.
         Modify test_infix_walk() to check for all the properties above.
         This can be achieved by storing the nodes' keys linearly, in a nullified
    @@ t/unit-tests/t-reftable-tree.c: static int test_compare(const void *a, const voi
      struct curry {
     -	void *last;
     +	void **arr;
    -+	size_t i;
    ++	size_t len;
      };
     -static void check_increasing(void *arg, void *key)
    @@ t/unit-tests/t-reftable-tree.c: static int test_compare(const void *a, const voi
     -	if (c->last)
     -		check_int(test_compare(c->last, key), <, 0);
     -	c->last = key;
    -+	c->arr[c->i++] = 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)
      {
      	struct tree_node *root = NULL;
      	void *values[11] = { 0 };
    -+	void *out[20] = { 0 };
    - 	struct curry c = { 0 };
    +-	struct curry c = { 0 };
    ++	void *out[11] = { 0 };
    ++	struct curry c = {
    ++		.arr = (void **) &out,
    ++	};
      	size_t i = 1;
    ++	size_t count = 0;
    -@@ t/unit-tests/t-reftable-tree.c: static void test_infix_walk(void)
    + 	do {
    + 		tree_search(values + i, &root, &test_compare, 1);
      		i = (i * 7) % 11;
    ++		count++;
      	} while (i != 1);
     -	infix_walk(root, &check_increasing, &c);
    -+	c.arr = (void **) &out;
     +	infix_walk(root, &store, &c);
     +	for (i = 1; i < ARRAY_SIZE(values); i++)
     +		check_pointer_eq(values + i, out[i - 1]);
    -+	check(!out[i]);
    ++	check(!out[i - 1]);
    ++	check_int(c.len, ==, count);
      	tree_free(root);
      }
Chandra Pratap· Jun 12, 2024, 12:52 UTC · re: Chandra Pratap · lore

[PATCH v3 1/5] reftable: remove unnecessary curly braces in reftable/tree.c

According to Documentation/CodingGuidelines, single-line control-flow statements must omit curly braces (except for some special cases). Make reftable/tree.c adhere to this guideline.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 reftable/tree.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
Show changes to reftable/tree.c +5 −10
diff --git a/reftable/tree.c b/reftable/tree.c
index 528f33ae38..5ffb2e0d69 100644
--- a/reftable/tree.c
+++ b/reftable/tree.c
@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,
 void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),
 		void *arg)
 {
-	if (t->left) {
+	if (t->left)
 		infix_walk(t->left, action, arg);
-	}
 	action(arg, t->key);
-	if (t->right) {
+	if (t->right)
 		infix_walk(t->right, action, arg);
-	}
 }
 
 void tree_free(struct tree_node *t)
 {
-	if (!t) {
+	if (!t)
 		return;
-	}
-	if (t->left) {
+	if (t->left)
 		tree_free(t->left);
-	}
-	if (t->right) {
+	if (t->right)
 		tree_free(t->right);
-	}
 	reftable_free(t);
 }
-- 
2.45.2.404.g9eaef5822c
Chandra Pratap· Jun 12, 2024, 12:53 UTC · re: Chandra Pratap · lore

[PATCH v3 2/5] t: move reftable/tree_test.c to the unit testing framework

reftable/tree_test.c exercises the functions defined in reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit testing framework. Migration involves refactoring the tests to use the unit testing framework instead of reftable's test framework.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                                      |  2 +-
 t/helper/test-reftable.c                      |  1 -
 .../unit-tests/t-reftable-tree.c              | 32 ++++++++-----------
 3 files changed, 15 insertions(+), 20 deletions(-)
 rename reftable/tree_test.c => t/unit-tests/t-reftable-tree.c (59%)
Show changes to 4 files +15 −20

Makefile, t/helper/test-reftable.c, reftable/tree_test.c, t/unit-tests/t-reftable-tree.c

diff --git a/Makefile b/Makefile
index 2f5f16847a..d736b2f8bd 100644
--- a/Makefile
+++ b/Makefile
@@ -1336,6 +1336,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
 UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-prio-queue
+UNIT_TEST_PROGRAMS += t-reftable-tree
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
 UNIT_TEST_PROGRAMS += t-strvec
@@ -2681,7 +2682,6 @@ REFTABLE_TEST_OBJS += reftable/record_test.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
 REFTABLE_TEST_OBJS += reftable/stack_test.o
 REFTABLE_TEST_OBJS += reftable/test_framework.o
-REFTABLE_TEST_OBJS += reftable/tree_test.o
 
 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index bae731669c..9475db2f76 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -8,7 +8,6 @@ int cmd__reftable(int argc, const char **argv)
 	basics_test_main(argc, argv);
 	record_test_main(argc, argv);
 	block_test_main(argc, argv);
-	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
 	merged_test_main(argc, argv);
diff --git a/reftable/tree_test.c b/t/unit-tests/t-reftable-tree.c
similarity index 59%
rename from reftable/tree_test.c
rename to t/unit-tests/t-reftable-tree.c
index 6961a657ad..208e7b7874 100644
--- a/reftable/tree_test.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -6,11 +6,8 @@ license that can be found in the LICENSE file or at
 https://developers.google.com/open-source/licenses/bsd
 */
 
-#include "system.h"
-#include "tree.h"
-
-#include "test_framework.h"
-#include "reftable-tests.h"
+#include "test-lib.h"
+#include "reftable/tree.h"
 
 static int test_compare(const void *a, const void *b)
 {
@@ -24,37 +21,36 @@ struct curry {
 static void check_increasing(void *arg, void *key)
 {
 	struct curry *c = arg;
-	if (c->last) {
-		EXPECT(test_compare(c->last, key) < 0);
-	}
+	if (c->last)
+		check_int(test_compare(c->last, key), <, 0);
 	c->last = key;
 }
 
 static void test_tree(void)
 {
 	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct tree_node *nodes[11] = { 0 };
+	size_t i = 1;
+	struct curry c = { 0 };
 
-	void *values[11] = { NULL };
-	struct tree_node *nodes[11] = { NULL };
-	int i = 1;
-	struct curry c = { NULL };
 	do {
 		nodes[i] = tree_search(values + i, &root, &test_compare, 1);
 		i = (i * 7) % 11;
 	} while (i != 1);
 
 	for (i = 1; i < ARRAY_SIZE(nodes); i++) {
-		EXPECT(values + i == nodes[i]->key);
-		EXPECT(nodes[i] ==
-		       tree_search(values + i, &root, &test_compare, 0));
+		check_pointer_eq(values + i, nodes[i]->key);
+		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
 	}
 
 	infix_walk(root, check_increasing, &c);
 	tree_free(root);
 }
 
-int tree_test_main(int argc, const char *argv[])
+int cmd_main(int argc, const char *argv[])
 {
-	RUN_TEST(test_tree);
-	return 0;
+	TEST(test_tree(), "tree_search and infix_walk work");
+
+	return test_done();
 }
-- 
2.45.2.404.g9eaef5822c
Chandra Pratap· Jun 12, 2024, 12:53 UTC · re: Chandra Pratap · lore

[PATCH v3 3/5] t-reftable-tree: split test_tree() into two sub-test functions

In the current testing setup, tests for both tree_search() and infix_walk() defined by reftable/tree.{c, h} are performed by a single test function, test_tree(). Split tree_test() into test_tree_search() and test_infix_walk() responsible for independently testing tree_search() and infix_walk() respectively. This improves the overall readability of the test file as well as simplifies debugging.

Note that the last parameter in the tree_search() functiom is 'int insert' which when set, inserts the key if it is not found in the tree. Otherwise, the function returns NULL for such cases.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +19 −4
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 208e7b7874..cb721b377a 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -26,13 +26,12 @@ static void check_increasing(void *arg, void *key)
 	c->last = key;
 }
 
-static void test_tree(void)
+static void test_tree_search(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
 	struct tree_node *nodes[11] = { 0 };
 	size_t i = 1;
-	struct curry c = { 0 };
 
 	do {
 		nodes[i] = tree_search(values + i, &root, &test_compare, 1);
@@ -44,13 +43,29 @@ static void test_tree(void)
 		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
 	}
 
-	infix_walk(root, check_increasing, &c);
+	tree_free(root);
+}
+
+static void test_infix_walk(void)
+{
+	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct curry c = { 0 };
+	size_t i = 1;
+
+	do {
+		tree_search(values + i, &root, &test_compare, 1);
+		i = (i * 7) % 11;
+	} while (i != 1);
+
+	infix_walk(root, &check_increasing, &c);
 	tree_free(root);
 }
 
 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");
 
 	return test_done();
 }
-- 
2.45.2.404.g9eaef5822c
Chandra Pratap· Jun 12, 2024, 12:53 UTC · re: Chandra Pratap · lore

[PATCH v3 4/5] t-reftable-tree: add test for non-existent key

In the current testing setup for tree_search(), the case for non-existent key is not exercised. Improve this by adding a test-case for the same.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/unit-tests/t-reftable-tree.c +1 −0
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index cb721b377a..f1adab4458 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -43,6 +43,7 @@ static void test_tree_search(void)
 		check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0));
 	}
 
+	check(!tree_search(values, &root, &test_compare, 0));
 	tree_free(root);
 }
 
-- 
2.45.2.404.g9eaef5822c
Chandra Pratap· Jun 12, 2024, 12:53 UTC · re: Chandra Pratap · lore

[PATCH v3 5/5] t-reftable-tree: improve the test for infix_walk()

In the current testing setup for infix_walk(), the following
properties of an infix traversal of a tree remain untested:
- every node of the tree must be visited
- every node must be visited exactly once
In fact, only the property 'traversal in increasing order' is tested.
Modify test_infix_walk() to check for all the properties above.

This can be achieved by storing the nodes' keys linearly, in a nullified buffer, as we visit them and then checking the input keys against this buffer in increasing order. By checking that the element just after the last input key is 'NULL' in the output buffer, we ensure that every node is traversed exactly once.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +15 −7
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index f1adab4458..79c6bfd49a 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -15,15 +15,14 @@ static int test_compare(const void *a, const void *b)
 }
 
 struct curry {
-	void *last;
+	void **arr;
+	size_t len;
 };
 
-static void check_increasing(void *arg, void *key)
+static void store(void *arg, void *key)
 {
 	struct curry *c = arg;
-	if (c->last)
-		check_int(test_compare(c->last, key), <, 0);
-	c->last = key;
+	c->arr[c->len++] = key;
 }
 
 static void test_tree_search(void)
@@ -51,15 +50,24 @@ static void test_infix_walk(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
-	struct curry c = { 0 };
+	void *out[11] = { 0 };
+	struct curry c = {
+		.arr = (void **) &out,
+	};
 	size_t i = 1;
+	size_t count = 0;
 
 	do {
 		tree_search(values + i, &root, &test_compare, 1);
 		i = (i * 7) % 11;
+		count++;
 	} while (i != 1);
 
-	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(!out[i - 1]);
+	check_int(c.len, ==, count);
 	tree_free(root);
 }
 
-- 
2.45.2.404.g9eaef5822c
Chandra Pratap· Jul 16, 2024, 07:48 UTC · re: Chandra Pratap · lore

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

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);
Chandra Pratap· Jul 16, 2024, 07:48 UTC · re: Chandra Pratap · lore

[PATCH v4 1/5] reftable: remove unnecessary curly braces in reftable/tree.c

According to Documentation/CodingGuidelines, single-line control-flow statements must omit curly braces (except for some special cases). Make reftable/tree.c adhere to this guideline.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 reftable/tree.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
Show changes to reftable/tree.c +5 −10
diff --git a/reftable/tree.c b/reftable/tree.c
index 528f33ae38..5ffb2e0d69 100644
--- a/reftable/tree.c
+++ b/reftable/tree.c
@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,
 void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),
 		void *arg)
 {
-	if (t->left) {
+	if (t->left)
 		infix_walk(t->left, action, arg);
-	}
 	action(arg, t->key);
-	if (t->right) {
+	if (t->right)
 		infix_walk(t->right, action, arg);
-	}
 }
 
 void tree_free(struct tree_node *t)
 {
-	if (!t) {
+	if (!t)
 		return;
-	}
-	if (t->left) {
+	if (t->left)
 		tree_free(t->left);
-	}
-	if (t->right) {
+	if (t->right)
 		tree_free(t->right);
-	}
 	reftable_free(t);
 }
-- 
2.45.GIT
Chandra Pratap· Jul 16, 2024, 07:48 UTC · re: Chandra Pratap · lore

[PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework

reftable/tree_test.c exercises the functions defined in reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit testing framework. Migration involves refactoring the tests to use the unit testing framework instead of reftable's test framework and renaming the tests to align with unit-tests' standards.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                       |  2 +-
 reftable/reftable-tests.h      |  1 -
 reftable/tree_test.c           | 60 ----------------------------------
 t/helper/test-reftable.c       |  1 -
 t/unit-tests/t-reftable-tree.c | 56 +++++++++++++++++++++++++++++++
 5 files changed, 57 insertions(+), 63 deletions(-)
 delete mode 100644 reftable/tree_test.c
 create mode 100644 t/unit-tests/t-reftable-tree.c
Show changes to 5 files +57 −63

Makefile, reftable/reftable-tests.h, reftable/tree_test.c, t/helper/test-reftable.c, t/unit-tests/t-reftable-tree.c

diff --git a/Makefile b/Makefile
index 3eab701b10..79e86ddf53 100644
--- a/Makefile
+++ b/Makefile
@@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-oidtree
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
+UNIT_TEST_PROGRAMS += t-reftable-tree
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-strcmp-offset
 UNIT_TEST_PROGRAMS += t-strvec
@@ -2685,7 +2686,6 @@ REFTABLE_TEST_OBJS += reftable/record_test.o
 REFTABLE_TEST_OBJS += reftable/readwrite_test.o
 REFTABLE_TEST_OBJS += reftable/stack_test.o
 REFTABLE_TEST_OBJS += reftable/test_framework.o
-REFTABLE_TEST_OBJS += reftable/tree_test.o
 
 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index 114cc3d053..d0abcc51e2 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -16,7 +16,6 @@ int pq_test_main(int argc, const char **argv);
 int record_test_main(int argc, const char **argv);
 int readwrite_test_main(int argc, const char **argv);
 int stack_test_main(int argc, const char **argv);
-int tree_test_main(int argc, const char **argv);
 int reftable_dump_main(int argc, char *const *argv);
 
 #endif
diff --git a/reftable/tree_test.c b/reftable/tree_test.c
deleted file mode 100644
index 6961a657ad..0000000000
--- a/reftable/tree_test.c
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
-Copyright 2020 Google LLC
-
-Use of this source code is governed by a BSD-style
-license that can be found in the LICENSE file or at
-https://developers.google.com/open-source/licenses/bsd
-*/
-
-#include "system.h"
-#include "tree.h"
-
-#include "test_framework.h"
-#include "reftable-tests.h"
-
-static int test_compare(const void *a, const void *b)
-{
-	return (char *)a - (char *)b;
-}
-
-struct curry {
-	void *last;
-};
-
-static void check_increasing(void *arg, void *key)
-{
-	struct curry *c = arg;
-	if (c->last) {
-		EXPECT(test_compare(c->last, key) < 0);
-	}
-	c->last = key;
-}
-
-static void test_tree(void)
-{
-	struct tree_node *root = NULL;
-
-	void *values[11] = { NULL };
-	struct tree_node *nodes[11] = { NULL };
-	int i = 1;
-	struct curry c = { NULL };
-	do {
-		nodes[i] = tree_search(values + i, &root, &test_compare, 1);
-		i = (i * 7) % 11;
-	} while (i != 1);
-
-	for (i = 1; i < ARRAY_SIZE(nodes); i++) {
-		EXPECT(values + i == nodes[i]->key);
-		EXPECT(nodes[i] ==
-		       tree_search(values + i, &root, &test_compare, 0));
-	}
-
-	infix_walk(root, check_increasing, &c);
-	tree_free(root);
-}
-
-int tree_test_main(int argc, const char *argv[])
-{
-	RUN_TEST(test_tree);
-	return 0;
-}
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 9160bc5da6..245b674a3c 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -7,7 +7,6 @@ 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);
 	readwrite_test_main(argc, argv);
 	merged_test_main(argc, argv);
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
new file mode 100644
index 0000000000..5df814d983
--- /dev/null
+++ b/t/unit-tests/t-reftable-tree.c
@@ -0,0 +1,56 @@
+/*
+Copyright 2020 Google LLC
+
+Use of this source code is governed by a BSD-style
+license that can be found in the LICENSE file or at
+https://developers.google.com/open-source/licenses/bsd
+*/
+
+#include "test-lib.h"
+#include "reftable/tree.h"
+
+static int t_compare(const void *a, const void *b)
+{
+	return (char *)a - (char *)b;
+}
+
+struct curry {
+	void *last;
+};
+
+static void check_increasing(void *arg, void *key)
+{
+	struct curry *c = arg;
+	if (c->last)
+		check_int(t_compare(c->last, key), <, 0);
+	c->last = key;
+}
+
+static void t_tree(void)
+{
+	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct tree_node *nodes[11] = { 0 };
+	size_t i = 1;
+	struct curry c = { 0 };
+
+	do {
+		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));
+	}
+
+	infix_walk(root, check_increasing, &c);
+	tree_free(root);
+}
+
+int cmd_main(int argc, const char *argv[])
+{
+	TEST(t_tree(), "tree_search and infix_walk work");
+
+	return test_done();
+}
-- 
2.45.GIT
Karthik Nayak· Jul 17, 2024, 11:49 UTC · re: Chandra Pratap · lore

Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework

Chandra Pratap <chandrapratap3519@gmail.com> writes:
Show 6 quoted lines
> reftable/tree_test.c exercises the functions defined in
> reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit
> testing framework. Migration involves refactoring the tests to use
> the unit testing framework instead of reftable's test framework and
> renaming the tests to align with unit-tests' standards.
>
Nit: it would be nice to mention that this commit copies it over as-is
(mostly) and the upcoming commits do refactoring. This would really help
reviewers.
Show 187 quoted lines
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>  Makefile                       |  2 +-
>  reftable/reftable-tests.h      |  1 -
>  reftable/tree_test.c           | 60 ----------------------------------
>  t/helper/test-reftable.c       |  1 -
>  t/unit-tests/t-reftable-tree.c | 56 +++++++++++++++++++++++++++++++
>  5 files changed, 57 insertions(+), 63 deletions(-)
>  delete mode 100644 reftable/tree_test.c
>  create mode 100644 t/unit-tests/t-reftable-tree.c
>
> diff --git a/Makefile b/Makefile
> index 3eab701b10..79e86ddf53 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1340,6 +1340,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
>  UNIT_TEST_PROGRAMS += t-oidtree
>  UNIT_TEST_PROGRAMS += t-prio-queue
>  UNIT_TEST_PROGRAMS += t-reftable-basics
> +UNIT_TEST_PROGRAMS += t-reftable-tree
>  UNIT_TEST_PROGRAMS += t-strbuf
>  UNIT_TEST_PROGRAMS += t-strcmp-offset
>  UNIT_TEST_PROGRAMS += t-strvec
> @@ -2685,7 +2686,6 @@ REFTABLE_TEST_OBJS += reftable/record_test.o
>  REFTABLE_TEST_OBJS += reftable/readwrite_test.o
>  REFTABLE_TEST_OBJS += reftable/stack_test.o
>  REFTABLE_TEST_OBJS += reftable/test_framework.o
> -REFTABLE_TEST_OBJS += reftable/tree_test.o
>
>  TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
>
> diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
> index 114cc3d053..d0abcc51e2 100644
> --- a/reftable/reftable-tests.h
> +++ b/reftable/reftable-tests.h
> @@ -16,7 +16,6 @@ int pq_test_main(int argc, const char **argv);
>  int record_test_main(int argc, const char **argv);
>  int readwrite_test_main(int argc, const char **argv);
>  int stack_test_main(int argc, const char **argv);
> -int tree_test_main(int argc, const char **argv);
>  int reftable_dump_main(int argc, char *const *argv);
>
>  #endif
> diff --git a/reftable/tree_test.c b/reftable/tree_test.c
> deleted file mode 100644
> index 6961a657ad..0000000000
> --- a/reftable/tree_test.c
> +++ /dev/null
> @@ -1,60 +0,0 @@
> -/*
> -Copyright 2020 Google LLC
> -
> -Use of this source code is governed by a BSD-style
> -license that can be found in the LICENSE file or at
> -https://developers.google.com/open-source/licenses/bsd
> -*/
> -
> -#include "system.h"
> -#include "tree.h"
> -
> -#include "test_framework.h"
> -#include "reftable-tests.h"
> -
> -static int test_compare(const void *a, const void *b)
> -{
> -	return (char *)a - (char *)b;
> -}
> -
> -struct curry {
> -	void *last;
> -};
> -
> -static void check_increasing(void *arg, void *key)
> -{
> -	struct curry *c = arg;
> -	if (c->last) {
> -		EXPECT(test_compare(c->last, key) < 0);
> -	}
> -	c->last = key;
> -}
> -
> -static void test_tree(void)
> -{
> -	struct tree_node *root = NULL;
> -
> -	void *values[11] = { NULL };
> -	struct tree_node *nodes[11] = { NULL };
> -	int i = 1;
> -	struct curry c = { NULL };
> -	do {
> -		nodes[i] = tree_search(values + i, &root, &test_compare, 1);
> -		i = (i * 7) % 11;
> -	} while (i != 1);
> -
> -	for (i = 1; i < ARRAY_SIZE(nodes); i++) {
> -		EXPECT(values + i == nodes[i]->key);
> -		EXPECT(nodes[i] ==
> -		       tree_search(values + i, &root, &test_compare, 0));
> -	}
> -
> -	infix_walk(root, check_increasing, &c);
> -	tree_free(root);
> -}
> -
> -int tree_test_main(int argc, const char *argv[])
> -{
> -	RUN_TEST(test_tree);
> -	return 0;
> -}
> diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
> index 9160bc5da6..245b674a3c 100644
> --- a/t/helper/test-reftable.c
> +++ b/t/helper/test-reftable.c
> @@ -7,7 +7,6 @@ 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);
>  	readwrite_test_main(argc, argv);
>  	merged_test_main(argc, argv);
> diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
> new file mode 100644
> index 0000000000..5df814d983
> --- /dev/null
> +++ b/t/unit-tests/t-reftable-tree.c
> @@ -0,0 +1,56 @@
> +/*
> +Copyright 2020 Google LLC
> +
> +Use of this source code is governed by a BSD-style
> +license that can be found in the LICENSE file or at
> +https://developers.google.com/open-source/licenses/bsd
> +*/
> +
> +#include "test-lib.h"
> +#include "reftable/tree.h"
> +
> +static int t_compare(const void *a, const void *b)
> +{
> +	return (char *)a - (char *)b;
> +}
> +
> +struct curry {
> +	void *last;
> +};
> +
> +static void check_increasing(void *arg, void *key)
> +{
> +	struct curry *c = arg;
> +	if (c->last)
> +		check_int(t_compare(c->last, key), <, 0);
> +	c->last = key;
> +}
> +
> +static void t_tree(void)
> +{
> +	struct tree_node *root = NULL;
> +	void *values[11] = { 0 };
> +	struct tree_node *nodes[11] = { 0 };
> +	size_t i = 1;
> +	struct curry c = { 0 };
> +
> +	do {
> +		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));
> +	}
> +
> +	infix_walk(root, check_increasing, &c);
> +	tree_free(root);
> +}
> +
> +int cmd_main(int argc, const char *argv[])
> +{
> +	TEST(t_tree(), "tree_search and infix_walk work");
> +
> +	return test_done();
> +}
> --
> 2.45.GIT
Chandra Pratap· Jul 17, 2024, 13:50 UTC · re: Karthik Nayak · lore

Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework

On Wed, 17 Jul 2024 at 17:19, Karthik Nayak <karthik.188@gmail.com> wrote:
Show 13 quoted lines
>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > reftable/tree_test.c exercises the functions defined in
> > reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit
> > testing framework. Migration involves refactoring the tests to use
> > the unit testing framework instead of reftable's test framework and
> > renaming the tests to align with unit-tests' standards.
> >
>
> Nit: it would be nice to mention that this commit copies it over as-is
> (mostly) and the upcoming commits do refactoring. This would really help
> reviewers.
I do mention that in the patch cover letter, specifically this paragraph:
> 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.
Would it be better if I transport that here?
----[snip]----
Karthik Nayak· Jul 18, 2024, 08:04 UTC · re: Chandra Pratap · lore

Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework

Chandra Pratap <chandrapratap3519@gmail.com> writes:
Show 24 quoted lines
> On Wed, 17 Jul 2024 at 17:19, Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>>
>> > reftable/tree_test.c exercises the functions defined in
>> > reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit
>> > testing framework. Migration involves refactoring the tests to use
>> > the unit testing framework instead of reftable's test framework and
>> > renaming the tests to align with unit-tests' standards.
>> >
>>
>> Nit: it would be nice to mention that this commit copies it over as-is
>> (mostly) and the upcoming commits do refactoring. This would really help
>> reviewers.
>
> I do mention that in the patch cover letter, specifically this paragraph:
>
>> 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.
>
> Would it be better if I transport that here?
>
> ----[snip]----
I think that would be nice!
Karthik Nayak· Jul 17, 2024, 12:39 UTC · re: Chandra Pratap · lore

Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework

Chandra Pratap <chandrapratap3519@gmail.com> writes:
Show 6 quoted lines
> reftable/tree_test.c exercises the functions defined in
> reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit
> testing framework. Migration involves refactoring the tests to use
> the unit testing framework instead of reftable's test framework and
> renaming the tests to align with unit-tests' standards.
>

On second thought, it's easier for me to review here for the existing state of the code. So let me do that..

[snip]
Show 22 quoted lines
> diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
> new file mode 100644
> index 0000000000..5df814d983
> --- /dev/null
> +++ b/t/unit-tests/t-reftable-tree.c
> @@ -0,0 +1,56 @@
> +/*
> +Copyright 2020 Google LLC
> +
> +Use of this source code is governed by a BSD-style
> +license that can be found in the LICENSE file or at
> +https://developers.google.com/open-source/licenses/bsd
> +*/
> +
> +#include "test-lib.h"
> +#include "reftable/tree.h"
> +
> +static int t_compare(const void *a, const void *b)
> +{
> +	return (char *)a - (char *)b;
> +}
> +

So this is the comparison code, and we're expecting the values to be a character. Okay.

Show 16 quoted lines
> +struct curry {
> +	void *last;
> +};
> +
> +static void check_increasing(void *arg, void *key)
> +{
> +	struct curry *c = arg;
> +	if (c->last)
> +		check_int(t_compare(c->last, key), <, 0);
> +	c->last = key;
> +}
> +
> +static void t_tree(void)
> +{
> +	struct tree_node *root = NULL;
> +	void *values[11] = { 0 };

Although we were comparing 'char' above, here we have a 'void *' array. Why?

Show 7 quoted lines
> +	struct tree_node *nodes[11] = { 0 };
> +	size_t i = 1;
> +	struct curry c = { 0 };
> +
> +	do {
> +		nodes[i] = tree_search(values + i, &root, &t_compare, 1);
> +		i = (i * 7) % 11;

It gets weirder, we calculate 'i' as {7, 5, 2, 3, 10, 4, 6, 9, 8, 1}. We use that to index 'values', but values is '0' initialized, so we always send '0' to tree_search? Doesn't that make this whole thing a moot? Or did I miss something?

Show 19 quoted lines
> +	} 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));
> +	}
> +
> +	infix_walk(root, check_increasing, &c);
> +	tree_free(root);
> +}
> +
> +int cmd_main(int argc, const char *argv[])
> +{
> +	TEST(t_tree(), "tree_search and infix_walk work");
> +
> +	return test_done();
> +}
> --
> 2.45.GIT
Chandra Pratap· Jul 17, 2024, 14:30 UTC · re: Karthik Nayak · lore

Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework

On Wed, 17 Jul 2024 at 18:09, Karthik Nayak <karthik.188@gmail.com> wrote:
Show 40 quoted lines
>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > reftable/tree_test.c exercises the functions defined in
> > reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit
> > testing framework. Migration involves refactoring the tests to use
> > the unit testing framework instead of reftable's test framework and
> > renaming the tests to align with unit-tests' standards.
> >
>
> On second thought, it's easier for me to review here for the existing
> state of the code. So let me do that..
>
> [snip]
>
> > diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
> > new file mode 100644
> > index 0000000000..5df814d983
> > --- /dev/null
> > +++ b/t/unit-tests/t-reftable-tree.c
> > @@ -0,0 +1,56 @@
> > +/*
> > +Copyright 2020 Google LLC
> > +
> > +Use of this source code is governed by a BSD-style
> > +license that can be found in the LICENSE file or at
> > +https://developers.google.com/open-source/licenses/bsd
> > +*/
> > +
> > +#include "test-lib.h"
> > +#include "reftable/tree.h"
> > +
> > +static int t_compare(const void *a, const void *b)
> > +{
> > +     return (char *)a - (char *)b;
> > +}
> > +
>
> So this is the comparison code, and we're expecting the values to be a
> character. Okay.

We're actually expecting the values 'a' and 'b' to be of the type (char *), which is a pointer to a character, and thus we perform the comparison on the basis of pointer arithmetic.

Show 19 quoted lines
> > +struct curry {
> > +     void *last;
> > +};
> > +
> > +static void check_increasing(void *arg, void *key)
> > +{
> > +     struct curry *c = arg;
> > +     if (c->last)
> > +             check_int(t_compare(c->last, key), <, 0);
> > +     c->last = key;
> > +}
> > +
> > +static void t_tree(void)
> > +{
> > +     struct tree_node *root = NULL;
> > +     void *values[11] = { 0 };
>
> Although we were comparing 'char' above, here we have a 'void *' array.
> Why?

The array is passed as a parameter to the 'tree_search()' function which requires a void * parameter (i.e. a generic pointer). In the comparison function (also passed as a parameter), we cast it to our expected type (a character pointer) and then perform the required comparison.

Show 12 quoted lines
> > +     struct tree_node *nodes[11] = { 0 };
> > +     size_t i = 1;
> > +     struct curry c = { 0 };
> > +
> > +     do {
> > +             nodes[i] = tree_search(values + i, &root, &t_compare, 1);
> > +             i = (i * 7) % 11;
>
> It gets weirder, we calculate 'i' as {7, 5, 2, 3, 10, 4, 6, 9, 8, 1}. We
> use that to index 'values', but values is '0' initialized, so we always
> send '0' to tree_search? Doesn't that make this whole thing a moot? Or
> did I miss something?

We don't use 'i' to index 'values[]', we use it to calculate the next pointer address to be passed to the 'tree_search()' function (the pointer that is 'i' ahead of the pointer 'values'), which isn't 0.

Show 19 quoted lines
> > +     } 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));
> > +     }
> > +
> > +     infix_walk(root, check_increasing, &c);
> > +     tree_free(root);
> > +}
> > +
> > +int cmd_main(int argc, const char *argv[])
> > +{
> > +     TEST(t_tree(), "tree_search and infix_walk work");
> > +
> > +     return test_done();
> > +}
> > --
> > 2.45.GIT
Justin Tobler· Jul 17, 2024, 22:14 UTC · re: Chandra Pratap · lore

Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework

On 24/07/17 08:00PM, Chandra Pratap wrote:
Show 28 quoted lines
> On Wed, 17 Jul 2024 at 18:09, Karthik Nayak <karthik.188@gmail.com> wrote:
> >
> > Chandra Pratap <chandrapratap3519@gmail.com> writes:
> >
> > > +struct curry {
> > > +     void *last;
> > > +};
> > > +
> > > +static void check_increasing(void *arg, void *key)
> > > +{
> > > +     struct curry *c = arg;
> > > +     if (c->last)
> > > +             check_int(t_compare(c->last, key), <, 0);
> > > +     c->last = key;
> > > +}
> > > +
> > > +static void t_tree(void)
> > > +{
> > > +     struct tree_node *root = NULL;
> > > +     void *values[11] = { 0 };
> >
> > Although we were comparing 'char' above, here we have a 'void *' array.
> > Why?
> 
> The array is passed as a parameter to the 'tree_search()' function which
> requires a void * parameter (i.e. a generic pointer). In the comparison
> function (also passed as a parameter), we cast it to our expected type
> (a character pointer) and then perform the required comparison.

The point of `values` is to provide a set of values of type `void **` to be inserted in the tree. As far as I can tell, there is no reason for `values` to be initialized to begin with and is a bit misleading. Might be reasonable to remove its initialization here.

Show 16 quoted lines
> > > +     struct tree_node *nodes[11] = { 0 };
> > > +     size_t i = 1;
> > > +     struct curry c = { 0 };
> > > +
> > > +     do {
> > > +             nodes[i] = tree_search(values + i, &root, &t_compare, 1);
> > > +             i = (i * 7) % 11;
> >
> > It gets weirder, we calculate 'i' as {7, 5, 2, 3, 10, 4, 6, 9, 8, 1}. We
> > use that to index 'values', but values is '0' initialized, so we always
> > send '0' to tree_search? Doesn't that make this whole thing a moot? Or
> > did I miss something?
> 
> We don't use 'i' to index 'values[]', we use it to calculate the next pointer
> address to be passed to the 'tree_search()' function (the pointer that is 'i'
> ahead of the pointer 'values'), which isn't 0.

The `i = (i * 7) % 11;` is used to deterministically generate numbers 1-10 in a psuedo-random fashion. These numbers are used as memory offsets to be inserted into the tree. I suspect the psuedo-randomness is useful keys should be ordered when inserted into the tree and that is later validated as part of the in-order traversal that is performed.

While rather compact, I find the test setup here to rather difficult to parse. It might be a good idea to either provide comments explaining this test setup or consider refactoring it. Honestly, I'd personally perfer the tree setup be done more explicitly as I think it would make understanding the test much easier.

-Justin
Chandra Pratap· Jul 18, 2024, 04:58 UTC · re: Justin Tobler · lore

Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework

On Thu, 18 Jul 2024 at 03:45, Justin Tobler <jltobler@gmail.com> wrote:
Show 35 quoted lines
>
> On 24/07/17 08:00PM, Chandra Pratap wrote:
> > On Wed, 17 Jul 2024 at 18:09, Karthik Nayak <karthik.188@gmail.com> wrote:
> > >
> > > Chandra Pratap <chandrapratap3519@gmail.com> writes:
> > >
> > > > +struct curry {
> > > > +     void *last;
> > > > +};
> > > > +
> > > > +static void check_increasing(void *arg, void *key)
> > > > +{
> > > > +     struct curry *c = arg;
> > > > +     if (c->last)
> > > > +             check_int(t_compare(c->last, key), <, 0);
> > > > +     c->last = key;
> > > > +}
> > > > +
> > > > +static void t_tree(void)
> > > > +{
> > > > +     struct tree_node *root = NULL;
> > > > +     void *values[11] = { 0 };
> > >
> > > Although we were comparing 'char' above, here we have a 'void *' array.
> > > Why?
> >
> > The array is passed as a parameter to the 'tree_search()' function which
> > requires a void * parameter (i.e. a generic pointer). In the comparison
> > function (also passed as a parameter), we cast it to our expected type
> > (a character pointer) and then perform the required comparison.
>
> The point of `values` is to provide a set of values of type `void **` to
> be inserted in the tree. As far as I can tell, there is no reason for
> `values` to be initialized to begin with and is a bit misleading. Might
> be reasonable to remove its initialization here.

The thing is, the values[] array being 0-initialized makes debugging a lot easier in the case of a test failure, so I'm not very sure about getting rid of the initialization here.

Show 22 quoted lines
> > > > +     struct tree_node *nodes[11] = { 0 };
> > > > +     size_t i = 1;
> > > > +     struct curry c = { 0 };
> > > > +
> > > > +     do {
> > > > +             nodes[i] = tree_search(values + i, &root, &t_compare, 1);
> > > > +             i = (i * 7) % 11;
> > >
> > > It gets weirder, we calculate 'i' as {7, 5, 2, 3, 10, 4, 6, 9, 8, 1}. We
> > > use that to index 'values', but values is '0' initialized, so we always
> > > send '0' to tree_search? Doesn't that make this whole thing a moot? Or
> > > did I miss something?
> >
> > We don't use 'i' to index 'values[]', we use it to calculate the next pointer
> > address to be passed to the 'tree_search()' function (the pointer that is 'i'
> > ahead of the pointer 'values'), which isn't 0.
>
> The `i = (i * 7) % 11;` is used to deterministically generate numbers
> 1-10 in a psuedo-random fashion. These numbers are used as memory
> offsets to be inserted into the tree. I suspect the psuedo-randomness is
> useful keys should be ordered when inserted into the tree and that is
> later validated as part of the in-order traversal that is performed.

That's right, the randomness of the insertion order is helpful in validating that the tree-functions 'tree_search()' and 'infix_walk()' work according to their defined behaviour.

Show 5 quoted lines
> While rather compact, I find the test setup here to rather difficult to
> parse. It might be a good idea to either provide comments explaining
> this test setup or consider refactoring it. Honestly, I'd personally
> perfer the tree setup be done more explicitly as I think it would make
> understanding the test much easier.

This probably ties in with the comments by Patrick on the previous iteration of this patch, that using 'tree_search()' to insert tree nodes leads to confusion. Solving that would require efforts outside the scope of this patch series though, so I'm more inclined towards providing comments and other ways of simplifying this subroutine.

Karthik Nayak· Jul 18, 2024, 08:10 UTC · re: Chandra Pratap · lore

Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework

Chandra Pratap <chandrapratap3519@gmail.com> writes:
Show 79 quoted lines
> On Thu, 18 Jul 2024 at 03:45, Justin Tobler <jltobler@gmail.com> wrote:
>>
>> On 24/07/17 08:00PM, Chandra Pratap wrote:
>> > On Wed, 17 Jul 2024 at 18:09, Karthik Nayak <karthik.188@gmail.com> wrote:
>> > >
>> > > Chandra Pratap <chandrapratap3519@gmail.com> writes:
>> > >
>> > > > +struct curry {
>> > > > +     void *last;
>> > > > +};
>> > > > +
>> > > > +static void check_increasing(void *arg, void *key)
>> > > > +{
>> > > > +     struct curry *c = arg;
>> > > > +     if (c->last)
>> > > > +             check_int(t_compare(c->last, key), <, 0);
>> > > > +     c->last = key;
>> > > > +}
>> > > > +
>> > > > +static void t_tree(void)
>> > > > +{
>> > > > +     struct tree_node *root = NULL;
>> > > > +     void *values[11] = { 0 };
>> > >
>> > > Although we were comparing 'char' above, here we have a 'void *' array.
>> > > Why?
>> >
>> > The array is passed as a parameter to the 'tree_search()' function which
>> > requires a void * parameter (i.e. a generic pointer). In the comparison
>> > function (also passed as a parameter), we cast it to our expected type
>> > (a character pointer) and then perform the required comparison.
>>
>> The point of `values` is to provide a set of values of type `void **` to
>> be inserted in the tree. As far as I can tell, there is no reason for
>> `values` to be initialized to begin with and is a bit misleading. Might
>> be reasonable to remove its initialization here.
>
> The thing is, the values[] array being 0-initialized makes debugging
> a lot easier in the case of a test failure, so I'm not very sure about
> getting rid of the initialization here.
>
>> > > > +     struct tree_node *nodes[11] = { 0 };
>> > > > +     size_t i = 1;
>> > > > +     struct curry c = { 0 };
>> > > > +
>> > > > +     do {
>> > > > +             nodes[i] = tree_search(values + i, &root, &t_compare, 1);
>> > > > +             i = (i * 7) % 11;
>> > >
>> > > It gets weirder, we calculate 'i' as {7, 5, 2, 3, 10, 4, 6, 9, 8, 1}. We
>> > > use that to index 'values', but values is '0' initialized, so we always
>> > > send '0' to tree_search? Doesn't that make this whole thing a moot? Or
>> > > did I miss something?
>> >
>> > We don't use 'i' to index 'values[]', we use it to calculate the next pointer
>> > address to be passed to the 'tree_search()' function (the pointer that is 'i'
>> > ahead of the pointer 'values'), which isn't 0.
>>
>> The `i = (i * 7) % 11;` is used to deterministically generate numbers
>> 1-10 in a psuedo-random fashion. These numbers are used as memory
>> offsets to be inserted into the tree. I suspect the psuedo-randomness is
>> useful keys should be ordered when inserted into the tree and that is
>> later validated as part of the in-order traversal that is performed.
>
> That's right, the randomness of the insertion order is helpful in validating
> that the tree-functions 'tree_search()' and 'infix_walk()' work according
> to their defined behaviour.
>
>> While rather compact, I find the test setup here to rather difficult to
>> parse. It might be a good idea to either provide comments explaining
>> this test setup or consider refactoring it. Honestly, I'd personally
>> perfer the tree setup be done more explicitly as I think it would make
>> understanding the test much easier.
>
> This probably ties in with the comments by Patrick on the previous iteration
> of this patch, that using 'tree_search()' to insert tree nodes leads to
> confusion. Solving that would require efforts outside the scope of this
> patch series though, so I'm more inclined towards providing comments
> and other ways of simplifying this subroutine.

Agreed that refactoring `tree_search()` probably is out of scope here. But rewriting the test is definitely something we can do.

Perhaps:
static void t_tree(void)
{
	struct tree_node *root = NULL;
	int values[11] = {7, 5, 2, 3, 10, 4, 6, 9, 8, 1};
	struct tree_node *nodes[11] = { 0 };
	size_t i = 1;
	struct curry c = { 0 };
    // Insert values to the tree by passing '1' as the last argument.
    for (i = 1; i < ARRAY_SIZE(values); i++) {
		nodes[i] = tree_search(&values[i], &root, &t_compare, 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));
	}
	infix_walk(root, check_increasing, &c);
	tree_free(root);
}
Wouldn't this have the same effect while making it much easier to read?
Chandra Pratap· Jul 18, 2024, 08:23 UTC · re: Karthik Nayak · lore

Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework

On Thu, 18 Jul 2024 at 13:40, Karthik Nayak <karthik.188@gmail.com> wrote:
Show 111 quoted lines
>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > On Thu, 18 Jul 2024 at 03:45, Justin Tobler <jltobler@gmail.com> wrote:
> >>
> >> On 24/07/17 08:00PM, Chandra Pratap wrote:
> >> > On Wed, 17 Jul 2024 at 18:09, Karthik Nayak <karthik.188@gmail.com> wrote:
> >> > >
> >> > > Chandra Pratap <chandrapratap3519@gmail.com> writes:
> >> > >
> >> > > > +struct curry {
> >> > > > +     void *last;
> >> > > > +};
> >> > > > +
> >> > > > +static void check_increasing(void *arg, void *key)
> >> > > > +{
> >> > > > +     struct curry *c = arg;
> >> > > > +     if (c->last)
> >> > > > +             check_int(t_compare(c->last, key), <, 0);
> >> > > > +     c->last = key;
> >> > > > +}
> >> > > > +
> >> > > > +static void t_tree(void)
> >> > > > +{
> >> > > > +     struct tree_node *root = NULL;
> >> > > > +     void *values[11] = { 0 };
> >> > >
> >> > > Although we were comparing 'char' above, here we have a 'void *' array.
> >> > > Why?
> >> >
> >> > The array is passed as a parameter to the 'tree_search()' function which
> >> > requires a void * parameter (i.e. a generic pointer). In the comparison
> >> > function (also passed as a parameter), we cast it to our expected type
> >> > (a character pointer) and then perform the required comparison.
> >>
> >> The point of `values` is to provide a set of values of type `void **` to
> >> be inserted in the tree. As far as I can tell, there is no reason for
> >> `values` to be initialized to begin with and is a bit misleading. Might
> >> be reasonable to remove its initialization here.
> >
> > The thing is, the values[] array being 0-initialized makes debugging
> > a lot easier in the case of a test failure, so I'm not very sure about
> > getting rid of the initialization here.
> >
> >> > > > +     struct tree_node *nodes[11] = { 0 };
> >> > > > +     size_t i = 1;
> >> > > > +     struct curry c = { 0 };
> >> > > > +
> >> > > > +     do {
> >> > > > +             nodes[i] = tree_search(values + i, &root, &t_compare, 1);
> >> > > > +             i = (i * 7) % 11;
> >> > >
> >> > > It gets weirder, we calculate 'i' as {7, 5, 2, 3, 10, 4, 6, 9, 8, 1}. We
> >> > > use that to index 'values', but values is '0' initialized, so we always
> >> > > send '0' to tree_search? Doesn't that make this whole thing a moot? Or
> >> > > did I miss something?
> >> >
> >> > We don't use 'i' to index 'values[]', we use it to calculate the next pointer
> >> > address to be passed to the 'tree_search()' function (the pointer that is 'i'
> >> > ahead of the pointer 'values'), which isn't 0.
> >>
> >> The `i = (i * 7) % 11;` is used to deterministically generate numbers
> >> 1-10 in a psuedo-random fashion. These numbers are used as memory
> >> offsets to be inserted into the tree. I suspect the psuedo-randomness is
> >> useful keys should be ordered when inserted into the tree and that is
> >> later validated as part of the in-order traversal that is performed.
> >
> > That's right, the randomness of the insertion order is helpful in validating
> > that the tree-functions 'tree_search()' and 'infix_walk()' work according
> > to their defined behaviour.
> >
> >> While rather compact, I find the test setup here to rather difficult to
> >> parse. It might be a good idea to either provide comments explaining
> >> this test setup or consider refactoring it. Honestly, I'd personally
> >> perfer the tree setup be done more explicitly as I think it would make
> >> understanding the test much easier.
> >
> > This probably ties in with the comments by Patrick on the previous iteration
> > of this patch, that using 'tree_search()' to insert tree nodes leads to
> > confusion. Solving that would require efforts outside the scope of this
> > patch series though, so I'm more inclined towards providing comments
> > and other ways of simplifying this subroutine.
>
> Agreed that refactoring `tree_search()` probably is out of scope here.
> But rewriting the test is definitely something we can do.
>
> Perhaps:
>
> static void t_tree(void)
> {
>         struct tree_node *root = NULL;
>         int values[11] = {7, 5, 2, 3, 10, 4, 6, 9, 8, 1};
>         struct tree_node *nodes[11] = { 0 };
>         size_t i = 1;
>         struct curry c = { 0 };
>
>     // Insert values to the tree by passing '1' as the last argument.
>     for (i = 1; i < ARRAY_SIZE(values); i++) {
>                 nodes[i] = tree_search(&values[i], &root, &t_compare, 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));
>         }
>
>         infix_walk(root, check_increasing, &c);
>         tree_free(root);
> }
>
> Wouldn't this have the same effect while making it much easier to read?

I agree that the change 'values + i -> &values[i]' is a net positive, I had this change in mind as well. This comment on the other hand,

>     // Insert values to the tree by passing '1' as the last argument.

has already been stated in the commit message of the 3rd patch as was suggested by Patrick earlier:

'Note that the last parameter in the tree_search() function is 'int insert' which when set, inserts the key if it is not found in the tree. Otherwise, the function returns NULL for such cases.'

So I was thinking of adding something along the lines of: '// pseudo-randomly insert pointers to elements between values[1] and values[10] in the tree'

Justin Tobler· Jul 18, 2024, 15:26 UTC · re: Karthik Nayak · lore

Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework

On 24/07/18 01:10AM, Karthik Nayak wrote:
Show 56 quoted lines
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
> 
> > On Thu, 18 Jul 2024 at 03:45, Justin Tobler <jltobler@gmail.com> wrote:
> >>
> >> On 24/07/17 08:00PM, Chandra Pratap wrote:
>
> >> The `i = (i * 7) % 11;` is used to deterministically generate numbers
> >> 1-10 in a psuedo-random fashion. These numbers are used as memory
> >> offsets to be inserted into the tree. I suspect the psuedo-randomness is
> >> useful keys should be ordered when inserted into the tree and that is
> >> later validated as part of the in-order traversal that is performed.
> >
> > That's right, the randomness of the insertion order is helpful in validating
> > that the tree-functions 'tree_search()' and 'infix_walk()' work according
> > to their defined behaviour.
> >
> >> While rather compact, I find the test setup here to rather difficult to
> >> parse. It might be a good idea to either provide comments explaining
> >> this test setup or consider refactoring it. Honestly, I'd personally
> >> perfer the tree setup be done more explicitly as I think it would make
> >> understanding the test much easier.
> >
> > This probably ties in with the comments by Patrick on the previous iteration
> > of this patch, that using 'tree_search()' to insert tree nodes leads to
> > confusion. Solving that would require efforts outside the scope of this
> > patch series though, so I'm more inclined towards providing comments
> > and other ways of simplifying this subroutine.
> 
> Agreed that refactoring `tree_search()` probably is out of scope here.
> But rewriting the test is definitely something we can do.
> 
> Perhaps:
> 
> static void t_tree(void)
> {
> 	struct tree_node *root = NULL;
> 	int values[11] = {7, 5, 2, 3, 10, 4, 6, 9, 8, 1};
> 	struct tree_node *nodes[11] = { 0 };
> 	size_t i = 1;
> 	struct curry c = { 0 };
> 
>     // Insert values to the tree by passing '1' as the last argument.
>     for (i = 1; i < ARRAY_SIZE(values); i++) {
> 		nodes[i] = tree_search(&values[i], &root, &t_compare, 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));
> 	}
> 
> 	infix_walk(root, check_increasing, &c);
> 	tree_free(root);
> }
> 
> Wouldn't this have the same effect while making it much easier to read?

Personally, I quite like this approach. It's more up front with what its doing and ultimately accoplishes the same thing.

Chandra Pratap· Jul 16, 2024, 07:48 UTC · re: Chandra Pratap · lore

[PATCH v4 3/5] t-reftable-tree: split test_tree() into two sub-test functions

In the current testing setup, tests for both tree_search() and infix_walk() defined by reftable/tree.{c, h} are performed by a single test function, test_tree(). Split tree_test() into test_tree_search() and test_infix_walk() responsible for independently testing tree_search() and infix_walk() respectively. This improves the overall readability of the test file as well as simplifies debugging.

Note that the last parameter in the tree_search() functiom is 'int insert' which when set, inserts the key if it is not found in the tree. Otherwise, the function returns NULL for such cases.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +19 −4
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 5df814d983..68b1b31176 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -26,13 +26,12 @@ static void check_increasing(void *arg, void *key)
 	c->last = key;
 }
 
-static void t_tree(void)
+static void t_tree_search(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
 	struct tree_node *nodes[11] = { 0 };
 	size_t i = 1;
-	struct curry c = { 0 };
 
 	do {
 		nodes[i] = tree_search(values + i, &root, &t_compare, 1);
@@ -44,13 +43,29 @@ 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 t_infix_walk(void)
+{
+	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct curry c = { 0 };
+	size_t i = 1;
+
+	do {
+		tree_search(values + i, &root, &t_compare, 1);
+		i = (i * 7) % 11;
+	} while (i != 1);
+
+	infix_walk(root, &check_increasing, &c);
 	tree_free(root);
 }
 
 int cmd_main(int argc, const char *argv[])
 {
-	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();
 }
-- 
2.45.GIT
Chandra Pratap· Jul 16, 2024, 07:48 UTC · re: Chandra Pratap · lore

[PATCH v4 4/5] t-reftable-tree: add test for non-existent key

In the current testing setup for tree_search(), the case for non-existent key is not exercised. Improve this by adding a test-case for the same.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/unit-tests/t-reftable-tree.c +1 −0
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 68b1b31176..e04e555509 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -43,6 +43,7 @@ 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));
 	tree_free(root);
 }
 
-- 
2.45.GIT
Chandra Pratap· Jul 16, 2024, 07:48 UTC · re: Chandra Pratap · lore

[PATCH v4 5/5] t-reftable-tree: improve the test for infix_walk()

In the current testing setup for infix_walk(), the following
properties of an infix traversal of a tree remain untested:
- every node of the tree must be visited
- every node must be visited exactly once
In fact, only the property 'traversal in increasing order' is tested.
Modify test_infix_walk() to check for all the properties above.

This can be achieved by storing the nodes' keys linearly, in a nullified buffer, as we visit them and then checking the input keys against this buffer in increasing order. By checking that the element just after the last input key is 'NULL' in the output buffer, we ensure that every node is traversed exactly once.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +15 −7
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index e04e555509..b3d4008e5c 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -15,15 +15,14 @@ static int t_compare(const void *a, const void *b)
 }
 
 struct curry {
-	void *last;
+	void **arr;
+	size_t len;
 };
 
-static void check_increasing(void *arg, void *key)
+static void store(void *arg, void *key)
 {
 	struct curry *c = arg;
-	if (c->last)
-		check_int(t_compare(c->last, key), <, 0);
-	c->last = key;
+	c->arr[c->len++] = key;
 }
 
 static void t_tree_search(void)
@@ -51,15 +50,24 @@ static void t_infix_walk(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
-	struct curry c = { 0 };
+	void *out[11] = { 0 };
+	struct curry c = {
+		.arr = (void **) &out,
+	};
 	size_t i = 1;
+	size_t count = 0;
 
 	do {
 		tree_search(values + i, &root, &t_compare, 1);
 		i = (i * 7) % 11;
+		count++;
 	} while (i != 1);
 
-	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(!out[i - 1]);
+	check_int(c.len, ==, count);
 	tree_free(root);
 }
 
-- 
2.45.GIT
Junio C Hamano· Jul 16, 2024, 19:52 UTC · re: Chandra Pratap · lore

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

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.
Show 18 quoted lines
> 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(-)
Show changes to t/unit-tests/t-reftable-tree.c +4 −4
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);
-- 
2.46.0-rc0-137-g851401d64b
Chandra Pratap· Jul 22, 2024, 05:57 UTC · re: Chandra Pratap · lore

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

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 <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
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 <ps@pks.im>
        Mentored-by: Christian Couder <chriscool@tuxfamily.org>
        Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
     ## 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 <ps@pks.im>
         Mentored-by: Christian Couder <chriscool@tuxfamily.org>
         Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
    @@ 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);
Chandra Pratap· Jul 22, 2024, 05:57 UTC · re: Chandra Pratap · lore

[PATCH v5 1/5] reftable: remove unnecessary curly braces in reftable/tree.c

According to Documentation/CodingGuidelines, single-line control-flow statements must omit curly braces (except for some special cases). Make reftable/tree.c adhere to this guideline.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 reftable/tree.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
Show changes to reftable/tree.c +5 −10
diff --git a/reftable/tree.c b/reftable/tree.c
index 528f33ae38..5ffb2e0d69 100644
--- a/reftable/tree.c
+++ b/reftable/tree.c
@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,
 void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),
 		void *arg)
 {
-	if (t->left) {
+	if (t->left)
 		infix_walk(t->left, action, arg);
-	}
 	action(arg, t->key);
-	if (t->right) {
+	if (t->right)
 		infix_walk(t->right, action, arg);
-	}
 }
 
 void tree_free(struct tree_node *t)
 {
-	if (!t) {
+	if (!t)
 		return;
-	}
-	if (t->left) {
+	if (t->left)
 		tree_free(t->left);
-	}
-	if (t->right) {
+	if (t->right)
 		tree_free(t->right);
-	}
 	reftable_free(t);
 }
-- 
2.45.GIT
Chandra Pratap· Jul 22, 2024, 05:57 UTC · re: Chandra Pratap · lore

[PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework

reftable/tree_test.c exercises the functions defined in reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit testing framework. Migration involves refactoring the tests to use 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 <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                       |  2 +-
 reftable/reftable-tests.h      |  1 -
 reftable/tree_test.c           | 60 ----------------------------------
 t/helper/test-reftable.c       |  1 -
 t/unit-tests/t-reftable-tree.c | 59 +++++++++++++++++++++++++++++++++
 5 files changed, 60 insertions(+), 63 deletions(-)
 delete mode 100644 reftable/tree_test.c
 create mode 100644 t/unit-tests/t-reftable-tree.c
Show changes to 5 files +60 −63

Makefile, reftable/reftable-tests.h, reftable/tree_test.c, t/helper/test-reftable.c, t/unit-tests/t-reftable-tree.c

diff --git a/Makefile b/Makefile
index d6479092a0..6f423a2a1e 100644
--- a/Makefile
+++ b/Makefile
@@ -1341,6 +1341,7 @@ 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
@@ -2685,7 +2686,6 @@ 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_TEST_OBJS += reftable/tree_test.o
 
 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index 114cc3d053..d0abcc51e2 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -16,7 +16,6 @@ int pq_test_main(int argc, const char **argv);
 int record_test_main(int argc, const char **argv);
 int readwrite_test_main(int argc, const char **argv);
 int stack_test_main(int argc, const char **argv);
-int tree_test_main(int argc, const char **argv);
 int reftable_dump_main(int argc, char *const *argv);
 
 #endif
diff --git a/reftable/tree_test.c b/reftable/tree_test.c
deleted file mode 100644
index 6961a657ad..0000000000
--- a/reftable/tree_test.c
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
-Copyright 2020 Google LLC
-
-Use of this source code is governed by a BSD-style
-license that can be found in the LICENSE file or at
-https://developers.google.com/open-source/licenses/bsd
-*/
-
-#include "system.h"
-#include "tree.h"
-
-#include "test_framework.h"
-#include "reftable-tests.h"
-
-static int test_compare(const void *a, const void *b)
-{
-	return (char *)a - (char *)b;
-}
-
-struct curry {
-	void *last;
-};
-
-static void check_increasing(void *arg, void *key)
-{
-	struct curry *c = arg;
-	if (c->last) {
-		EXPECT(test_compare(c->last, key) < 0);
-	}
-	c->last = key;
-}
-
-static void test_tree(void)
-{
-	struct tree_node *root = NULL;
-
-	void *values[11] = { NULL };
-	struct tree_node *nodes[11] = { NULL };
-	int i = 1;
-	struct curry c = { NULL };
-	do {
-		nodes[i] = tree_search(values + i, &root, &test_compare, 1);
-		i = (i * 7) % 11;
-	} while (i != 1);
-
-	for (i = 1; i < ARRAY_SIZE(nodes); i++) {
-		EXPECT(values + i == nodes[i]->key);
-		EXPECT(nodes[i] ==
-		       tree_search(values + i, &root, &test_compare, 0));
-	}
-
-	infix_walk(root, check_increasing, &c);
-	tree_free(root);
-}
-
-int tree_test_main(int argc, const char *argv[])
-{
-	RUN_TEST(test_tree);
-	return 0;
-}
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index aa6538a8da..8c41ef7c9d 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -6,7 +6,6 @@ int cmd__reftable(int argc, const char **argv)
 {
 	/* test from simple to complex. */
 	block_test_main(argc, argv);
-	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
 	merged_test_main(argc, argv);
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
new file mode 100644
index 0000000000..08f1873ad3
--- /dev/null
+++ b/t/unit-tests/t-reftable-tree.c
@@ -0,0 +1,59 @@
+/*
+Copyright 2020 Google LLC
+
+Use of this source code is governed by a BSD-style
+license that can be found in the LICENSE file or at
+https://developers.google.com/open-source/licenses/bsd
+*/
+
+#include "test-lib.h"
+#include "reftable/tree.h"
+
+static int t_compare(const void *a, const void *b)
+{
+	return (char *)a - (char *)b;
+}
+
+struct curry {
+	void *last;
+};
+
+static void check_increasing(void *arg, void *key)
+{
+	struct curry *c = arg;
+	if (c->last)
+		check_int(t_compare(c->last, key), <, 0);
+	c->last = key;
+}
+
+static void t_tree(void)
+{
+	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct tree_node *nodes[11] = { 0 };
+	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);
+		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));
+	}
+
+	infix_walk(root, check_increasing, &c);
+	tree_free(root);
+}
+
+int cmd_main(int argc, const char *argv[])
+{
+	TEST(t_tree(), "tree_search and infix_walk work");
+
+	return test_done();
+}
-- 
2.45.GIT
Junio C Hamano· Jul 22, 2024, 17:52 UTC · re: Chandra Pratap · lore

Re: [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework

Chandra Pratap <chandrapratap3519@gmail.com> writes:
Show 16 quoted lines
> reftable/tree_test.c exercises the functions defined in
> reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit
> testing framework. Migration involves refactoring the tests to use
> 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 <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
Looking good.

"git show -M30" matches the moved file up correctly and makes it easy to see that there is no serious changes snuck in (please do not take this as a suggestion to run format-patch with -M30---I am just saying that it was a good way to give an extra validation).

Thanks.
Junio C Hamano· Jul 22, 2024, 17:52 UTC · re: Chandra Pratap · lore

Re: [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework

Chandra Pratap <chandrapratap3519@gmail.com> writes:
> +	/* pseudo-randomly insert the pointers for elements between
> +	 * values[1] and values[10] (included) in the tree.
> +	 */
Style?
Junio C Hamano· Jul 22, 2024, 17:56 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
>> +	/* pseudo-randomly insert the pointers for elements between
>> +	 * values[1] and values[10] (included) in the tree.
>> +	 */
>
> Style?
Show changes to t/unit-tests/t-reftable-tree.c +3 −2
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index d7d530f2f7..e7d774d774 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -32,8 +32,9 @@ static void t_tree_search(void)
 	struct tree_node *nodes[11] = { 0 };
 	size_t i = 1;
 
-	/* pseudo-randomly insert the pointers for elements between
-	 * values[1] and values[10] (included) in the tree.
+	/*
+	 * Pseudo-randomly insert the pointers for elements between
+	 * values[1] and values[10] (inclusive) in the tree.
 	 */
 	do {
 		nodes[i] = tree_search(&values[i], &root, &t_compare, 1);
-- 
2.46.0-rc1-48-g0900f1888e
Chandra Pratap· Jul 22, 2024, 05:57 UTC · re: Chandra Pratap · lore

[PATCH v5 3/5] t-reftable-tree: split test_tree() into two sub-test functions

In the current testing setup, tests for both tree_search() and infix_walk() defined by reftable/tree.{c, h} are performed by a single test function, test_tree(). Split tree_test() into test_tree_search() and test_infix_walk() responsible for independently testing tree_search() and infix_walk() respectively. This improves the overall readability of the test file as well as simplifies debugging.

Note that the last parameter in the tree_search() functiom is '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 <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +19 −4
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 08f1873ad3..07c6c6dce5 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -26,13 +26,12 @@ static void check_increasing(void *arg, void *key)
 	c->last = key;
 }
 
-static void t_tree(void)
+static void t_tree_search(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
 	struct tree_node *nodes[11] = { 0 };
 	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.
@@ -47,13 +46,29 @@ 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 t_infix_walk(void)
+{
+	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct curry c = { 0 };
+	size_t i = 1;
+
+	do {
+		tree_search(&values[i], &root, t_compare, 1);
+		i = (i * 7) % 11;
+	} while (i != 1);
+
+	infix_walk(root, &check_increasing, &c);
 	tree_free(root);
 }
 
 int cmd_main(int argc, const char *argv[])
 {
-	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();
 }
-- 
2.45.GIT
Chandra Pratap· Jul 22, 2024, 05:57 UTC · re: Chandra Pratap · lore

[PATCH v5 4/5] t-reftable-tree: add test for non-existent key

In the current testing setup for tree_search(), the case for non-existent key is not exercised. Improve this by adding a test-case for the same.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/unit-tests/t-reftable-tree.c +1 −0
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 07c6c6dce5..6cd35b0ea0 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -46,6 +46,7 @@ static void t_tree_search(void)
 		check_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));
 	}
 
+	check(!tree_search(values, &root, t_compare, 0));
 	tree_free(root);
 }
 
-- 
2.45.GIT
Chandra Pratap· Jul 22, 2024, 05:57 UTC · re: Chandra Pratap · lore

[PATCH v5 5/5] t-reftable-tree: improve the test for infix_walk()

In the current testing setup for infix_walk(), the following
properties of an infix traversal of a tree remain untested:
- every node of the tree must be visited
- every node must be visited exactly once
In fact, only the property 'traversal in increasing order' is tested.
Modify test_infix_walk() to check for all the properties above.

This can be achieved by storing the nodes' keys linearly, in a nullified buffer, as we visit them and then checking the input keys against this buffer in increasing order. By checking that the element just after the last input key is 'NULL' in the output buffer, we ensure that every node is traversed exactly once.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +15 −7
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 6cd35b0ea0..d7d530f2f7 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -15,15 +15,14 @@ static int t_compare(const void *a, const void *b)
 }
 
 struct curry {
-	void *last;
+	void **arr;
+	size_t len;
 };
 
-static void check_increasing(void *arg, void *key)
+static void store(void *arg, void *key)
 {
 	struct curry *c = arg;
-	if (c->last)
-		check_int(t_compare(c->last, key), <, 0);
-	c->last = key;
+	c->arr[c->len++] = key;
 }
 
 static void t_tree_search(void)
@@ -54,15 +53,24 @@ static void t_infix_walk(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
-	struct curry c = { 0 };
+	void *out[11] = { 0 };
+	struct curry c = {
+		.arr = (void **) &out,
+	};
 	size_t i = 1;
+	size_t count = 0;
 
 	do {
 		tree_search(&values[i], &root, t_compare, 1);
 		i = (i * 7) % 11;
+		count++;
 	} while (i != 1);
 
-	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(!out[i - 1]);
+	check_int(c.len, ==, count);
 	tree_free(root);
 }
 
-- 
2.45.GIT
Patrick Steinhardt· Aug 1, 2024, 11:45 UTC · re: Chandra Pratap · lore

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

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
Chandra Pratap· Aug 2, 2024, 12:08 UTC · re: Chandra Pratap · lore

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

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 <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
Changes in v6:
- Fix a style issue in a comment introduced in patch 3
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 v5:
1:  59d5c17d5e ! 1:  501ea6c05d 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
      	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.
    +-	/* pseudo-randomly insert the pointers for elements between
    +-	 * values[1] and values[10] (included) in the tree.
    ++	/* Pseudo-randomly insert the pointers for elements between
    ++	 * values[1] and values[10] (inclusive) in the tree.
    + 	 */
    + 	do {
    + 		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));
      	}
2:  c1ce79916b = 2:  ecadc1833e t-reftable-tree: add test for non-existent key
3:  d1a5ced526 = 3:  cda7509281 t-reftable-tree: improve the test for infix_walk()
Chandra Pratap· Aug 2, 2024, 12:08 UTC · re: Chandra Pratap · lore

[PATCH v6 1/5] reftable: remove unnecessary curly braces in reftable/tree.c

According to Documentation/CodingGuidelines, single-line control-flow statements must omit curly braces (except for some special cases). Make reftable/tree.c adhere to this guideline.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 reftable/tree.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
Show changes to reftable/tree.c +5 −10
diff --git a/reftable/tree.c b/reftable/tree.c
index 528f33ae38..5ffb2e0d69 100644
--- a/reftable/tree.c
+++ b/reftable/tree.c
@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,
 void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),
 		void *arg)
 {
-	if (t->left) {
+	if (t->left)
 		infix_walk(t->left, action, arg);
-	}
 	action(arg, t->key);
-	if (t->right) {
+	if (t->right)
 		infix_walk(t->right, action, arg);
-	}
 }
 
 void tree_free(struct tree_node *t)
 {
-	if (!t) {
+	if (!t)
 		return;
-	}
-	if (t->left) {
+	if (t->left)
 		tree_free(t->left);
-	}
-	if (t->right) {
+	if (t->right)
 		tree_free(t->right);
-	}
 	reftable_free(t);
 }
-- 
2.45.GIT
Chandra Pratap· Aug 2, 2024, 12:08 UTC · re: Chandra Pratap · lore

[PATCH v6 2/5] t: move reftable/tree_test.c to the unit testing framework

reftable/tree_test.c exercises the functions defined in reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit testing framework. Migration involves refactoring the tests to use 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 <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                       |  2 +-
 reftable/reftable-tests.h      |  1 -
 reftable/tree_test.c           | 60 ----------------------------------
 t/helper/test-reftable.c       |  1 -
 t/unit-tests/t-reftable-tree.c | 59 +++++++++++++++++++++++++++++++++
 5 files changed, 60 insertions(+), 63 deletions(-)
 delete mode 100644 reftable/tree_test.c
 create mode 100644 t/unit-tests/t-reftable-tree.c
Show changes to 5 files +60 −63

Makefile, reftable/reftable-tests.h, reftable/tree_test.c, t/helper/test-reftable.c, t/unit-tests/t-reftable-tree.c

diff --git a/Makefile b/Makefile
index 3863e60b66..5499f7bcbd 100644
--- a/Makefile
+++ b/Makefile
@@ -1342,6 +1342,7 @@ UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
 UNIT_TEST_PROGRAMS += t-reftable-merged
 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
@@ -2685,7 +2686,6 @@ 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_TEST_OBJS += reftable/tree_test.o
 
 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index d5e03dcc1b..8516b1f923 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -15,7 +15,6 @@ int pq_test_main(int argc, const char **argv);
 int record_test_main(int argc, const char **argv);
 int readwrite_test_main(int argc, const char **argv);
 int stack_test_main(int argc, const char **argv);
-int tree_test_main(int argc, const char **argv);
 int reftable_dump_main(int argc, char *const *argv);
 
 #endif
diff --git a/reftable/tree_test.c b/reftable/tree_test.c
deleted file mode 100644
index 6961a657ad..0000000000
--- a/reftable/tree_test.c
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
-Copyright 2020 Google LLC
-
-Use of this source code is governed by a BSD-style
-license that can be found in the LICENSE file or at
-https://developers.google.com/open-source/licenses/bsd
-*/
-
-#include "system.h"
-#include "tree.h"
-
-#include "test_framework.h"
-#include "reftable-tests.h"
-
-static int test_compare(const void *a, const void *b)
-{
-	return (char *)a - (char *)b;
-}
-
-struct curry {
-	void *last;
-};
-
-static void check_increasing(void *arg, void *key)
-{
-	struct curry *c = arg;
-	if (c->last) {
-		EXPECT(test_compare(c->last, key) < 0);
-	}
-	c->last = key;
-}
-
-static void test_tree(void)
-{
-	struct tree_node *root = NULL;
-
-	void *values[11] = { NULL };
-	struct tree_node *nodes[11] = { NULL };
-	int i = 1;
-	struct curry c = { NULL };
-	do {
-		nodes[i] = tree_search(values + i, &root, &test_compare, 1);
-		i = (i * 7) % 11;
-	} while (i != 1);
-
-	for (i = 1; i < ARRAY_SIZE(nodes); i++) {
-		EXPECT(values + i == nodes[i]->key);
-		EXPECT(nodes[i] ==
-		       tree_search(values + i, &root, &test_compare, 0));
-	}
-
-	infix_walk(root, check_increasing, &c);
-	tree_free(root);
-}
-
-int tree_test_main(int argc, const char *argv[])
-{
-	RUN_TEST(test_tree);
-	return 0;
-}
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 9d378427da..0acaf85494 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -6,7 +6,6 @@ int cmd__reftable(int argc, const char **argv)
 {
 	/* test from simple to complex. */
 	block_test_main(argc, argv);
-	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
 	stack_test_main(argc, argv);
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
new file mode 100644
index 0000000000..08f1873ad3
--- /dev/null
+++ b/t/unit-tests/t-reftable-tree.c
@@ -0,0 +1,59 @@
+/*
+Copyright 2020 Google LLC
+
+Use of this source code is governed by a BSD-style
+license that can be found in the LICENSE file or at
+https://developers.google.com/open-source/licenses/bsd
+*/
+
+#include "test-lib.h"
+#include "reftable/tree.h"
+
+static int t_compare(const void *a, const void *b)
+{
+	return (char *)a - (char *)b;
+}
+
+struct curry {
+	void *last;
+};
+
+static void check_increasing(void *arg, void *key)
+{
+	struct curry *c = arg;
+	if (c->last)
+		check_int(t_compare(c->last, key), <, 0);
+	c->last = key;
+}
+
+static void t_tree(void)
+{
+	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct tree_node *nodes[11] = { 0 };
+	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);
+		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));
+	}
+
+	infix_walk(root, check_increasing, &c);
+	tree_free(root);
+}
+
+int cmd_main(int argc, const char *argv[])
+{
+	TEST(t_tree(), "tree_search and infix_walk work");
+
+	return test_done();
+}
-- 
2.45.GIT
Chandra Pratap· Aug 2, 2024, 12:08 UTC · re: Chandra Pratap · lore

[PATCH v6 3/5] t-reftable-tree: split test_tree() into two sub-test functions

In the current testing setup, tests for both tree_search() and infix_walk() defined by reftable/tree.{c, h} are performed by a single test function, test_tree(). Split tree_test() into test_tree_search() and test_infix_walk() responsible for independently testing tree_search() and infix_walk() respectively. This improves the overall readability of the test file as well as simplifies debugging.

Note that the last parameter in the tree_search() functiom is '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 <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +21 −6
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 08f1873ad3..5efe34835e 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -26,16 +26,15 @@ static void check_increasing(void *arg, void *key)
 	c->last = key;
 }
 
-static void t_tree(void)
+static void t_tree_search(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
 	struct tree_node *nodes[11] = { 0 };
 	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.
+	/* Pseudo-randomly insert the pointers for elements between
+	 * values[1] and values[10] (inclusive) in the tree.
 	 */
 	do {
 		nodes[i] = tree_search(&values[i], &root, &t_compare, 1);
@@ -47,13 +46,29 @@ 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 t_infix_walk(void)
+{
+	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct curry c = { 0 };
+	size_t i = 1;
+
+	do {
+		tree_search(&values[i], &root, t_compare, 1);
+		i = (i * 7) % 11;
+	} while (i != 1);
+
+	infix_walk(root, &check_increasing, &c);
 	tree_free(root);
 }
 
 int cmd_main(int argc, const char *argv[])
 {
-	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();
 }
-- 
2.45.GIT
Junio C Hamano· Aug 2, 2024, 16:25 UTC · re: Chandra Pratap · lore

Re: [PATCH v6 3/5] t-reftable-tree: split test_tree() into two sub-test functions

Chandra Pratap <chandrapratap3519@gmail.com> writes:
Show 5 quoted lines
> -	/* pseudo-randomly insert the pointers for elements between
> -	 * values[1] and values[10] (included) in the tree.
> +	/* Pseudo-randomly insert the pointers for elements between
> +	 * values[1] and values[10] (inclusive) in the tree.
>  	 */
If you are "fixing" the comment, let's fix its style as well.
	/*
         * Our multi-line comments begin with slash-asterisk, and
	 * end with asterisk-slash, on their own line.
	 */
Chandra Pratap· Aug 2, 2024, 12:08 UTC · re: Chandra Pratap · lore

[PATCH v6 4/5] t-reftable-tree: add test for non-existent key

In the current testing setup for tree_search(), the case for non-existent key is not exercised. Improve this by adding a test-case for the same.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/unit-tests/t-reftable-tree.c +1 −0
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 5efe34835e..0f00a31819 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -46,6 +46,7 @@ static void t_tree_search(void)
 		check_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));
 	}
 
+	check(!tree_search(values, &root, t_compare, 0));
 	tree_free(root);
 }
 
-- 
2.45.GIT
Chandra Pratap· Aug 2, 2024, 12:08 UTC · re: Chandra Pratap · lore

[PATCH v6 5/5] t-reftable-tree: improve the test for infix_walk()

In the current testing setup for infix_walk(), the following
properties of an infix traversal of a tree remain untested:
- every node of the tree must be visited
- every node must be visited exactly once
In fact, only the property 'traversal in increasing order' is tested.
Modify test_infix_walk() to check for all the properties above.

This can be achieved by storing the nodes' keys linearly, in a nullified buffer, as we visit them and then checking the input keys against this buffer in increasing order. By checking that the element just after the last input key is 'NULL' in the output buffer, we ensure that every node is traversed exactly once.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +15 −7
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 0f00a31819..2fc6a34008 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -15,15 +15,14 @@ static int t_compare(const void *a, const void *b)
 }
 
 struct curry {
-	void *last;
+	void **arr;
+	size_t len;
 };
 
-static void check_increasing(void *arg, void *key)
+static void store(void *arg, void *key)
 {
 	struct curry *c = arg;
-	if (c->last)
-		check_int(t_compare(c->last, key), <, 0);
-	c->last = key;
+	c->arr[c->len++] = key;
 }
 
 static void t_tree_search(void)
@@ -54,15 +53,24 @@ static void t_infix_walk(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
-	struct curry c = { 0 };
+	void *out[11] = { 0 };
+	struct curry c = {
+		.arr = (void **) &out,
+	};
 	size_t i = 1;
+	size_t count = 0;
 
 	do {
 		tree_search(&values[i], &root, t_compare, 1);
 		i = (i * 7) % 11;
+		count++;
 	} while (i != 1);
 
-	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(!out[i - 1]);
+	check_int(c.len, ==, count);
 	tree_free(root);
 }
 
-- 
2.45.GIT
Chandra Pratap· Aug 4, 2024, 14:06 UTC · re: Chandra Pratap · lore

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

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 <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
Changes in v7:
- Fix style issues in a comment introduced in patch 3
  of the previous series
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 v6:
1:  d4a45e602c = 1:  d738bf57e2 reftable: remove unnecessary curly braces in reftable/tree.c
2:  9148e740e8 ! 2:  f090ace685 t: move reftable/tree_test.c to the unit testing framework
    @@ 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.
    ++	/*
    ++	 * Pseudo-randomly insert the pointers for elements between
    ++	 * values[1] and values[10] (inclusive) in the tree.
     +	 */
     +	do {
     +		nodes[i] = tree_search(&values[i], &root, &t_compare, 1);
3:  f73ad11238 ! 3:  22256e77b3 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
      	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.
    -+	/* Pseudo-randomly insert the pointers for elements between
    -+	 * values[1] and values[10] (inclusive) in the tree.
    - 	 */
    - 	do {
    - 		nodes[i] = tree_search(&values[i], &root, &t_compare, 1);
    + 	/*
    + 	 * Pseudo-randomly insert the pointers for elements between
     @@ 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));
      	}
4:  edb02d2e84 = 4:  0d04daad28 t-reftable-tree: add test for non-existent key
5:  6aecd4e374 = 5:  80d4aa2a66 t-reftable-tree: improve the test for infix_walk()
Chandra Pratap· Aug 4, 2024, 14:06 UTC · re: Chandra Pratap · lore

[PATCH v7 1/5] reftable: remove unnecessary curly braces in reftable/tree.c

According to Documentation/CodingGuidelines, single-line control-flow statements must omit curly braces (except for some special cases). Make reftable/tree.c adhere to this guideline.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 reftable/tree.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
Show changes to reftable/tree.c +5 −10
diff --git a/reftable/tree.c b/reftable/tree.c
index 528f33ae38..5ffb2e0d69 100644
--- a/reftable/tree.c
+++ b/reftable/tree.c
@@ -39,25 +39,20 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,
 void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),
 		void *arg)
 {
-	if (t->left) {
+	if (t->left)
 		infix_walk(t->left, action, arg);
-	}
 	action(arg, t->key);
-	if (t->right) {
+	if (t->right)
 		infix_walk(t->right, action, arg);
-	}
 }
 
 void tree_free(struct tree_node *t)
 {
-	if (!t) {
+	if (!t)
 		return;
-	}
-	if (t->left) {
+	if (t->left)
 		tree_free(t->left);
-	}
-	if (t->right) {
+	if (t->right)
 		tree_free(t->right);
-	}
 	reftable_free(t);
 }
-- 
2.45.GIT
Chandra Pratap· Aug 4, 2024, 14:06 UTC · re: Chandra Pratap · lore

[PATCH v7 2/5] t: move reftable/tree_test.c to the unit testing framework

reftable/tree_test.c exercises the functions defined in reftable/tree.{c, h}. Migrate reftable/tree_test.c to the unit testing framework. Migration involves refactoring the tests to use 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 <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 Makefile                       |  2 +-
 reftable/reftable-tests.h      |  1 -
 reftable/tree_test.c           | 60 ----------------------------------
 t/helper/test-reftable.c       |  1 -
 t/unit-tests/t-reftable-tree.c | 60 ++++++++++++++++++++++++++++++++++
 5 files changed, 61 insertions(+), 63 deletions(-)
 delete mode 100644 reftable/tree_test.c
 create mode 100644 t/unit-tests/t-reftable-tree.c
Show changes to 5 files +61 −63

Makefile, reftable/reftable-tests.h, reftable/tree_test.c, t/helper/test-reftable.c, t/unit-tests/t-reftable-tree.c

diff --git a/Makefile b/Makefile
index 3863e60b66..5499f7bcbd 100644
--- a/Makefile
+++ b/Makefile
@@ -1342,6 +1342,7 @@ UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-reftable-basics
 UNIT_TEST_PROGRAMS += t-reftable-merged
 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
@@ -2685,7 +2686,6 @@ 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_TEST_OBJS += reftable/tree_test.o
 
 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
diff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h
index d5e03dcc1b..8516b1f923 100644
--- a/reftable/reftable-tests.h
+++ b/reftable/reftable-tests.h
@@ -15,7 +15,6 @@ int pq_test_main(int argc, const char **argv);
 int record_test_main(int argc, const char **argv);
 int readwrite_test_main(int argc, const char **argv);
 int stack_test_main(int argc, const char **argv);
-int tree_test_main(int argc, const char **argv);
 int reftable_dump_main(int argc, char *const *argv);
 
 #endif
diff --git a/reftable/tree_test.c b/reftable/tree_test.c
deleted file mode 100644
index 6961a657ad..0000000000
--- a/reftable/tree_test.c
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
-Copyright 2020 Google LLC
-
-Use of this source code is governed by a BSD-style
-license that can be found in the LICENSE file or at
-https://developers.google.com/open-source/licenses/bsd
-*/
-
-#include "system.h"
-#include "tree.h"
-
-#include "test_framework.h"
-#include "reftable-tests.h"
-
-static int test_compare(const void *a, const void *b)
-{
-	return (char *)a - (char *)b;
-}
-
-struct curry {
-	void *last;
-};
-
-static void check_increasing(void *arg, void *key)
-{
-	struct curry *c = arg;
-	if (c->last) {
-		EXPECT(test_compare(c->last, key) < 0);
-	}
-	c->last = key;
-}
-
-static void test_tree(void)
-{
-	struct tree_node *root = NULL;
-
-	void *values[11] = { NULL };
-	struct tree_node *nodes[11] = { NULL };
-	int i = 1;
-	struct curry c = { NULL };
-	do {
-		nodes[i] = tree_search(values + i, &root, &test_compare, 1);
-		i = (i * 7) % 11;
-	} while (i != 1);
-
-	for (i = 1; i < ARRAY_SIZE(nodes); i++) {
-		EXPECT(values + i == nodes[i]->key);
-		EXPECT(nodes[i] ==
-		       tree_search(values + i, &root, &test_compare, 0));
-	}
-
-	infix_walk(root, check_increasing, &c);
-	tree_free(root);
-}
-
-int tree_test_main(int argc, const char *argv[])
-{
-	RUN_TEST(test_tree);
-	return 0;
-}
diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
index 9d378427da..0acaf85494 100644
--- a/t/helper/test-reftable.c
+++ b/t/helper/test-reftable.c
@@ -6,7 +6,6 @@ int cmd__reftable(int argc, const char **argv)
 {
 	/* test from simple to complex. */
 	block_test_main(argc, argv);
-	tree_test_main(argc, argv);
 	pq_test_main(argc, argv);
 	readwrite_test_main(argc, argv);
 	stack_test_main(argc, argv);
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
new file mode 100644
index 0000000000..8b1f9a66a0
--- /dev/null
+++ b/t/unit-tests/t-reftable-tree.c
@@ -0,0 +1,60 @@
+/*
+Copyright 2020 Google LLC
+
+Use of this source code is governed by a BSD-style
+license that can be found in the LICENSE file or at
+https://developers.google.com/open-source/licenses/bsd
+*/
+
+#include "test-lib.h"
+#include "reftable/tree.h"
+
+static int t_compare(const void *a, const void *b)
+{
+	return (char *)a - (char *)b;
+}
+
+struct curry {
+	void *last;
+};
+
+static void check_increasing(void *arg, void *key)
+{
+	struct curry *c = arg;
+	if (c->last)
+		check_int(t_compare(c->last, key), <, 0);
+	c->last = key;
+}
+
+static void t_tree(void)
+{
+	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct tree_node *nodes[11] = { 0 };
+	size_t i = 1;
+	struct curry c = { 0 };
+
+	/*
+	 * Pseudo-randomly insert the pointers for elements between
+	 * values[1] and values[10] (inclusive) in the tree.
+	 */
+	do {
+		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));
+	}
+
+	infix_walk(root, check_increasing, &c);
+	tree_free(root);
+}
+
+int cmd_main(int argc, const char *argv[])
+{
+	TEST(t_tree(), "tree_search and infix_walk work");
+
+	return test_done();
+}
-- 
2.45.GIT
Chandra Pratap· Aug 4, 2024, 14:06 UTC · re: Chandra Pratap · lore

[PATCH v7 3/5] t-reftable-tree: split test_tree() into two sub-test functions

In the current testing setup, tests for both tree_search() and infix_walk() defined by reftable/tree.{c, h} are performed by a single test function, test_tree(). Split tree_test() into test_tree_search() and test_infix_walk() responsible for independently testing tree_search() and infix_walk() respectively. This improves the overall readability of the test file as well as simplifies debugging.

Note that the last parameter in the tree_search() functiom is '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 <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +19 −4
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 8b1f9a66a0..7cc52a1925 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -26,13 +26,12 @@ static void check_increasing(void *arg, void *key)
 	c->last = key;
 }
 
-static void t_tree(void)
+static void t_tree_search(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
 	struct tree_node *nodes[11] = { 0 };
 	size_t i = 1;
-	struct curry c = { 0 };
 
 	/*
 	 * Pseudo-randomly insert the pointers for elements between
@@ -48,13 +47,29 @@ 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 t_infix_walk(void)
+{
+	struct tree_node *root = NULL;
+	void *values[11] = { 0 };
+	struct curry c = { 0 };
+	size_t i = 1;
+
+	do {
+		tree_search(&values[i], &root, t_compare, 1);
+		i = (i * 7) % 11;
+	} while (i != 1);
+
+	infix_walk(root, &check_increasing, &c);
 	tree_free(root);
 }
 
 int cmd_main(int argc, const char *argv[])
 {
-	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();
 }
-- 
2.45.GIT
Chandra Pratap· Aug 4, 2024, 14:06 UTC · re: Chandra Pratap · lore

[PATCH v7 4/5] t-reftable-tree: add test for non-existent key

In the current testing setup for tree_search(), the case for non-existent key is not exercised. Improve this by adding a test-case for the same.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/unit-tests/t-reftable-tree.c +1 −0
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 7cc52a1925..2220414a18 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -47,6 +47,7 @@ static void t_tree_search(void)
 		check_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));
 	}
 
+	check(!tree_search(values, &root, t_compare, 0));
 	tree_free(root);
 }
 
-- 
2.45.GIT
Chandra Pratap· Aug 4, 2024, 14:06 UTC · re: Chandra Pratap · lore

[PATCH v7 5/5] t-reftable-tree: improve the test for infix_walk()

In the current testing setup for infix_walk(), the following
properties of an infix traversal of a tree remain untested:
- every node of the tree must be visited
- every node must be visited exactly once
In fact, only the property 'traversal in increasing order' is tested.
Modify test_infix_walk() to check for all the properties above.

This can be achieved by storing the nodes' keys linearly, in a nullified buffer, as we visit them and then checking the input keys against this buffer in increasing order. By checking that the element just after the last input key is 'NULL' in the output buffer, we ensure that every node is traversed exactly once.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
 t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
Show changes to t/unit-tests/t-reftable-tree.c +15 −7
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 2220414a18..e7d774d774 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -15,15 +15,14 @@ static int t_compare(const void *a, const void *b)
 }
 
 struct curry {
-	void *last;
+	void **arr;
+	size_t len;
 };
 
-static void check_increasing(void *arg, void *key)
+static void store(void *arg, void *key)
 {
 	struct curry *c = arg;
-	if (c->last)
-		check_int(t_compare(c->last, key), <, 0);
-	c->last = key;
+	c->arr[c->len++] = key;
 }
 
 static void t_tree_search(void)
@@ -55,15 +54,24 @@ static void t_infix_walk(void)
 {
 	struct tree_node *root = NULL;
 	void *values[11] = { 0 };
-	struct curry c = { 0 };
+	void *out[11] = { 0 };
+	struct curry c = {
+		.arr = (void **) &out,
+	};
 	size_t i = 1;
+	size_t count = 0;
 
 	do {
 		tree_search(&values[i], &root, t_compare, 1);
 		i = (i * 7) % 11;
+		count++;
 	} while (i != 1);
 
-	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(!out[i - 1]);
+	check_int(c.len, ==, count);
 	tree_free(root);
 }
 
-- 
2.45.GIT
Patrick Steinhardt· Aug 5, 2024, 11:04 UTC · re: Chandra Pratap · lore

Re: [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework

On Sun, Aug 04, 2024 at 07:36:44PM +0530, Chandra Pratap wrote:
Show 18 quoted lines
> 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 <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>

Only a single change compared to v6, addressing the only feedback on that version. So this looks good to me, thanks!

Patrick
Junio C Hamano· Aug 5, 2024, 15:53 UTC · re: Patrick Steinhardt · lore

Re: [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework

Patrick Steinhardt <ps@pks.im> writes:
Show 22 quoted lines
> On Sun, Aug 04, 2024 at 07:36:44PM +0530, Chandra Pratap wrote:
>> 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 <ps@pks.im>
>> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
> Only a single change compared to v6, addressing the only feedback on
> that version. So this looks good to me, thanks!

FWIW, I didn't have other feedback not because I found the rest perfect, but because I didn't read the series myself carefully, hoping others are sharing the burden.

Thanks.
Patrick Steinhardt· Aug 6, 2024, 06:30 UTC · re: Junio C Hamano · lore

Re: [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework

On Mon, Aug 05, 2024 at 08:53:09AM -0700, Junio C Hamano wrote:
Show 28 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > On Sun, Aug 04, 2024 at 07:36:44PM +0530, Chandra Pratap wrote:
> >> 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 <ps@pks.im>
> >> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> >> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> >
> > Only a single change compared to v6, addressing the only feedback on
> > that version. So this looks good to me, thanks!
> 
> FWIW, I didn't have other feedback not because I found the rest
> perfect, but because I didn't read the series myself carefully,
> hoping others are sharing the burden.

Oh, yes. I didn't mean to say that I relied on your feedback being addressed exclusively. I already reviewed v5/v6 of this patch series and found it to be good, and given that there was only a single change proposed by you that I'm happy with it translates into me being in favor of v7, as well.

Patrick
Junio C Hamano· Aug 6, 2024, 15:35 UTC · re: Patrick Steinhardt · lore

Re: [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework

Patrick Steinhardt <ps@pks.im> writes:
Show 5 quoted lines
> Oh, yes. I didn't mean to say that I relied on your feedback being
> addressed exclusively. I already reviewed v5/v6 of this patch series and
> found it to be good, and given that there was only a single change
> proposed by you that I'm happy with it translates into me being in favor
> of v7, as well.

Yes, after sending the message you are responding to, I went back to the original thread and figured that much---the topic is marked for 'next'.

Thanks for your help.

← back to recent threads