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

[Karthik Nayak <karthik.188@gmail.com>]

[-- 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 --]