Re: [Tarantool-patches] [PATCH v2 luajit 3/6] test: introduce utils.h helper for C tests

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

Hi, Maxim!
Thanks for the review!
Please, see my comments below.

On 19.05.23, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> Please consider a few comments below.
> > 
> >>This header contains generic init and close for tests and helpers for
> >>loading auxiliary Lua script with functions to run inside a C test.
> >>
> >>It will be properly used in the next commit.
> >>
> >>Part of tarantool/tarantool#7900
> >>---
> >> test/tarantool-c-tests/utils.h | 63 ++++++++++++++++++++++++++++++++++
> >> 1 file changed, 63 insertions(+)
> >> create mode 100644 test/tarantool-c-tests/utils.h
> >>
> >>diff --git a/test/tarantool-c-tests/utils.h b/test/tarantool-c-tests/utils.h
> >>new file mode 100644
> >>index 00000000..cf668006
> >>--- /dev/null
> >>+++ b/test/tarantool-c-tests/utils.h
> >>@@ -0,0 +1,63 @@
> >>+#include <limits.h>
> >>+#include <string.h>
> >>+
> >>+#include "lauxlib.h"
> >>+#include "lua.h"
> >>+#include "luajit.h"
> >>+#include "lualib.h"
> >>+
> >>+#include "test.h"
> >>+
> >>+/* Generic init for our tests. */
> >>+static lua_State *utils_lua_init(void)
> >>+{
> >>+ lua_State *L = luaL_newstate();
> >>+ if (!L)
> >>+ bail_out("Can't init Lua state");
> >>+ /* Stop collector during library initialization. */
> >>+ lua_gc(L, LUA_GCSTOP, 0);
> >>+ luaL_openlibs(L);
> >>+ lua_gc(L, LUA_GCRESTART, -1);
> >>+ return L;
> >>+}
> >>+
> >>+/* Generic close for our tests. */
> >>+static void utils_lua_close(lua_State *L)
> >>+{
> >>+ lua_close(L);
> >>+}
> >Do we __really__ need this function?)

We haven't any other work to do __for now__, but with time maybe other
checks will come here.
So, leave it as is for consistensy, if you don't mind.

> >>+
> >>+/*
> >>+ * Load the pair to the test file <filename-script.lua>.
> >>+ * Each file should return the table with functions (named the
> >>+ * same as the corresponding unit tests) to call in unit tests.
> >>+ * Push the table with those functions on the Lua stack.
> >>+ */
> >>+#define utils_load_aux_script(L) do { \
> >>+ /* \
> >>+ * Format script name. \
> >>+ * `__ABS_FILENAME__` is set via CMake. \
> >>+ */ \
> >>+ char script[PATH_MAX] = __ABS_FILENAME__; \
> >>+ char *file_suffix = strstr(script, ".test.c"); \
> >The `strstr` function is not safe, use `strnstr` instead.
> >Also, maybe it is better to use the `strrstr` variation,
> >because it searches from the end. It eleminates issues
> >in case of a file with .test.c somewhere else in the name.

There's no function named `strnstr()`, in standard C or POSIX. It's not
in glibc/Linux either, IINM. I want to avoid additional linking
dependencies (as for -lsstrings2). `strstr()` is unsafe when we have
non-null terminated strings. It's not our case.

> >>+ strcpy(file_suffix, "-script.lua"); \
> >Same applies here.

Don't see the reason to write `"script.lua", sizeof("script.lua")`
| The strncpy() function is similar, except that at most n bytes of
| src are copied.  Warning: If there is no null byte among the
| first n bytes of src, the string placed in dest will not be
| null-terminated.

So, I'll just use `strcpy()`.

> >>+ \
> >>+ if (luaL_dofile((L), script) != LUA_OK) { \
> >>+ test_comment("Can't load %s: '%s'", script, \
> >>+ lua_tostring((L), -1)); \
> >>+ bail_out("Can't load auxiliary script"); \
> >>+ } \
> >>+ \
> >>+ if (!lua_istable((L), -1)) \
> >>+ bail_out("Returned value from script is not a table"); \
> >>+} while (0)
> >>+
> >>+/*
> >>+ * Accept a table on top of the Lua stack which containing the
> >>+ * function named as the unit test we currently executing.
> >>+ */
> >>+#define utils_get_aux_lfunc(L) do { \
> >>+ lua_getfield((L), -1, __func__); \
> >>+ if (!lua_isfunction((L), -1)) \
> >>+ bail_out("Can't get auxiliary test function"); \
> >>+} while (0)
> >>--
> >>2.34.1
> > 
> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun