| Message ID | 20240610131017.8321-4-chandrapratap3519@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: port reftable/tree_test.c to the unit testing framework | expand |
On Mon, Jun 10, 2024 at 06:31:30PM +0530, Chandra Pratap wrote: > @@ -44,13 +44,29 @@ static void test_tree(void) > check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0)); > } > > - infix_walk(root, check_increasing, &c); > + tree_free(root); > +} > + > +static void test_infix_walk(void) > +{ > + struct tree_node *root = NULL; > + void *values[13] = { 0 }; Is there a reason why we have 13 values here while we had 11 values in the test this was split out from? > + struct curry c = { 0 }; > + size_t i = 1; > + > + do { > + tree_search(values + i, &root, &test_compare, 1); > + i = (i * 5) % 13; > + } while (i != 1); It's completely non-obvious that `tree_search()` ends up _inserting_ nodes into the tree when the entry we're searching for wasn't found (and if the last parameter is `1`. I feel like this interface could really use a complete makeover and split up its concerns. In any case, that does not need to happen as part of this patch seriesr What I think would help though is if the commit message itself mentioned this unorthodox way of inserting values into the tree. > + infix_walk(root, &check_increasing, &c); Not a fault of this commit, but this test certainly isn't great. It would succeed even if `infix_walk()` didn't do anything as we do not verify at all whether all nodes have been traversed (and traversed once, exactly). Patrick
On Mon, 10 Jun 2024 at 19:19, Patrick Steinhardt <ps@pks.im> wrote: > > On Mon, Jun 10, 2024 at 06:31:30PM +0530, Chandra Pratap wrote: > > @@ -44,13 +44,29 @@ static void test_tree(void) > > check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0)); > > } > > > > - infix_walk(root, check_increasing, &c); > > + tree_free(root); > > +} > > + > > +static void test_infix_walk(void) > > +{ > > + struct tree_node *root = NULL; > > + void *values[13] = { 0 }; > > Is there a reason why we have 13 values here while we had 11 values in > the test this was split out from? I did that to introduce some variety between the test cases, but now that you mention it, this change doesn't go well with the objective of this patch. > > + struct curry c = { 0 }; > > + size_t i = 1; > > + > > + do { > > + tree_search(values + i, &root, &test_compare, 1); > > + i = (i * 5) % 13; > > + } while (i != 1); > > It's completely non-obvious that `tree_search()` ends up _inserting_ > nodes into the tree when the entry we're searching for wasn't found (and > if the last parameter is `1`. I feel like this interface could really > use a complete makeover and split up its concerns. In any case, that > does not need to happen as part of this patch seriesr I don't really mind it because all tree operations are only used in reftable/writer.c and only in a couple of places, so it would make sense for the original authors to focus their efforts on other parts of the codebase. Still, I do agree that the readability takes a hit 'cause of that. > What I think would help though is if the commit message itself mentioned > this unorthodox way of inserting values into the tree. Sure thing. > > + infix_walk(root, &check_increasing, &c); > > Not a fault of this commit, but this test certainly isn't great. It > would succeed even if `infix_walk()` didn't do anything as we do not > verify at all whether all nodes have been traversed (and traversed once, > exactly). I'll try to modify the test to check for these properties of an infix walk as well.
diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c index 208e7b7874..78d5caafbe 100644 --- a/t/unit-tests/t-reftable-tree.c +++ b/t/unit-tests/t-reftable-tree.c @@ -26,7 +26,7 @@ static void check_increasing(void *arg, void *key) c->last = key; } -static void test_tree(void) +static void test_tree_search(void) { struct tree_node *root = NULL; void *values[11] = { 0 }; @@ -44,13 +44,29 @@ static void test_tree(void) check_pointer_eq(nodes[i], tree_search(values + i, &root, &test_compare, 0)); } - infix_walk(root, check_increasing, &c); + tree_free(root); +} + +static void test_infix_walk(void) +{ + struct tree_node *root = NULL; + void *values[13] = { 0 }; + struct curry c = { 0 }; + size_t i = 1; + + do { + tree_search(values + i, &root, &test_compare, 1); + i = (i * 5) % 13; + } while (i != 1); + + infix_walk(root, &check_increasing, &c); tree_free(root); } int cmd_main(int argc, const char *argv[]) { - TEST(test_tree(), "tree_search and infix_walk work"); + TEST(test_tree_search(), "tree_search works"); + TEST(test_infix_walk(), "infix_walk works"); return test_done(); }
In the current testing setup, tests for both tree_search() and infix_walk() defined by reftable/tree.{c, h} are performed by a single test function, test_tree(). Split tree_test() into test_tree_search() and test_infix_walk() responsible for independently testing tree_search() and infix_walk() respectively. This improves the overall readability of the test file as well as simplifies debugging. Mentored-by: Patrick Steinhardt <ps@pks.im> Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com> --- t/unit-tests/t-reftable-tree.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)