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

["Ghanshyam Thakkar" <shyamthakkar001@gmail.com>]

Patrick Steinhardt <ps@pks.im> wrote:
> 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.

Will update.

>
> > +	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.

I am not aware of any requirements, but if we want to interact with
other files like in this case (and where we potentially have to
interact with a test repository), we'd need to have some requirement
to construct the path to these data files (and the test repository),
similar to end-to-end tests where they can be run in only t/
directory. Do you think calling `setup_git_directory()` and then using
`the_repository->worktree` to get the root of the worktree of Git source
and then construct the path relative to that, would be useful? That way
we can atleast call the binaries from anywhere within the tree.

(P.S. I know we want to avoid using `the_repository`, but I don't know
any other way yet.)

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

Will update.

Thank you.

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