Re: [GSoC][PATCH] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 6, 2024, 21:54 UTC
- Message-ID
- <xmqqo78dka99.fsf@gitster.g>
- In-Reply-To
- <20240605134400.37309-1-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
Show 11 quoted lines
> helper/test-oidtree.c along with t0069-oidtree.sh test the oidtree.h > library, which is a wrapper around crit-bit tree. Migrate them to > the unit testing framework for better debugging and runtime > performance. > > To achieve this, introduce a new library called 'lib-oid.h' > exclusively for the unit tests to use. It currently mainly includes > utility to generate object_id from an arbitrary hex string > (i.e. '12a' -> '12a0000000000000000000000000000000000000'). > This will also be helpful when we port other unit tests such > as oid-array, oidset etc.
Perhaps. With only a single user it is hard to judge if it is worth doing, but once the code is written, it is not worth a code churn to merge it into t-oidtree.c.
Show 6 quoted lines
> +#define FILL_TREE(tree, ...) \
> + do { \
> + const char *hexes[] = { __VA_ARGS__ }; \
> + if (fill_tree_loc(tree, hexes, ARRAY_SIZE(hexes))) \
> + return; \
> + } while (0)Nice.
Show 12 quoted lines
> +static enum cb_next check_each_cb(const struct object_id *oid, void *data)
> +{
> + const char *hex = data;
> + struct object_id expected;
> +
> + if (!check_int(get_oid_arbitrary_hex(hex, &expected), ==, 0))
> + return CB_CONTINUE;
> + if (!check(oideq(oid, &expected)))
> + test_msg("expected: %s\n got: %s",
> + hash_to_hex(expected.hash), hash_to_hex(oid->hash));
> + return CB_CONTINUE;
> +}The control flow looks somewhat strange here. I would have written:
if (!check_int(..., ==, 0)) ; /* the data is bogus and cannot be used */ else if (!check(oideq(...)) test_msg(... expected and got differ ...); return CB_CONTINUE;
but OK.
Show 28 quoted lines
> +static void check_each(struct oidtree *ot, char *hex, char *expected)
> +{
> + struct object_id oid;
> +
> + if (!check_int(get_oid_arbitrary_hex(hex, &oid), ==, 0))
> + return;
> + oidtree_each(ot, &oid, 40, check_each_cb, expected);
> +}
> +
> +static void setup(void (*f)(struct oidtree *ot))
> +{
> + struct oidtree ot;
> +
> + oidtree_init(&ot);
> + f(&ot);
> + oidtree_clear(&ot);
> +}
> +
> +static void t_contains(struct oidtree *ot)
> +{
> + FILL_TREE(ot, "444", "1", "2", "3", "4", "5", "a", "b", "c", "d", "e");
> + check_contains(ot, "44", 0);
> + check_contains(ot, "441", 0);
> + check_contains(ot, "440", 0);
> + check_contains(ot, "444", 1);
> + check_contains(ot, "4440", 1);
> + check_contains(ot, "4444", 0);
> +}OK.
Compared to the original, this makes the correspondence between the input and the expected result slightly easier to see, which is good.
Show 8 quoted lines
> +static void t_each(struct oidtree *ot)
> +{
> + FILL_TREE(ot, "f", "9", "8", "123", "321", "a", "b", "c", "d", "e");
> + check_each(ot, "12300", "123");
> + check_each(ot, "3211", ""); /* should not reach callback */
> + check_each(ot, "3210", "321");
> + check_each(ot, "32100", "321");
> +}Testing "each" with test data that yields only at most one response smells iffy. It is a problem in the original test, and not a problem with the conversion, ...
BUT
... in the original, it is easy to do something like the attached to demonstrate that "each" can yield all oid that the shares the query prefix. But the rewritten unit test bakes the assumption that we will only try a query that yields at most one response into the test helper functions. Shouldn't we do a bit better, perhaps allowing the check_each() helper to take variable number of parameters, e.g.
check_each(ot, "12300", "123", NULL); check_each(ot, "32", "320", "321", NULL);
so the latter invocation asks "ot" trie "I have prefix 32, please call me back with each element you have that match", and makes sure that we get called back with "320" and then "321" and never after.
Come to think of it, how is your check_each_cb() ensuring that it is only called once with "123" when queried with "12300"? If the callback is made with "123" 100 times with the single query with "12300", would it even notice? I would imagine that the original would (simply because it dumps each and every callback to a file to be compared with the golden copy).
t/t0069-oidtree.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git c/t/t0069-oidtree.sh w/t/t0069-oidtree.sh index 889db50818..40b836aff5 100755 --- c/t/t0069-oidtree.sh +++ w/t/t0069-oidtree.sh @@ -35,13 +35,14 @@ test_expect_success 'oidtree insert and contains' ' ' test_expect_success 'oidtree each' ' - echoid "" 123 321 321 >expect && + echoid "" 123 321 321 320 321 >expect && { - echoid insert f 9 8 123 321 a b c d e && + echoid insert f 9 8 123 321 320 a b c d e && echo each 12300 && echo each 3211 && echo each 3210 && echo each 32100 && + echo each 32 && echo clear } | test-tool oidtree >actual && test_cmp expect actual