Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 luajit 4/6] test: rewrite misclib-getmetrics-capi test in C
Date: Sat, 20 May 2023 11:08:19 +0300	[thread overview]
Message-ID: <ZGh/8w/OI16ove2r@root> (raw)
In-Reply-To: <1684498677.336586177@f752.i.mail.ru>

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

  reply	other threads:[~2023-05-20  8:12 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-18 20:44 [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests Sergey Kaplun via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 1/6] test: fix setting of {DY}LD_LIBRARY_PATH variables Sergey Kaplun via Tarantool-patches
2023-05-19 11:23   ` Maxim Kokryashkin via Tarantool-patches
2023-05-22 11:03   ` Sergey Bronnikov via Tarantool-patches
2023-05-23  6:47     ` Sergey Kaplun via Tarantool-patches
2023-05-29 14:37       ` Sergey Bronnikov via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests Sergey Kaplun via Tarantool-patches
2023-05-19 11:46   ` Maxim Kokryashkin via Tarantool-patches
2023-05-22 12:33   ` Sergey Bronnikov via Tarantool-patches
2023-05-24  6:41     ` Sergey Kaplun via Tarantool-patches
2023-05-25 17:33       ` Sergey Bronnikov via Tarantool-patches
2023-05-29 10:03         ` Sergey Kaplun via Tarantool-patches
2023-05-29 14:38           ` Sergey Bronnikov via Tarantool-patches
2023-05-31 13:32             ` Sergey Kaplun via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 3/6] test: introduce utils.h helper " Sergey Kaplun via Tarantool-patches
2023-05-19 11:58   ` Maxim Kokryashkin via Tarantool-patches
2023-05-20  7:52     ` Sergey Kaplun via Tarantool-patches
2023-05-29 15:26   ` Sergey Bronnikov via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 4/6] test: rewrite misclib-getmetrics-capi test in C Sergey Kaplun via Tarantool-patches
2023-05-19 12:17   ` Maxim Kokryashkin via Tarantool-patches
2023-05-20  8:08     ` Sergey Kaplun via Tarantool-patches [this message]
2023-05-29 16:15   ` Sergey Bronnikov via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 5/6] test: rewrite misclib-sysprof-capi " Sergey Kaplun via Tarantool-patches
2023-05-19 13:00   ` Maxim Kokryashkin via Tarantool-patches
2023-05-20  7:28     ` Sergey Kaplun via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 6/6] test: rewrite lj-49-bad-lightuserdata " Sergey Kaplun via Tarantool-patches
2023-05-19 12:40   ` Maxim Kokryashkin via Tarantool-patches
2023-05-19 14:29 ` [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests Maxim Kokryashkin via Tarantool-patches
2023-05-20  8:38   ` Sergey Kaplun via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZGh/8w/OI16ove2r@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 luajit 4/6] test: rewrite misclib-getmetrics-capi test in C' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox