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

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

Hi, Sergey!
Thanks for the review!

Fixed some of your comments, branch is force-pushed.

On 22.05.23, Sergey Bronnikov wrote:
> Hi, Sergey!
> 
> 
> Thanks for the patch! Please see my comments inline.
> 
> 
> On 5/18/23 23:44, Sergey Kaplun wrote:
> > We need an instrument to write tests in plain C for LuaJIT, to be able:
> > * easily test LuaC API
> > * test patches without usage of plain Lua
> > * write unit tests
> > * startup LuaJIT with custom memory allocator, to test some GC issues
> > * maybe, in future, use custom hashing function to test a behavior
> >    of LuaJIT tables
> > and so on.
> 
> Unexpected newline, put this on a previous line.

Fixed.

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

> > 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);
}
===================================================================

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

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

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

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

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

-- 
Best regards,
Sergey Kaplun