Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jan 9, 2024, 10:35 UTC
- 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:
Show 30 quoted lines
> 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
Show 9 quoted lines
> -#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
Show 8 quoted lines
> +/* 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