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

From: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Date: Jul 24, 2024, 00:24 UTC
Message-ID: <D2XCB5UYIB5B.1L3U95DQSWJ2@gmail.com>
In-Reply-To: <Zp-3e6VV5bl8dWvR@tanuki>

Patrick Steinhardt <ps@pks.im> wrote:

Show 19 quoted lines

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

Show 16 quoted lines

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

Show 7 quoted lines

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