Re: [GSoC][PATCH v3] t: migrate t0110-urlmatch-normalization to the new framework
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Aug 19, 2024, 12:46 UTC
- Message-ID
- <CAP8UFD2-VbyK-ZecDKEvgKicWrVe=e=z6mH_xjmrf=a4ZAYd8w@mail.gmail.com>
- In-Reply-To
- <20240814142057.94671-1-shyamthakkar001@gmail.com>
On Wed, Aug 14, 2024 at 4:21 PM Ghanshyam Thakkar <shyamthakkar001@gmail.com> wrote:
Show 8 quoted lines
> > helper/test-urlmatch-normalization along with > t0110-urlmatch-normalization test the `url_normalize()` function from > 'urlmatch.h'. Migrate them to the unit testing framework for better > performance. And also add different test_msg()s for better debugging. > > In the migration, last two of the checks from `t_url_general_escape()` > were slightly changed compared to the shellscript. This involves changing
Nit: s/shellscript/shell script/
Show 7 quoted lines
> > '\'' -> ' > '\!' -> ! > > in the urls of those checks. This is because in C strings, we don't > need to escape "'" and "!". Other than these two, all the urls were > pasted verbatim from the shellscript.
Nit: s/shellscript/shell script/
> Another change is the removal of MINGW prerequisite from one of the
Nit: s/of MINGW prerequisite/of a MINGW prerequisite/
Show 18 quoted lines
> test. It was there because[1] on Windows, the command line is a > Unicode string, it is not possible to pass arbitrary bytes to a > program. But in unit tests we don't have this limitation. > > And since we can construct strings with arbitrary bytes in C, let's > also remove the test files which contain URLs with arbitrary bytes in > the 't/t0110' directory and instead embed those URLs in the unit test > code itself. > > [1]: https://lore.kernel.org/git/53CAC8EF.6020707@gmail.com/ > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> > --- > This version addresses Junio's review and removes the restriction > of running the unit tests in the 't/' and 't/unit-tests/bin' > introduced in v2 by embedding the URLs in the code itself.
Nice change.
[...]
> +static void compare_normalized_urls(const char *url1, const char *url2, > + size_t equal)
Nit: it's better to use 'unsigned int' or just 'int' for bool flags like "equal". Or is there a reason to use 'size_t' instead?
Show 12 quoted lines
> +{
> + 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: something like "normalized url1" might be a bit better than "url1_norm" as for the 'url1' variable we use "input url1" instead of just "url1".
Show 13 quoted lines
> + }
> + free(url1_norm);
> + free(url2_norm);
> +}
> +
> +static void check_normalized_url_length(const char *url, size_t len)
> +{
> + struct url_info info;
> + char *url_norm = url_normalize(url, &info);
> +
> + if (!check_int(info.url_len, ==, len))
> + test_msg(" input url: %s\n normalized url: %s", url,
> + url_norm);Above "normalized url" is used for "url_norm" which is good.
> + free(url_norm); > +} > + > +/* Note that only file: URLs should be allowed without a host */
Nit: maybe s/file:/"file:"/ would make things a bit clearer.
[...]
Show 5 quoted lines
> +/* > + * http://@foo specifies an empty user name but does not specify a password > + * http://foo specifies neither a user name nor a password > + * So they should not be equivalent > + */
Nit: the above comment would be a bit better with URLs inside double quotes, with a full stop (period) at the end of each sentence and with only one space character between "http://foo" and "specifies".
Except for the above nits, I am happy with this version. Thanks.