Re: [Tarantool-patches] [PATCH v2 luajit 4/6] test: rewrite misclib-getmetrics-capi test in C

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

Hi, Maxim!
Thanks for the review!
See my answers below.

On 19.05.23, Maxim Kokryashkin wrote:
> 
> Hi, Sergey
> Thanks for the patch.
> LGTM, except for a few comments below.
>  
> > 

<snipped>

> >>diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> >>index c6b7cd30..c9d75d6c 100644
> >>--- a/test/tarantool-c-tests/CMakeLists.txt
> >>+++ b/test/tarantool-c-tests/CMakeLists.txt
> >>@@ -22,13 +22,37 @@ set_target_properties(libtest PROPERTIES
> >>   LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
> >> )
> >> 
> >>-# XXX: For now, just build libtest. The tests to be depended on
> >>-# will be added in the next commit.
> >>+# TARGET_C_FLAGS is required here to be sure that headers like
> >>+# lj_arch.h in compiled test are consistent with the LuaJIT library
> >>+# to link.
> >>+AppendFlags(TESTS_C_FLAGS ${TARGET_C_FLAGS})
> >>+
> >>+set(CTEST_SRC_SUFFIX ".test.c")
> >>+file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}")
> >>+foreach(test_source ${tests})
> >>+ string(REGEX REPLACE ".+/([^/]+)${CTEST_SRC_SUFFIX}" "\\1" exe ${test_source})
> >Please drop a comment explaining the line above.

Added the following comment.
Branch is force-pushed.

===================================================================
diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
index c9d75d6c..20495386 100644
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -30,6 +30,7 @@ AppendFlags(TESTS_C_FLAGS ${TARGET_C_FLAGS})
 set(CTEST_SRC_SUFFIX ".test.c")
 file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}")
 foreach(test_source ${tests})
+  # Get test name without suffix. Needed to set OUTPUT_NAME.
   string(REGEX REPLACE ".+/([^/]+)${CTEST_SRC_SUFFIX}" "\\1" exe ${test_source})
   add_executable(${exe} EXCLUDE_FROM_ALL ${test_source})
   target_include_directories(${exe} PRIVATE
===================================================================

> > 
> ><snipped>
> >>
> >>diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
> >>similarity index 68%
> >>rename from test/tarantool-tests/misclib-getmetrics-capi.test.lua
> >>rename to test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
> >>index 654e5545..2f0ee5cf 100644
> >>--- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua
> >>+++ b/test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
> ><snipped>
> >>diff --git a/test/tarantool-c-tests/misclib-getmetrics-capi.test.c b/test/tarantool-c-tests/misclib-getmetrics-capi.test.c
> >>new file mode 100644
> >>index 00000000..202cd395
> >>--- /dev/null
> >>+++ b/test/tarantool-c-tests/misclib-getmetrics-capi.test.c

<snipped>

> >>+ lua_gc(L, LUA_GCCOLLECT, 0);
> >>+ luaM_metrics(L, &newm);
> >>+ assert_true(newm.gc_steps_pause - oldm.gc_steps_pause == 1);
> >>+ assert_true(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 1);
> >>+ assert_true(newm.gc_steps_atomic - oldm.gc_steps_atomic == 1);
> >>+ assert_true(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 1);
> >>+ assert_true(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 1);
> >>+ /* Nothing to finalize, skipped. */
> >>+ assert_true(newm.gc_steps_finalize == 0);
> >>+ oldm = newm;
> >>+
> >>+ /*
> >>+ * Now let's run three GC cycles to ensure that
> >>+ * increment was not a lucky coincidence.
> >>+ */
> >>+ lua_gc(L, LUA_GCCOLLECT, 0);
> >>+ lua_gc(L, LUA_GCCOLLECT, 0);
> >>+ lua_gc(L, LUA_GCCOLLECT, 0);
> >I think it is better to create a for loop here.

I just reuse code from the previous implementation, there is no goal to
refactor it. Also, use 3 calls in a raw looks OK to me.

> >>+ luaM_metrics(L, &newm);
> >>+ assert_true(newm.gc_steps_pause - oldm.gc_steps_pause == 3);
> >>+ assert_true(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 3);
> >>+ assert_true(newm.gc_steps_atomic - oldm.gc_steps_atomic == 3);
> >>+ assert_true(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 3);
> >>+ assert_true(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 3);
> >That part is duplicated. It is better to either write a corresponding macro, or
> >a function.

Ditto.

> >>+ /* Nothing to finalize, skipped. */

<snipped>

> >>+/*
> >>+ * Get function to call to generate the corresponding snapshot
> >>+ * restores on top of the Lua stack. Function returns the amount
> >>+ * of snapshot restorations expected.
> >>+ * Clear stack after call.
> >>+ */
> >>+static void check_snap_restores(lua_State *L)
> >>+{
> >>+ struct luam_Metrics oldm, newm;
> >>+ luaM_metrics(L, &oldm);
> >>+ /* Generate snapshots. */
> >>+ lua_call(L, 0, 1);
> >>+ int n = lua_gettop(L);
> >>+ /*
> >>+ * The first value is the table with functions,
> >>+ * the second is number of snapshot restores.
> >>+ */
> >>+ if (n != 2 || !lua_isnumber(L, -1))
> >>+ bail_out("incorrect return value: 1 number is required");
> >>+ size_t snap_restores = lua_tonumber(L, -1);
> >>+ luaM_metrics(L, &newm);
> >>+ /*
> >>+ * Remove `snap_restores` from stack.
> >>+ * Must be done before potiential assert and exit from
> >>+ * the test.
> >>+ */
> >>+ lua_pop(L, 1);
> >>+ assert_true(newm.jit_snap_restore - oldm.jit_snap_restore
> >>+ == snap_restores);
> >>+}
> >>+
> >>+static int snap_restores_direct_exit(void *test_state)
> >>+{
> >>+ lua_State *L = test_state;
> >>+ utils_get_aux_lfunc(L);
> >>+ check_snap_restores(L);
> >>+ return TEST_EXIT_SUCCESS;
> >>+}
> >>+
> >>+static int snap_restores_direct_exit_scalar(void *test_state)
> >>+{
> >>+ lua_State *L = test_state;
> >>+ utils_get_aux_lfunc(L);
> >>+ check_snap_restores(L);
> >>+ return TEST_EXIT_SUCCESS;
> >>+}
> >>+
> >>+static int snap_restores_side_exit_compiled(void *test_state)
> >>+{
> >>+ lua_State *L = test_state;
> >>+ utils_get_aux_lfunc(L);
> >>+ check_snap_restores(L);
> >>+ return TEST_EXIT_SUCCESS;
> >>+}
> >>+
> >>+static int snap_restores_side_exit_not_compiled(void *test_state)
> >>+{
> >>+ lua_State *L = test_state;
> >>+ utils_get_aux_lfunc(L);
> >>+ check_snap_restores(L);
> >>+ return TEST_EXIT_SUCCESS;
> >>+}
> >>+
> >Same here. Either a dedicated implementation func, or a macro is needed here.

`utils_get_aux_lfunc(L)` helper macro looks to the __func__ value, to
load the corresponding function from Lua script. We may add the macro to
generate functions, but I doubt that it will improve readability of the
code. I may add an additional comment to `check_snap_restores()` about
`utils_get_aux_lfunc(L)`, but existing one looks self-sufficient for me
right now, so please guide me here:).

> >>+static int snap_restores_group(void *test_state)

<snipped>

> >>diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt

<snipped>

> >>--
> >>2.34.1
> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun