git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [GSoC][PATCH] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Jul 4, 2024, 16:33 UTC
Message-ID
<db2f97b6-a06e-470f-b1f9-60a78a0a2a7f@gmail.com>
In-Reply-To
<20240703034638.8019-2-shyamthakkar001@gmail.com>
Hi Ghanshyam

Overall this looks like a faithful conversion, I've left a few comments below.

On 03/07/2024 04:46, Ghanshyam Thakkar wrote:
Show 16 quoted lines
> helper/test-oid-array.c along with t0064-oid-array.sh test the
> oid-array.h library, which provides storage and processing
> 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
> 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>
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
> Note: Once 'rs/unit-tests-test-run' is merged to atleast next, I plan to
> replace these internal function used in TEST_LOOKUP() with TEST_RUN().
Nice idea
Show 57 quoted lines
> diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c
> index 37105f0a8f..8f0ccac532 100644
> --- a/t/unit-tests/lib-oid.c
> +++ b/t/unit-tests/lib-oid.c
> @@ -3,7 +3,7 @@
>   #include "strbuf.h"
>   #include "hex.h"
>   
> -static int init_hash_algo(void)
> +int init_hash_algo(void)
>   {
>   	static int algo = -1;
>   
> diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h
> index 8d2acca768..011a2d88de 100644
> --- a/t/unit-tests/lib-oid.h
> +++ b/t/unit-tests/lib-oid.h
> @@ -13,5 +13,11 @@
>    * environment variable.
>    */
>   int get_oid_arbitrary_hex(const char *s, struct object_id *oid);
> +/*
> + * 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.
> + */
> +int init_hash_algo(void);
>   
>   #endif /* LIB_OID_H */
> diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c
> new file mode 100644
> index 0000000000..0a506fab07
> --- /dev/null
> +++ b/t/unit-tests/t-oid-array.c
> @@ -0,0 +1,138 @@
> +#define USE_THE_REPOSITORY_VARIABLE
> +
> +#include "test-lib.h"
> +#include "lib-oid.h"
> +#include "oid-array.h"
> +#include "hex.h"
> +
> +static inline int test_min(int a, int b)
> +{
> +	return a <= b ? a : b;
> +}
> +
> +static int fill_array(struct oid_array *array, const char *hexes[], size_t n)
> +{
> +	for (size_t i = 0; i < n; i++) {
> +		struct object_id oid;
> +
> +		if (get_oid_arbitrary_hex(hexes[i], &oid))
> +			return -1;
> +		oid_array_append(array, &oid);
> +	}
> +	if (!check_int(array->nr, ==, n))

This should probably use check_uint() as the arguments are unsigned integers.

Show 7 quoted lines
> +		return -1;
> +	return 0;
> +}
> +
> +static int add_to_oid_array(const struct object_id *oid, void *data)
> +{
> +	struct oid_array *array = data;

style: we leave a blank line after variable declarations at the start of a block.

Show 6 quoted lines
> +	oid_array_append(array, oid);
> +	return 0;
> +}
> +
> +static void t_enumeration(const char *input_args[], size_t input_sz,
> +			  const char *result[], size_t result_sz)
style: we use "const char **arg" rather than "const char *arg[]"
Show 29 quoted lines
> +{
> +	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;
> +
> +	oid_array_for_each_unique(&input, add_to_oid_array, &actual);
> +	check_int(actual.nr, ==, expect.nr);
> +
> +	for (i = 0; i < test_min(actual.nr, expect.nr); i++) {
> +		if (!check(oideq(&actual.oid[i], &expect.oid[i])))
> +			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_int(i, ==, result_sz);
> +
> +	oid_array_clear(&actual);
> +	oid_array_clear(&input);
> +	oid_array_clear(&expect);
> +}
> +
> +#define TEST_ENUMERATION(input, result, desc)                                     \
> +	TEST(t_enumeration(input, ARRAY_SIZE(input), result, ARRAY_SIZE(result)), \
> +			   desc " works")
This macro and its helper function look good.
Show 17 quoted lines
> +static int t_lookup(struct object_id *oid_query, const char *query,
> +		    const char *hexes[], size_t n)
> +{
> +	struct oid_array array = OID_ARRAY_INIT;
> +	int ret;
> +
> +	if (get_oid_arbitrary_hex(query, oid_query))
> +		return INT_MIN;
> +	if (fill_array(&array, hexes, n))
> +		return INT_MIN;
> +	ret = oid_array_lookup(&array, oid_query);
> +
> +	oid_array_clear(&array);
> +	return ret;
> +}
> +
> +#define TEST_LOOKUP(input_args, query, condition, desc)                   \

Passing in the condition is a bit unfortunate as it means that the caller has to know which variable name to use. It might be nicer to have a function instead that takes the upper and lower bounds of the expected result and then does

	check_int(res, >=, expected_lower);
	check_int(res, <=, expected_upper);

It might be worth checking that array[res] matches the expected entry as well.

Show 50 quoted lines
> +	do {                                                              \
> +		int skip = test__run_begin();                             \
> +		if (!skip) {                                              \
> +			struct object_id oid_query;                       \
> +			int ret = t_lookup(&oid_query, query, input_args, \
> +					   ARRAY_SIZE(input_args));       \
> +                                                                          \
> +			if (ret != INT_MIN && !check(condition))          \
> +				test_msg("oid query for lookup: %s",      \
> +					 oid_to_hex(&oid_query));         \
> +		}                                                         \
> +		test__run_end(!skip, TEST_LOCATION(), desc " works");     \
> +	} while (0)
> +
> +static void setup(void)
> +{
> +	int algo = init_hash_algo();
> +	/* because the_hash_algo is used by oid_array_lookup() internally */
> +	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" };
> +
> +	if (!TEST(setup(), "setup"))
> +		test_skip_all("hash algo initialization failed");
> +
> +	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() */
> +	TEST_LOOKUP(arr_input, "55", ret == 1, "lookup");
> +	TEST_LOOKUP(arr_input, "33", ret < 0, "lookup non-existent entry");
> +	TEST_LOOKUP(arr_input_dup, "55", ret >= 3 && ret <= 5,
> +		    "lookup with duplicates");
> +	TEST_LOOKUP(arr_input_dup, "66", ret < 0,
> +		    "lookup non-existent entry with duplicates");
> +
> +	TEST_LOOKUP(((const char *[]){
> +		    "55",
> +		    init_hash_algo() == GIT_HASH_SHA1 ?
> +				"5500000000000000000000000000000000000001" :
> +				"5500000000000000000000000000000000000000000000000000000000000001" }),
> +		    "55", ret == 0, "lookup with almost duplicate values");

This might be slightly more readable if we stored the oids in a separate variable at the beginning of this function and then it would look something like

	TEST_LOOKUP(((const char *[]) {"55", nearly_55}), "55", 0, 0,
		    "lookup with almost duplicate values");

Having said that it is kind of unfortunate that we have all the variable definitions at the start as it makes it harder to see what's going on in each test. We could avoid that by using TEST_RUN() and declaring the variables in the test block. For example the first test would look like

	TEST_RUN("ordered enumeration") {
		const char *input[] = { "88", "44", "aa", "55" };
		const char *expected[] = { "44", "55", "88", "aa" };
		TEST_ENUMERATION(input, expected)
	}

where TEST_ENUMERATION is adjusted so it does not call TEST(). It's more verbose but it is clearer what the input and expected values actually are.

Best Wishes
Phillip
Show 5 quoted lines
> +	TEST_LOOKUP(((const char *[]){ "55", "55" }), "55",
> +		    ret >= 0 && ret <= 1, "lookup with single duplicate value");
> +
> +	return test_done();
> +}
Previous: Ghanshyam ThakkarNext: Ghanshyam Thakkar
Message 2 of 12 in “t: port helper/test-oid-array.c to unit-tests/t-oid-array.c”
  1. Ghanshyam ThakkarJul 3, 2024
  2. Phillip WoodJul 4, 2024
  3. Ghanshyam ThakkarAug 3, 2024
  4. [GSoC][PATCH v2] t: port helper/test-oid-array.c to unit-tests/t-oid-array.cGhanshyam Thakkar, Aug 3, 2024
  5. Christian CouderAug 19, 2024
  6. Ghanshyam ThakkarAug 24, 2024
  7. [GSoC][PATCH v3] t: port helper/test-oid-array.c to unit-tests/t-oid-array.cGhanshyam Thakkar, Aug 24, 2024
  8. Christian CouderAug 25, 2024
  9. Ghanshyam ThakkarAug 25, 2024
  10. t: port helper/test-oid-array.c to unit-tests/t-oid-array.cGhanshyam Thakkar, Sep 1, 2024
  11. Christian CouderSep 4, 2024
  12. Junio C HamanoSep 4, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.