Re: [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5071 bytes --]


Hi!
Thanks for the comments.
 
> 
>>The whole idea of the patch-set introduce module for LuaJIT C tests. It
>>also, can be used for unit tests.
>>* The first patch is the prerequisite for the patch-set. It fixes
>>  LD_LIBRARY_PATH definition.
>>* The 2nd and 3d patches provides an API and helper for writing the tests.
>>* The last 3 patches rewrite existing tests that should be written in C in
>>  the proper way.
>>
>>Branch:  https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-tarantool-c-tests
>>PR:  https://github.com/tarantool/tarantool/pull/8444
>>Related Issue:
>>*  https://github.com/tarantool/tarantool/issues/7900
>>*  https://github.com/tarantool/tarantool/issues/781
>>
>>Thanks Maxim, for the review!
>>
>>I've fixed some Maxim comments and suggestions for the previous series.
>>Some ignorable comments about wording are ignored:).
>>
>>Also, see answers for your questions below:
>>
>>> >+if(NOT PROVE)
>>> >+ message(WARNING "`prove' is not found, so tarantool-c-tests target is not generated")
>>> >+ return()
>>> >+endif()
>>> There is the same check in the test/tarantool-tests/CMakeLists.txt. Maybe
>>> we should move it to the higher-level CMake so the lower-level CMakeLists
>>> inherit it.
>>
>>I agree it maybe done, but not within this particular patchsett, so
>>ignoring for now.
>Don’t see any reason to postpone it, tbh. New module requires the same
>checks as the already present one. It seems logical to do necessary changes
>in this patchset.
>>
>>> >+#define test_run_group(t_arr, t_state) \
>>> >+ _test_run_group(__func__, t_arr, lengthof(t_arr), t_state)
>>> Is there any reason for it to be a macro and not a function wrapper?
>>> I believe it is better to use the functions when possible, since they are
>>> easier to support and debug.
>>
>>Just for the convenience in usage of __func__ macro as a test group name.
>Oh, I see, thx.
>>
>>> >+/* Need for `strchr()` in diagnostic parsing. */
>>> `strchr()` is not safe, despite the fact it searches till `\0`.
>>> We should at least replace it with `memchr()`, which has
>>> the explicit constraint for buffer length.
>>> >+#include <string.h>
>>
>>Yes, but:
>>1) We use it only for our test code, where we set this `\0` directly to
>>mark EOL.
>>2) It's simplier than use several marks in buffer.
>>So, ignoring for now.
>Ok
>>
>>> >+# vim: ft=cmake expandtab shiftwidth=2: tabstop=2:
>>> That change is not necessary.
>>
>>Yes, but more convenient to use in vim -- since our usual codestyle isn't 4
>>tabs as its default for CMake. :)
>>Still we don't use it anywhere (unfortunately), so removed.
>>
>>> >+ bail_out("failed to translate Lua code snippet");
>>> Why `bail_out` and not an assertion? Here and below.
>>
>>Assertion is for some thing we wnat to test and may fail.
>>Bail out usage is more specific:
>>| As an emergency measure a test script can decide that further tests are
>>| useless (e.g. missing dependencies) and testing should stop immediately.
>>| In that case the test script prints the magic words
>>See [1]. I think that loading Lua script helper (dependency) is
>>something like that.
>Thanks for the clarification.
>>
>>> >+#include "lj_arch.h"
>>> Side note: I don't like the approach with private headers, but
>>> I couldn't find any better way to check that. Maybe it is a good
>>> idea to implement a public C API function to get the information
>>> about OS and ARCH, since it is a really common to check them?
>>
>>I think, that this is the best option, espessialy if we want to write
>>some unit test for some specific module (I mean <src/lj_*>).
>>
>>Changes in v2:
>>1) use
>>
>>| int _test_run_group(const char *group_name, const struct test_unit tests[],
>>| size_t n_tests, void *test_state);
>>
>>instead of
>>
>>| int _test_run_group(const char *group_name, const struct test_unit *tests,
>>| size_t n_tests, void *test_state);
>>
>>2) `skip()` `skip_all()` and `todo()` helpers now return values to be
>>return to runner.
>>i.e. change usage from
>>| if (cond)
>>| skip("NIY");
>>to
>>| if (cond)
>>| return skip("NIY");
>>
>>`bail_out()` helper still just exits with error code, which corresponding
>>its standard specification.
>>
>>But now some parts of the code start to look "alya cringe":
>>| return todo("Need to replace backtrace with libunwind first");
>>| lua_State *L = test_state;
>>| utils_get_aux_lfunc(L);
>>| (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_OFF);
>>| (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_FLUSH);
>>| check_profile_func(L);
>>| (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_ON);
>>| return TEST_EXIT_SUCCESS;
>Well, what is cringe here? There are a few unreachable lines, but now it is
>obvious that those are skipped.
>>
>>(Yes, we want to use unconditional `todo()`).
>>So I commented the similar code, helper `check_profile_func()`, etc.
>>with `#if 0`.
>Do we really need to that though? Again, it is clearly visible that those are
>unreachable. Comment in `todo` is sufficient.
>
><snipped>
>--
>Best regards,
>Maxim Kokryashkin
> 

[-- Attachment #2: Type: text/html, Size: 7056 bytes --]