Re: [GSoC][PATCH v2] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Aug 19, 2024, 16:55 UTC
- Message-ID
- <CAP8UFD3E2idN6mUYzEyh11Fzmj07q+BQuyVCtUkPP=cuxsUODw@mail.gmail.com>
- In-Reply-To
- <20240803132206.72166-1-shyamthakkar001@gmail.com>
On Sat, Aug 3, 2024 at 3:22 PM Ghanshyam Thakkar <shyamthakkar001@gmail.com> wrote:
> > helper/test-oid-array.c along with t0064-oid-array.sh test the > oid-array.h library, which provides storage and processing
Nit: I think "oid-array.h" is more an API than a library.
> efficiency over large lists of object identifiers. > > Migrate them to the unit testing framework for better runtime > performance and efficiency. Also 'the_hash_algo' is used internally in
It doesn't seem to me that a variable called 'the_hash_algo' is used internally in oid_array_lookup() anymore.
Show 17 quoted lines
> oid_array_lookup(), but we do not initialize a repository directory, > therefore initialize the_hash_algo manually. And > init_hash_algo():lib-oid.c can aid in this process, so make it public. > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> > --- > Changes in v2: > - removed the use of internal test__run_*() functions. > - TEST_LOOKUP() is entirely changed where it now accepts a lower bound > and an upper bound for checking return values of oid_array_lookup(), > instead of passing the condition as a whole literal. (i.e. > v1: TEST_LOOKUP(..., ret < 0, ...) > v2: TEST_LOOKUP(..., INT_MIN, -1, ...) > )
Nice improvements.
> - TEST_ENUMERATION() remains unchanged.
[...]
Show 5 quoted lines
> +/*
> + * Returns one of GIT_HASH_{SHA1, SHA256, UNKNOWN} based on the value of
> + * GIT_TEST_DEFAULT_HASH. The fallback value in case of absence of
> + * GIT_TEST_DEFAULT_HASH is GIT_HASH_SHA1.
> + */In this comment, it might be helpful to say that GIT_TEST_DEFAULT_HASH is an environment variable (while GIT_HASH_* are #defined values). Also while at it, it might be helpful to say that the function uses check(algo != GIT_HASH_UNKNOWN) before returning to verify that GIT_TEST_DEFAULT_HASH is either unset or properly set.
> +int init_hash_algo(void);
[...]
Show 11 quoted lines
> +static void t_enumeration(const char **input_args, size_t input_sz,
> + const char **result, size_t result_sz)
> +{
> + struct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT,
> + actual = OID_ARRAY_INIT;
> + size_t i;
> +
> + if (fill_array(&input, input_args, input_sz))
> + return;
> + if (fill_array(&expect, result, result_sz))
> + return;It would have been nice if the arguments were called 'expect_args' and 'expect_sz' in the same way as for 'input'. Is there a reason why we couldn't just use 'expect' (or maybe 'expected') everywhere instead of 'result'?
Also after the above 'input.nr' is equal to 'input_sz' and 'expect.nr' is equal to 'result_sz' otherwise we would have already returned fron the current function.
> + oid_array_for_each_unique(&input, add_to_oid_array, &actual); > + check_uint(actual.nr, ==, expect.nr);
I think it might be better to return if this check fails. Otherwise it means that we likely messed something up in the 'input_args' or 'result' arguments we passed to the function, and then...
> + for (i = 0; i < test_min(actual.nr, expect.nr); i++) {
> + if (!check(oideq(&actual.oid[i], &expect.oid[i])))...we might not compare here the input oid with the corresponding result oid we intended to compare it to. This might result in a lot of not very relevant output.
Returning if check_uint(actual.nr, ==, expect.nr) fails would avoid such output and also enable us to just use 'actual.nr' instead of 'test_min(actual.nr, expect.nr)' in the 'for' loop above.
Show 5 quoted lines
> + test_msg("expected: %s\n got: %s\n index: %" PRIuMAX,
> + oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),
> + (uintmax_t)i);
> + }
> + check_uint(i, ==, result_sz);As we saw above that 'expect.nr' is equal to 'result_sz', this check can fail only if 'actual.nr' is different from 'expect.nr' which we already checked above. So I think this check is redundant and we might want to get rid of it.
> + oid_array_clear(&actual); > + oid_array_clear(&input); > + oid_array_clear(&expect); > +}
[...]
Show 9 quoted lines
> +static void t_lookup(const char **input_hexes, size_t n, const char *query_hex,
> + int lower_bound, int upper_bound)
> +{
> + struct oid_array array = OID_ARRAY_INIT;
> + struct object_id oid_query;
> + int ret;
> +
> + if (get_oid_arbitrary_hex(query_hex, &oid_query))
> + return;In fill_array() above, we use check_int() to check the result of get_oid_arbitrary_hex() like this:
if (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))
It doesn't look consistent to not use check_int() to check the result of get_oid_arbitrary_hex() here. Or is there a specific reason to do it in one place but not in another?
Show 20 quoted lines
> + if (fill_array(&array, input_hexes, n))
> + return;
> + ret = oid_array_lookup(&array, &oid_query);
> +
> + if (!check_int(ret, <=, upper_bound) ||
> + !check_int(ret, >=, lower_bound))
> + test_msg("oid query for lookup: %s", oid_to_hex(&oid_query));
> +
> + oid_array_clear(&array);
> +}
> +
> +#define TEST_LOOKUP(input_hexes, query, lower_bound, upper_bound, desc) \
> + TEST(t_lookup(input_hexes, ARRAY_SIZE(input_hexes), query, \
> + lower_bound, upper_bound), \
> + desc " works")
> +
> +static void setup(void)
> +{
> + int algo = init_hash_algo();
> + /* because the_hash_algo is used by oid_array_lookup() internally */I think this comment should be above the first line in this function as it also explains why we need to use init_hash_algo().
Also something like "/* The hash algo is used by oid_array_lookup() internally */" seems better to me as there is no 'the_hash_algo' global variable used by oid_array_lookup().
Show 15 quoted lines
> + if (check_int(algo, !=, GIT_HASH_UNKNOWN))
> + repo_set_hash_algo(the_repository, algo);
> +}
> +
> +int cmd_main(int argc UNUSED, const char **argv UNUSED)
> +{
> + const char *arr_input[] = { "88", "44", "aa", "55" };
> + const char *arr_input_dup[] = { "88", "44", "aa", "55",
> + "88", "44", "aa", "55",
> + "88", "44", "aa", "55" };
> + const char *res_sorted[] = { "44", "55", "88", "aa" };
> + const char *nearly_55;
> +
> + if (!TEST(setup(), "setup"))
> + test_skip_all("hash algo initialization failed");Nice that we skip all the other tests with a helpful message if setup() fails.
Show 5 quoted lines
> + TEST_ENUMERATION(arr_input, res_sorted, "ordered enumeration"); > + TEST_ENUMERATION(arr_input_dup, res_sorted, > + "ordered enumeration with duplicate suppression"); > + > + /* ret is the return value of oid_array_lookup() */
This comment is not relevant anymore in this version.
Show 19 quoted lines
> + TEST_LOOKUP(arr_input, "55", 1, 1, "lookup");
> + TEST_LOOKUP(arr_input, "33", INT_MIN, -1, "lookup non-existent entry");
> + TEST_LOOKUP(arr_input_dup, "55", 3, 5, "lookup with duplicates");
> + TEST_LOOKUP(arr_input_dup, "66", INT_MIN, -1,
> + "lookup non-existent entry with duplicates");
> +
> + nearly_55 = init_hash_algo() == GIT_HASH_SHA1 ?
> + "5500000000000000000000000000000000000001" :
> + "5500000000000000000000000000000000000000000000000000000000000001";
> + TEST_LOOKUP(((const char *[]){ "55", nearly_55 }), "55", 0, 0,
> + "lookup with almost duplicate values");
> + TEST_LOOKUP(((const char *[]){ "55", "55" }), "55", 0, 1,
> + "lookup with single duplicate value");
> +
> + return test_done();
> +}
> --
> 2.46.0
>