Re: [GSoC][PATCH] t: migrate helper/test-urlmatch-normalization to unit tests

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]

On Fri, Jun 28, 2024 at 06:26:24PM +0530, Ghanshyam Thakkar wrote:
> +static void compare_normalized_urls(const char *url1, const char *url2,
> +				    size_t equal)
> +{
> +	char *url1_norm = url_normalize(url1, NULL);
> +	char *url2_norm = url_normalize(url2, NULL);
> +
> +	if (equal) {
> +		if (!check_str(url1_norm, url2_norm))
> +			test_msg("input url1: %s\n  input url2: %s", url1,
> +				 url2);
> +	} else if (!check_int(strcmp(url1_norm, url2_norm), !=, 0))
> +		test_msg(" url1_norm: %s\n   url2_norm: %s\n"
> +			 "  input url1: %s\n  input url2: %s",
> +			 url1_norm, url2_norm, url1, url2);

Nit: this is missing braces around the `else if` branch. If one of the
conditional bodies has braces, then all should have according to our
style guide.

> +	free(url1_norm);
> +	free(url2_norm);
> +}
> +
> +static void check_normalized_url_from_file(const char *file, const char *expect)
> +{
> +	struct strbuf content = STRBUF_INIT, path = STRBUF_INIT;
> +
> +	strbuf_getcwd(&path);
> +	strbuf_strip_suffix(&path, "/unit-tests/bin"); /* because 'unit-tests-test-tool' is run from 'bin' directory */

Curious: is this a new requirement or do other tests have the same
requirement? I was under the impression that I could execude the
resulting unit test binaries from whatever directory I wanted to, but
didn't verify.

In any case, the line should probably be wrapped as it is overly long.

Other than that this looks good to me. I've gave a cursory read to the
testcases themselves and they do look like a faithful conversion to me.

Thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]