Re: [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey! Thanks for fixes, see comments below.

On 5/24/23 09:41, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
>
> Fixed some of your comments, branch is force-pushed.
>
<snipped>
> The <test.c> module serves to achieve these goals without too fancy
> features.
>
> It's functionality inspired by cmoka API [1], but only TAP14 [2]
>> typo: cmocka
> Fixed, thanks!


Nit: probably it is better "CMocka", because it is a proper name ("имя 
собственное") and should be with capital letters.

Feel free to ignore.

>
>>> protocol is supported (Version of TAP set to 13 to be compatible with
>>> old TAP13 harnesses).
>> Please add tests for TAP13/TAP14 conformance testing.
>>
>> It would be unpleasant if proposed library will produce TAP-incompatible
>> output and it will break parsing in 'prove'.
>>
>> At least single test for passed testcase "ok", single testcase for
>> failed "not ok" testcase, one testcase for every directive.
> I've added the tests for ok|skip|todo. "not ok" is skipped, because it
> rather tricky to test it behaviour via prove without test failure.
>
> The content of <test/tarantool-c-tests/unit-tap.test.c> is the
> following:
> ===================================================================
> #include "test.h"
>
> #define UNUSED(x) ((void)(x))
>
> static int test_ok(void *test_state)
> {
> 	UNUSED(test_state);
> 	return TEST_EXIT_SUCCESS;
> }
>
> static int test_skip(void *test_state)
> {
> 	UNUSED(test_state);
> 	return skip("test skip");
> }
>
> static int test_todo(void *test_state)
> {
> 	UNUSED(test_state);
> 	return todo("test todo");
> }
>
> int main(void)
> {
> 	const struct test_unit tgroup[] = {
> 		test_unit_new(test_ok),
> 		test_unit_new(test_skip),
> 		test_unit_new(test_todo)
> 	};
> 	return test_run_group(tgroup, NULL);
> }
> ===================================================================

This test checks that macroses/functions in test.h could be called and 
nothing more.

We need checking TAP output that binary will emit for different test 
statuses, plan, testcase counters ("1..10").


>
>>> The group of unit tests is declared like the following:
> <snipped>
>
>>> [1]: https://github.com/clibs/cmocka
>>> [2]: https://testanything.org/tap-version-14-specification.html
>>>
>>> Part of tarantool/tarantool#7900
>>> ---
>>>    test/CMakeLists.txt                   |   2 +
>>>    test/tarantool-c-tests/CMakeLists.txt |  41 +++++
>>>    test/tarantool-c-tests/test.c         | 251 ++++++++++++++++++++++++++
>>>    test/tarantool-c-tests/test.h         | 217 ++++++++++++++++++++++
>>>    4 files changed, 511 insertions(+)
>>>    create mode 100644 test/tarantool-c-tests/CMakeLists.txt
>>>    create mode 100644 test/tarantool-c-tests/test.c
>>>    create mode 100644 test/tarantool-c-tests/test.h
>>>
>>> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
>>> index a8262b12..47296a22 100644
>>> --- a/test/CMakeLists.txt
>>> +++ b/test/CMakeLists.txt
>>> @@ -48,12 +48,14 @@ separate_arguments(LUAJIT_TEST_COMMAND)
>>>    add_subdirectory(LuaJIT-tests)
>>>    add_subdirectory(PUC-Rio-Lua-5.1-tests)
>>>    add_subdirectory(lua-Harness-tests)
>>> +add_subdirectory(tarantool-c-tests)
>>>    add_subdirectory(tarantool-tests)
>>>    
>>>    add_custom_target(${PROJECT_NAME}-test DEPENDS
>>>      LuaJIT-tests
>>>      PUC-Rio-Lua-5.1-tests
>>>      lua-Harness-tests
>>> +  tarantool-c-tests
>>>      tarantool-tests
>> Should we rename tarantool-tests to tarantool-lua-tests in a separate
>> commit?
> Maybe, but I don't really want to do it since inconsistencies with
> Tarantool's integration suite, so I suppose we can leave it as is, can't
> we?

Sure, let's skip.


>>>    )
>>>    
>>> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
>>> new file mode 100644
>>> index 00000000..c6b7cd30
>>> --- /dev/null
>>> +++ b/test/tarantool-c-tests/CMakeLists.txt
>>> @@ -0,0 +1,41 @@
>>> +find_program(PROVE prove)
>>> +if(NOT PROVE)
>>> +  message(WARNING "`prove' is not found, so tarantool-c-tests target is not generated")
>>> +  return()
>>> +endif()
>> copy-pasted from test/tarantool-tests/CMakeLists.txt, I would place it
>> to test/CMakeLists.txt.
>>
>> At least searching prove (find_program(PROVE prove)).
> As, discussed here [1], this refactoring should be the part of the separate
> pachset.


OK

>
>>> +
>>> +set(C_TEST_SUFFIX .c_test)
>> Lua tests in test/tarantool-tests have prefix ".test.lua".
>>
>> Should we use here ".test.c" for consistency?
> "c_test" is the suffix for executable files.
> ".test.c" is used for test files themselves.


OK

>
>>
>>> +set(C_TEST_FLAGS --failures --shuffle)
>>> +
> <snipped>
>
>>> diff --git a/test/tarantool-c-tests/test.c b/test/tarantool-c-tests/test.c
>>> new file mode 100644
>>> index 00000000..74cba3a3
>>> --- /dev/null
>>> +++ b/test/tarantool-c-tests/test.c
> <snipped>
>
>>> diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
>>> new file mode 100644
>>> index 00000000..28df9daf
>>> --- /dev/null
>>> +++ b/test/tarantool-c-tests/test.h
>>> @@ -0,0 +1,217 @@
>>> +#ifndef TEST_H
>>> +#define TEST_H
>> I would use prefix to avoid intersection with another headers. For
>> example "TARANTOOL_TEST_H" or something else.
> Replaced with TARANTOOL_LUAJIT_TEST_H. See the iterative patch below:

Good, thanks!


>
> ===================================================================
> diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
> index 28df9daf..047f01a5 100644
> --- a/test/tarantool-c-tests/test.h
> +++ b/test/tarantool-c-tests/test.h
> @@ -1,5 +1,5 @@
> -#ifndef TEST_H
> -#define TEST_H
> +#ifndef TARANTOOL_LUAJIT_TEST_H
> +#define TARANTOOL_LUAJIT_TEST_H
>   
>   #include <stdio.h>
>   #include <stdlib.h>
> @@ -214,4 +214,4 @@ static inline int todo(const char *reason)
>           );                                                              \
>   } while (0)
>   
> -#endif /* TEST_H */
> +#endif /* TARANTOOL_LUAJIT_TEST_H */
> ===================================================================
>
> Same for utils header:
>
> ===================================================================
> diff --git a/test/tarantool-c-tests/utils.h b/test/tarantool-c-tests/utils.h
> index cf668006..f651f7f6 100644
> --- a/test/tarantool-c-tests/utils.h
> +++ b/test/tarantool-c-tests/utils.h
> @@ -1,3 +1,6 @@
> +#ifndef TARANTOOL_LUAJIT_TEST_UTILS_H
> +#define TARANTOOL_LUAJIT_TEST_UTILS_H
> +
>   #include <limits.h>
>   #include <string.h>
>   
> @@ -61,3 +64,5 @@ static void utils_lua_close(lua_State *L)
>           if (!lua_isfunction((L), -1))                                   \
>                   bail_out("Can't get auxiliary test function");          \
>   } while (0)
> +
> +#endif /* TARANTOOL_LUAJIT_TEST_UTILS_H */
> ===================================================================
>
>>> +
> <snipped>
>
>>> +#endif /* TEST_H */
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2023-May/027577.html
>