From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 09 Jan 2024 10:35:11 GMT
Subject: Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c
Message-ID: <33c81610-0958-49da-b702-ba8d96ecf1d3@gmail.com>
In-Reply-To: <20240105161413.10422-1-ach.lumap@gmail.com>

Hi Achu

On 05/01/2024 16:14, Achu Luma wrote:
> In the recent codebase update (8bf6fbd00d (Merge branch
> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> merged, providing a standardized approach for testing C code. Prior to
> this update, some unit tests relied on the test helper mechanism,
> lacking a dedicated unit testing framework. It's more natural to perform
> these unit tests using the new unit test framework.
> 
> This commit migrates the unit tests for C character classification
> functions (isdigit(), isspace(), etc) from the legacy approach
> using the test-tool command `test-tool ctype` in t/helper/test-ctype.c
> to the new unit testing framework (t/unit-tests/test-lib.h).
> 
> The migration involves refactoring the tests to utilize the testing
> macros provided by the framework (TEST() and check_*()).
> 
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Helped-by: René Scharfe <l.s.r@web.de>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Achu Luma <ach.lumap@gmail.com>
> ---
>   The changes between version 3 and version 4 are the following:
> 
>   - Some duplication has been reduced using a new TEST_CHAR_CLASS() macro.
>   - A "0x"prefix has been added to avoid confusion between decimal and
>     hexadecimal codes printed by test_msg().
>   - The "Mentored-by:..." trailer has been restored.
>   - "works as expected" has been reduced to just "works" as suggested by Taylor.
>   - Some "Helped-by: ..." trailers have been added.
>   - Some whitespace fixes have been made.

Thanks for the re-roll, the changes look good, there is one issue that I 
spotted below

> -#define TEST_CLASS(t,s) {			\
> -	int i;					\
> -	for (i = 0; i < 256; i++) {		\
> -		if (is_in(s, i) != t(i))	\
> -			report_error(#t, i);	\
> -	}					\
> -	if (t(EOF))				\
> -		report_error(#t, EOF);		\
> -}

In the old version we test EOF in addition to each character between 0-255

> +/* Macro to test a character type */
> +#define TEST_CTYPE_FUNC(func, string) \
> +static void test_ctype_##func(void) { \
> +	for (int i = 0; i < 256; i++) { \
> +		if (!check_int(func(i), ==, is_in(string, i))) \
> +			test_msg("       i: 0x%02x", i); \
> +	} \
> +}

In the new version we only test the characters 0-255, not EOF. If there 
is a good reason for removing the EOF tests then it should be explained 
in the commit message. If not it would be good to add those tests back.

Best Wishes

Phillip