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