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

> 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?