* [PATCH v4 1/5] reftable: remove unnecessary curly braces in reftable/tree.c
2024-07-16 7:48 ` [GSoC][PATCH v4 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
@ 2024-07-16 7:48 ` Chandra Pratap
2024-07-16 7:48 ` [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework Chandra Pratap
` (5 subsequent siblings)
6 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-07-16 7:48 UTC (permalink / raw)
To: git; +Cc: karthik188, chriscool
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(-)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-16 7:48 ` [GSoC][PATCH v4 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
2024-07-16 7:48 ` [PATCH v4 1/5] reftable: remove unnecessary curly braces in reftable/tree.c Chandra Pratap
@ 2024-07-16 7:48 ` Chandra Pratap
2024-07-17 11:49 ` Karthik Nayak
2024-07-17 12:39 ` Karthik Nayak
2024-07-16 7:48 ` [PATCH v4 3/5] t-reftable-tree: split test_tree() into two sub-test functions Chandra Pratap
` (4 subsequent siblings)
6 siblings, 2 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-07-16 7:48 UTC (permalink / raw)
To: git; +Cc: karthik188, chriscool
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
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-16 7:48 ` [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework Chandra Pratap
@ 2024-07-17 11:49 ` Karthik Nayak
2024-07-17 13:50 ` Chandra Pratap
2024-07-17 12:39 ` Karthik Nayak
1 sibling, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2024-07-17 11:49 UTC (permalink / raw)
To: Chandra Pratap, git; +Cc: karthik188, chriscool
[-- Attachment #1: Type: text/plain, Size: 5982 bytes --]
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.
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-17 11:49 ` Karthik Nayak
@ 2024-07-17 13:50 ` Chandra Pratap
2024-07-18 8:04 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Chandra Pratap @ 2024-07-17 13:50 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, karthik188, chriscool
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]----
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-17 13:50 ` Chandra Pratap
@ 2024-07-18 8:04 ` Karthik Nayak
0 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2024-07-18 8:04 UTC (permalink / raw)
To: Chandra Pratap; +Cc: git, karthik188, chriscool
[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]
Chandra Pratap <chandrapratap3519@gmail.com> writes:
> 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!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-16 7:48 ` [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework Chandra Pratap
2024-07-17 11:49 ` Karthik Nayak
@ 2024-07-17 12:39 ` Karthik Nayak
2024-07-17 14:30 ` Chandra Pratap
1 sibling, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2024-07-17 12:39 UTC (permalink / raw)
To: Chandra Pratap, git; +Cc: karthik188, chriscool
[-- Attachment #1: Type: text/plain, Size: 2435 bytes --]
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.
> +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?
> + 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?
> + } 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-17 12:39 ` Karthik Nayak
@ 2024-07-17 14:30 ` Chandra Pratap
2024-07-17 22:14 ` Justin Tobler
0 siblings, 1 reply; 66+ messages in thread
From: Chandra Pratap @ 2024-07-17 14:30 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, karthik188, chriscool
On Wed, 17 Jul 2024 at 18:09, 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.
> >
>
> 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.
> > +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.
> > + 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.
> > + } 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
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-17 14:30 ` Chandra Pratap
@ 2024-07-17 22:14 ` Justin Tobler
2024-07-18 4:58 ` Chandra Pratap
0 siblings, 1 reply; 66+ messages in thread
From: Justin Tobler @ 2024-07-17 22:14 UTC (permalink / raw)
To: Chandra Pratap; +Cc: Karthik Nayak, git, karthik188, chriscool
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.
> > > + 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
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-17 22:14 ` Justin Tobler
@ 2024-07-18 4:58 ` Chandra Pratap
2024-07-18 8:10 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Chandra Pratap @ 2024-07-18 4:58 UTC (permalink / raw)
To: Justin Tobler; +Cc: Karthik Nayak, git, karthik188, chriscool
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.
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-18 4:58 ` Chandra Pratap
@ 2024-07-18 8:10 ` Karthik Nayak
2024-07-18 8:23 ` Chandra Pratap
2024-07-18 15:26 ` Justin Tobler
0 siblings, 2 replies; 66+ messages in thread
From: Karthik Nayak @ 2024-07-18 8:10 UTC (permalink / raw)
To: Chandra Pratap, Justin Tobler; +Cc: git, karthik188, chriscool
[-- Attachment #1: Type: text/plain, Size: 4494 bytes --]
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?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-18 8:10 ` Karthik Nayak
@ 2024-07-18 8:23 ` Chandra Pratap
2024-07-18 15:26 ` Justin Tobler
1 sibling, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-07-18 8:23 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Justin Tobler, git, karthik188, chriscool
On Thu, 18 Jul 2024 at 13:40, Karthik Nayak <karthik.188@gmail.com> wrote:
>
> 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'
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-18 8:10 ` Karthik Nayak
2024-07-18 8:23 ` Chandra Pratap
@ 2024-07-18 15:26 ` Justin Tobler
1 sibling, 0 replies; 66+ messages in thread
From: Justin Tobler @ 2024-07-18 15:26 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Chandra Pratap, git, karthik188, chriscool
On 24/07/18 01:10AM, Karthik Nayak wrote:
> 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.
^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v4 3/5] t-reftable-tree: split test_tree() into two sub-test functions
2024-07-16 7:48 ` [GSoC][PATCH v4 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
2024-07-16 7:48 ` [PATCH v4 1/5] reftable: remove unnecessary curly braces in reftable/tree.c Chandra Pratap
2024-07-16 7:48 ` [PATCH v4 2/5] t: move reftable/tree_test.c to the unit testing framework Chandra Pratap
@ 2024-07-16 7:48 ` Chandra Pratap
2024-07-16 7:48 ` [PATCH v4 4/5] t-reftable-tree: add test for non-existent key Chandra Pratap
` (3 subsequent siblings)
6 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-07-16 7:48 UTC (permalink / raw)
To: git; +Cc: karthik188, chriscool
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(-)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v4 4/5] t-reftable-tree: add test for non-existent key
2024-07-16 7:48 ` [GSoC][PATCH v4 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
` (2 preceding siblings ...)
2024-07-16 7:48 ` [PATCH v4 3/5] t-reftable-tree: split test_tree() into two sub-test functions Chandra Pratap
@ 2024-07-16 7:48 ` Chandra Pratap
2024-07-16 7:48 ` [PATCH v4 5/5] t-reftable-tree: improve the test for infix_walk() Chandra Pratap
` (2 subsequent siblings)
6 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-07-16 7:48 UTC (permalink / raw)
To: git; +Cc: karthik188, chriscool
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(+)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v4 5/5] t-reftable-tree: improve the test for infix_walk()
2024-07-16 7:48 ` [GSoC][PATCH v4 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
` (3 preceding siblings ...)
2024-07-16 7:48 ` [PATCH v4 4/5] t-reftable-tree: add test for non-existent key Chandra Pratap
@ 2024-07-16 7:48 ` Chandra Pratap
2024-07-16 19:52 ` [GSoC][PATCH v4 0/5] t: port reftable/tree_test.c to the unit testing framework Junio C Hamano
2024-07-22 5:57 ` [GSoC][PATCH v5 " Chandra Pratap
6 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-07-16 7:48 UTC (permalink / raw)
To: git; +Cc: karthik188, chriscool
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(-)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [GSoC][PATCH v4 0/5] t: port reftable/tree_test.c to the unit testing framework
2024-07-16 7:48 ` [GSoC][PATCH v4 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
` (4 preceding siblings ...)
2024-07-16 7:48 ` [PATCH v4 5/5] t-reftable-tree: improve the test for infix_walk() Chandra Pratap
@ 2024-07-16 19:52 ` Junio C Hamano
2024-07-22 5:57 ` [GSoC][PATCH v5 " Chandra Pratap
6 siblings, 0 replies; 66+ messages in thread
From: Junio C Hamano @ 2024-07-16 19:52 UTC (permalink / raw)
To: Chandra Pratap; +Cc: git, karthik188, chriscool
Chandra Pratap <chandrapratap3519@gmail.com> writes:
> In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
Consistently refer to an existing commit with --format=reference.
$ git show -s --format=reference 8bf6fbd
8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)
> testing framework written entirely in C was introduced to the Git project
> aimed at simplifying testing and reducing test run times.
I doubt that the reason why "unit-tests" written entirely in C was
introduced is because we wanted to simplify testing and to reduce
test run times to begin with. The traditional tests to observe the
effect visible to end-users by actually running commands that would
be run by end-users and unit tests serve two separate purposes. The
latter does not "simplify", or "reduce time"---you cannot write a
test "entirely in C" to make sure, say, that "git push --force"
allows a non-fast-forward update to happen using unit-test
framework. The unit-tests cannot replace end-to-end tests. They
are complementary.
The statement may need to be rethought.
Or just stop at saying something like
The reftable library comes with self tests, which are exercised
as part of the usual end-to-end tests that are designed to
observe the end-user visible effects of Git commands. What it
exercises, however, is a better match to the unit-testing
framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',
2023-12-09), that are designed to observe how low level
implementation details, at the level of sequences of individual
function calls, behave.
which already covers the next paragraph while at it.
> Currently, tests for the reftable refs-backend are performed by a custom
> testing framework defined by reftable/test_framework.{c, h}. Port
> reftable/tree_test.c to the unit testing framework and improve upon
> the ported test.
>
> The first patch in the series is preparatory cleanup, the second patch
> moves the test to the unit testing framework, and the rest of the patches
> improve upon the ported test.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
> ---
> Changes in v4:
> - Rename the tests to be in-line with unit-tests' standards
>
> CI/PR: https://github.com/gitgitgadget/git/pull/1740
By the way, did you rebase the patches? On which commit is this
iteration based? Judging from the second patch, it seems to assume
that Makefile does not yet have t-reftable-record in it, and it
applies cleanly to 'master' before 9118e46e (Merge branch
'cp/unit-test-reftable-record', 2024-07-15). Newer 'master' has
textual conflicts (nothing that I cannot resolve, but it shows that
apparently that anything newer than 9118e46e are not commits that
you developed this series on).
Has this series even been compile-tested? I do not think that even
the unit test added by this series has been run (or compiled).
$ make -j32 t/unit-tests/t-reftable-tree.o
GIT_VERSION = 2.46.0.rc0.27.g0c2075a7c5
* new build flags
CC t/unit-tests/t-reftable-tree.o
In file included from t/unit-tests/t-reftable-tree.c:9:
t/unit-tests/t-reftable-tree.c: In function ‘t_tree_search’:
t/unit-tests/t-reftable-tree.c:45:44: error: ‘test_compare’ undeclared (first use in this function); did you mean ‘t_compare’?
45 | check(!tree_search(values, &root, &test_compare, 0));
| ^~~~~~~~~~~~
t/unit-tests/test-lib.h:75:45: note: in definition of macro ‘check’
75 | check_bool_loc(TEST_LOCATION(), #x, x)
| ^
t/unit-tests/t-reftable-tree.c:45:44: note: each undeclared identifier is reported only once for each function it appears in
45 | check(!tree_search(values, &root, &test_compare, 0));
| ^~~~~~~~~~~~
t/unit-tests/test-lib.h:75:45: note: in definition of macro ‘check’
75 | check_bool_loc(TEST_LOCATION(), #x, x)
| ^
make: *** [Makefile:2754: t/unit-tests/t-reftable-tree.o] Error 1
Let's concentrate on quality, not quantity; too many topics with the
same prefix cp/unit-test* seem to be left unreviewed on the list.
In the meantime, I'll queue the following fix-up on top. In this
codebase, it is preferred to write a pointer to a function whose
name is "func" as just "func", not "&func".
t/unit-tests/t-reftable-tree.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index b3d4008e5c..107f1f69bf 100644
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -33,16 +33,16 @@ static void t_tree_search(void)
size_t i = 1;
do {
- nodes[i] = tree_search(values + i, &root, &t_compare, 1);
+ nodes[i] = tree_search(values + i, &root, t_compare, 1);
i = (i * 7) % 11;
} while (i != 1);
for (i = 1; i < ARRAY_SIZE(nodes); i++) {
check_pointer_eq(values + i, nodes[i]->key);
- check_pointer_eq(nodes[i], tree_search(values + i, &root, &t_compare, 0));
+ check_pointer_eq(nodes[i], tree_search(values + i, &root, t_compare, 0));
}
- check(!tree_search(values, &root, &test_compare, 0));
+ check(!tree_search(values, &root, t_compare, 0));
tree_free(root);
}
@@ -58,7 +58,7 @@ static void t_infix_walk(void)
size_t count = 0;
do {
- tree_search(values + i, &root, &t_compare, 1);
+ tree_search(values + i, &root, t_compare, 1);
i = (i * 7) % 11;
count++;
} while (i != 1);
--
2.46.0-rc0-137-g851401d64b
^ permalink raw reply related [flat|nested] 66+ messages in thread* [GSoC][PATCH v5 0/5] t: port reftable/tree_test.c to the unit testing framework
2024-07-16 7:48 ` [GSoC][PATCH v4 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
` (5 preceding siblings ...)
2024-07-16 19:52 ` [GSoC][PATCH v4 0/5] t: port reftable/tree_test.c to the unit testing framework Junio C Hamano
@ 2024-07-22 5:57 ` Chandra Pratap
2024-07-22 5:57 ` [PATCH v5 1/5] reftable: remove unnecessary curly braces in reftable/tree.c Chandra Pratap
` (6 more replies)
6 siblings, 7 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-07-22 5:57 UTC (permalink / raw)
To: git; +Cc: karthik188, chriscool
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);
^ permalink raw reply [flat|nested] 66+ messages in thread* [PATCH v5 1/5] reftable: remove unnecessary curly braces in reftable/tree.c
2024-07-22 5:57 ` [GSoC][PATCH v5 " Chandra Pratap
@ 2024-07-22 5:57 ` Chandra Pratap
2024-07-22 5:57 ` [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework Chandra Pratap
` (5 subsequent siblings)
6 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-07-22 5:57 UTC (permalink / raw)
To: git; +Cc: karthik188, chriscool
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(-)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-22 5:57 ` [GSoC][PATCH v5 " Chandra Pratap
2024-07-22 5:57 ` [PATCH v5 1/5] reftable: remove unnecessary curly braces in reftable/tree.c Chandra Pratap
@ 2024-07-22 5:57 ` Chandra Pratap
2024-07-22 17:52 ` Junio C Hamano
2024-07-22 17:52 ` Junio C Hamano
2024-07-22 5:57 ` [PATCH v5 3/5] t-reftable-tree: split test_tree() into two sub-test functions Chandra Pratap
` (4 subsequent siblings)
6 siblings, 2 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-07-22 5:57 UTC (permalink / raw)
To: git; +Cc: karthik188, chriscool
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
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-22 5:57 ` [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework Chandra Pratap
@ 2024-07-22 17:52 ` Junio C Hamano
2024-07-22 17:52 ` Junio C Hamano
1 sibling, 0 replies; 66+ messages in thread
From: Junio C Hamano @ 2024-07-22 17:52 UTC (permalink / raw)
To: Chandra Pratap; +Cc: git, karthik188, chriscool
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.
>
> 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.
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-22 5:57 ` [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework Chandra Pratap
2024-07-22 17:52 ` Junio C Hamano
@ 2024-07-22 17:52 ` Junio C Hamano
2024-07-22 17:56 ` Junio C Hamano
1 sibling, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2024-07-22 17:52 UTC (permalink / raw)
To: Chandra Pratap; +Cc: git, karthik188, chriscool
Chandra Pratap <chandrapratap3519@gmail.com> writes:
> + /* pseudo-randomly insert the pointers for elements between
> + * values[1] and values[10] (included) in the tree.
> + */
Style?
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-07-22 17:52 ` Junio C Hamano
@ 2024-07-22 17:56 ` Junio C Hamano
0 siblings, 0 replies; 66+ messages in thread
From: Junio C Hamano @ 2024-07-22 17:56 UTC (permalink / raw)
To: Chandra Pratap; +Cc: git, karthik188, chriscool
Junio C Hamano <gitster@pobox.com> writes:
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
>> + /* pseudo-randomly insert the pointers for elements between
>> + * values[1] and values[10] (included) in the tree.
>> + */
>
> Style?
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
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v5 3/5] t-reftable-tree: split test_tree() into two sub-test functions
2024-07-22 5:57 ` [GSoC][PATCH v5 " Chandra Pratap
2024-07-22 5:57 ` [PATCH v5 1/5] reftable: remove unnecessary curly braces in reftable/tree.c Chandra Pratap
2024-07-22 5:57 ` [PATCH v5 2/5] t: move reftable/tree_test.c to the unit testing framework Chandra Pratap
@ 2024-07-22 5:57 ` Chandra Pratap
2024-07-22 5:57 ` [PATCH v5 4/5] t-reftable-tree: add test for non-existent key Chandra Pratap
` (3 subsequent siblings)
6 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-07-22 5:57 UTC (permalink / raw)
To: git; +Cc: karthik188, chriscool
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(-)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v5 4/5] t-reftable-tree: add test for non-existent key
2024-07-22 5:57 ` [GSoC][PATCH v5 " Chandra Pratap
` (2 preceding siblings ...)
2024-07-22 5:57 ` [PATCH v5 3/5] t-reftable-tree: split test_tree() into two sub-test functions Chandra Pratap
@ 2024-07-22 5:57 ` Chandra Pratap
2024-07-22 5:57 ` [PATCH v5 5/5] t-reftable-tree: improve the test for infix_walk() Chandra Pratap
` (2 subsequent siblings)
6 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-07-22 5:57 UTC (permalink / raw)
To: git; +Cc: karthik188, chriscool
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(+)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v5 5/5] t-reftable-tree: improve the test for infix_walk()
2024-07-22 5:57 ` [GSoC][PATCH v5 " Chandra Pratap
` (3 preceding siblings ...)
2024-07-22 5:57 ` [PATCH v5 4/5] t-reftable-tree: add test for non-existent key Chandra Pratap
@ 2024-07-22 5:57 ` Chandra Pratap
2024-08-01 11:21 ` [GSoC][PATCH v5 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
2024-08-02 12:08 ` [GSoC][PATCH v6 " Chandra Pratap
6 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-07-22 5:57 UTC (permalink / raw)
To: git; +Cc: karthik188, chriscool
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(-)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [GSoC][PATCH v5 0/5] t: port reftable/tree_test.c to the unit testing framework
2024-07-22 5:57 ` [GSoC][PATCH v5 " Chandra Pratap
` (4 preceding siblings ...)
2024-07-22 5:57 ` [PATCH v5 5/5] t-reftable-tree: improve the test for infix_walk() Chandra Pratap
@ 2024-08-01 11:21 ` Chandra Pratap
2024-08-01 11:45 ` Patrick Steinhardt
2024-08-02 12:08 ` [GSoC][PATCH v6 " Chandra Pratap
6 siblings, 1 reply; 66+ messages in thread
From: Chandra Pratap @ 2024-08-01 11:21 UTC (permalink / raw)
To: git, Patrick Steinhardt; +Cc: karthik188, chriscool
A reminder for reviews/acks.
^ permalink raw reply [flat|nested] 66+ messages in thread* [GSoC][PATCH v6 0/5] t: port reftable/tree_test.c to the unit testing framework
2024-07-22 5:57 ` [GSoC][PATCH v5 " Chandra Pratap
` (5 preceding siblings ...)
2024-08-01 11:21 ` [GSoC][PATCH v5 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
@ 2024-08-02 12:08 ` Chandra Pratap
2024-08-02 12:08 ` [PATCH v6 1/5] reftable: remove unnecessary curly braces in reftable/tree.c Chandra Pratap
` (5 more replies)
6 siblings, 6 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-08-02 12:08 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Christian Couder, Chandra Pratap
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()
^ permalink raw reply [flat|nested] 66+ messages in thread* [PATCH v6 1/5] reftable: remove unnecessary curly braces in reftable/tree.c
2024-08-02 12:08 ` [GSoC][PATCH v6 " Chandra Pratap
@ 2024-08-02 12:08 ` Chandra Pratap
2024-08-02 12:08 ` [PATCH v6 2/5] t: move reftable/tree_test.c to the unit testing framework Chandra Pratap
` (4 subsequent siblings)
5 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-08-02 12:08 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Patrick Steinhardt, Christian Couder
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(-)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v6 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-08-02 12:08 ` [GSoC][PATCH v6 " Chandra Pratap
2024-08-02 12:08 ` [PATCH v6 1/5] reftable: remove unnecessary curly braces in reftable/tree.c Chandra Pratap
@ 2024-08-02 12:08 ` Chandra Pratap
2024-08-02 12:08 ` [PATCH v6 3/5] t-reftable-tree: split test_tree() into two sub-test functions Chandra Pratap
` (3 subsequent siblings)
5 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-08-02 12:08 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Patrick Steinhardt, Christian Couder
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
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v6 3/5] t-reftable-tree: split test_tree() into two sub-test functions
2024-08-02 12:08 ` [GSoC][PATCH v6 " Chandra Pratap
2024-08-02 12:08 ` [PATCH v6 1/5] reftable: remove unnecessary curly braces in reftable/tree.c Chandra Pratap
2024-08-02 12:08 ` [PATCH v6 2/5] t: move reftable/tree_test.c to the unit testing framework Chandra Pratap
@ 2024-08-02 12:08 ` Chandra Pratap
2024-08-02 16:25 ` Junio C Hamano
2024-08-02 12:08 ` [PATCH v6 4/5] t-reftable-tree: add test for non-existent key Chandra Pratap
` (2 subsequent siblings)
5 siblings, 1 reply; 66+ messages in thread
From: Chandra Pratap @ 2024-08-02 12:08 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Patrick Steinhardt, Christian Couder
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(-)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v6 4/5] t-reftable-tree: add test for non-existent key
2024-08-02 12:08 ` [GSoC][PATCH v6 " Chandra Pratap
` (2 preceding siblings ...)
2024-08-02 12:08 ` [PATCH v6 3/5] t-reftable-tree: split test_tree() into two sub-test functions Chandra Pratap
@ 2024-08-02 12:08 ` Chandra Pratap
2024-08-02 12:08 ` [PATCH v6 5/5] t-reftable-tree: improve the test for infix_walk() Chandra Pratap
2024-08-04 14:06 ` [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
5 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-08-02 12:08 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Patrick Steinhardt, Christian Couder
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(+)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v6 5/5] t-reftable-tree: improve the test for infix_walk()
2024-08-02 12:08 ` [GSoC][PATCH v6 " Chandra Pratap
` (3 preceding siblings ...)
2024-08-02 12:08 ` [PATCH v6 4/5] t-reftable-tree: add test for non-existent key Chandra Pratap
@ 2024-08-02 12:08 ` Chandra Pratap
2024-08-04 14:06 ` [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
5 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-08-02 12:08 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Patrick Steinhardt, Christian Couder
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(-)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework
2024-08-02 12:08 ` [GSoC][PATCH v6 " Chandra Pratap
` (4 preceding siblings ...)
2024-08-02 12:08 ` [PATCH v6 5/5] t-reftable-tree: improve the test for infix_walk() Chandra Pratap
@ 2024-08-04 14:06 ` Chandra Pratap
2024-08-04 14:06 ` [PATCH v7 1/5] reftable: remove unnecessary curly braces in reftable/tree.c Chandra Pratap
` (5 more replies)
5 siblings, 6 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-08-04 14:06 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Christian Couder, Chandra Pratap
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()
^ permalink raw reply [flat|nested] 66+ messages in thread* [PATCH v7 1/5] reftable: remove unnecessary curly braces in reftable/tree.c
2024-08-04 14:06 ` [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
@ 2024-08-04 14:06 ` Chandra Pratap
2024-08-04 14:06 ` [PATCH v7 2/5] t: move reftable/tree_test.c to the unit testing framework Chandra Pratap
` (4 subsequent siblings)
5 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-08-04 14:06 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Patrick Steinhardt, Christian Couder
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(-)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v7 2/5] t: move reftable/tree_test.c to the unit testing framework
2024-08-04 14:06 ` [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
2024-08-04 14:06 ` [PATCH v7 1/5] reftable: remove unnecessary curly braces in reftable/tree.c Chandra Pratap
@ 2024-08-04 14:06 ` Chandra Pratap
2024-08-04 14:06 ` [PATCH v7 3/5] t-reftable-tree: split test_tree() into two sub-test functions Chandra Pratap
` (3 subsequent siblings)
5 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-08-04 14:06 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Patrick Steinhardt, Christian Couder
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
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v7 3/5] t-reftable-tree: split test_tree() into two sub-test functions
2024-08-04 14:06 ` [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
2024-08-04 14:06 ` [PATCH v7 1/5] reftable: remove unnecessary curly braces in reftable/tree.c Chandra Pratap
2024-08-04 14:06 ` [PATCH v7 2/5] t: move reftable/tree_test.c to the unit testing framework Chandra Pratap
@ 2024-08-04 14:06 ` Chandra Pratap
2024-08-04 14:06 ` [PATCH v7 4/5] t-reftable-tree: add test for non-existent key Chandra Pratap
` (2 subsequent siblings)
5 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-08-04 14:06 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Patrick Steinhardt, Christian Couder
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(-)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v7 4/5] t-reftable-tree: add test for non-existent key
2024-08-04 14:06 ` [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
` (2 preceding siblings ...)
2024-08-04 14:06 ` [PATCH v7 3/5] t-reftable-tree: split test_tree() into two sub-test functions Chandra Pratap
@ 2024-08-04 14:06 ` Chandra Pratap
2024-08-04 14:06 ` [PATCH v7 5/5] t-reftable-tree: improve the test for infix_walk() Chandra Pratap
2024-08-05 11:04 ` [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework Patrick Steinhardt
5 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-08-04 14:06 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Patrick Steinhardt, Christian Couder
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(+)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v7 5/5] t-reftable-tree: improve the test for infix_walk()
2024-08-04 14:06 ` [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
` (3 preceding siblings ...)
2024-08-04 14:06 ` [PATCH v7 4/5] t-reftable-tree: add test for non-existent key Chandra Pratap
@ 2024-08-04 14:06 ` Chandra Pratap
2024-08-05 11:04 ` [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework Patrick Steinhardt
5 siblings, 0 replies; 66+ messages in thread
From: Chandra Pratap @ 2024-08-04 14:06 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Patrick Steinhardt, Christian Couder
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(-)
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
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework
2024-08-04 14:06 ` [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework Chandra Pratap
` (4 preceding siblings ...)
2024-08-04 14:06 ` [PATCH v7 5/5] t-reftable-tree: improve the test for infix_walk() Chandra Pratap
@ 2024-08-05 11:04 ` Patrick Steinhardt
2024-08-05 15:53 ` Junio C Hamano
5 siblings, 1 reply; 66+ messages in thread
From: Patrick Steinhardt @ 2024-08-05 11:04 UTC (permalink / raw)
To: Chandra Pratap; +Cc: git, Christian Couder
[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]
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!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework
2024-08-05 11:04 ` [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework Patrick Steinhardt
@ 2024-08-05 15:53 ` Junio C Hamano
2024-08-06 6:30 ` Patrick Steinhardt
0 siblings, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2024-08-05 15:53 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Chandra Pratap, git, Christian Couder
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.
Thanks.
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework
2024-08-05 15:53 ` Junio C Hamano
@ 2024-08-06 6:30 ` Patrick Steinhardt
2024-08-06 15:35 ` Junio C Hamano
0 siblings, 1 reply; 66+ messages in thread
From: Patrick Steinhardt @ 2024-08-06 6:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Chandra Pratap, git, Christian Couder
[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]
On Mon, Aug 05, 2024 at 08:53:09AM -0700, Junio C Hamano wrote:
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [GSoC][PATCH v7 0/5] t: port reftable/tree_test.c to the unit testing framework
2024-08-06 6:30 ` Patrick Steinhardt
@ 2024-08-06 15:35 ` Junio C Hamano
0 siblings, 0 replies; 66+ messages in thread
From: Junio C Hamano @ 2024-08-06 15:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Chandra Pratap, git, Christian Couder
Patrick Steinhardt <ps@pks.im> writes:
> 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.
^ permalink raw reply [flat|nested] 66+ messages in thread