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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1437 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]