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

From: Patrick Steinhardt <ps@pks.im>
Date: Jun 12, 2024, 06:59 UTC
Message-ID: <ZmlHZXN8_7rKLTYk@tanuki>
In-Reply-To: <20240612055031.3607-6-chandrapratap3519@gmail.com>

On Wed, Jun 12, 2024 at 11:08:14AM +0530, Chandra Pratap wrote:

> In the current testing setup for infix_walk(), the following
> properties of an infix traversal of a tree remain untested:
> - every node of the tree must be visited
> - every node must be visited exactly only

s/only/once

> and only the property 'traversal in increasing order' is tested.

Nit: this reads a bit awkward. How about "In fact, we only verify that
the traversal happens in increasing order."

Show 5 quoted lines

> @@ -51,6 +50,7 @@ static void test_infix_walk(void)
>  {
>  	struct tree_node *root = NULL;
>  	void *values[11] = { 0 };
> +	void *out[20] = { 0 };

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

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

Show 9 quoted lines

>  	struct curry c = { 0 };
>  	size_t i = 1;
>  
> @@ -59,7 +59,11 @@ static void test_infix_walk(void)
>  		i = (i * 7) % 11;
>  	} while (i != 1);
>  
> -	infix_walk(root, &check_increasing, &c);
> +	c.arr = (void **) &out;

We can initialize this variable directly when declaring `c`:

    struct curry c = {
        .arr = &out;
    };

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

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

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

Patrick

> +	check(!out[i]);
>  	tree_free(root);
>  }