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

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

Hi, Maxim!
Thanks for the reply!

On 19.05.23, Maxim Kokryashkin wrote:
> 
> 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.

There are two testing suite (except the new one), that use this
approach:
* tarantool-tests
* lua-Harness-tests
So, for this patchset I just use the same beaten path.
Also, we may try to build tests-target (suite) independently, and in
this case the check should be in low-level CMake.

> >>

<snipped>

> >>
> >>> >+#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, unreachable lines looks very strange to me.

> >>
> >>(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.

I'm not sure about that, so I'll wait for the 2nd reviewer opinion.

> >
> ><snipped>
> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun