From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit 3/5] test: run LuaJIT tests via CMake Date: Sun, 28 Feb 2021 21:18:54 +0300 [thread overview] Message-ID: <20210228181854.GH9042@tarantool.org> (raw) In-Reply-To: <20210227135031.GC6842@root> Sergey, Thanks for your review, added your tag: | Reviewed-by: Sergey Kaplun <skaplun@tarantool.org> On 27.02.21, Sergey Kaplun wrote: > Igor, > <snipped> > > > Side note: (this is more like discussion question) -- do we really need > > > to use Tarantool's <tap.lua> module here? I provide some > > > dissatisfactions, but as I say below, we can't just change this > > > module API because we want (if you remove this module from Tarantool > > > and save it only here). > > > If it is saved untouchable in Tarantool repo I see no reason to > > > avoid replacing it with another one more suitable (if we want) or > > > improving it. Amount of tests is really small now and it can be easily > > > adapted. May be we should take a look at Lua test-modules like [1] and > > > [2]. > > > > I'm totally for lua-TestMore. We can try to port everything to its tap, > > when it has been incorporated to the trunk. > > OK, we may discuss this offline. For now, IINM, tap.lua inside Tarantool > is not the same as tap.lua inside LuaJIT. Please correct me if I'm > wrong. After the changes you requested -- yes, they are different. > > The fixes LGTM, but I still don't get why we don't create a separate > repository for the "third party" module tap.lua instead of managing 2 > versions of it manually. Or why we won't replace it here with > lua-TestMore just now to avoid to do our job twice. Moreover, probably > we will not have time to do this later, I afraid. The answer is quite simple. There was no a requirement to reimplement Tarantool tests in terms of the engine. There was a requirement to make them self-sufficient and break the tie described in the issue. Both you and Timur have already reviewed rather big changeset, hence while working on #4862 I tried to reduce the AOE. There are no external dependencies in LuaJIT for now and this is great. There are no alternative TAP engines in repo for now, unfortunately. What is more important: there is no other suites than the one originally moved from Tarantool and using its TAP module. It has been working quite well for more than a year[1] and I see no reason to reimplement testing using another external TAP. Besides, I remember we discussed this several times while talking about adopting other available suites. Yes, you mentioned one bug (that has not been pointed for many years... it means either nobody is using this assertion routine or nobody uses self-recursive tables in tests) and several minor things bothering you. I've fixed everything as you wanted and now you are asking about the sense of doing this... I have nothing to say here, but to checkout the patchset to the trunk if you don't mind. Anyway, I guess you can freely rewrite everything using another TAP engine in scope of lua-Harness issue[2]. Even if we don't want to make our fork depend on Test.More, there is a minimal TAP library used when Test.More is not available. Hope you'll like it more than Tarantool's. > > Also, I should remember, that if we want to support 2 versions of this > module in Tarantool and in LuaJIT, it *may* not work in the same way > when we run tests under Tarantool binary, as it works for the LuaJIT > binary (thanks for the manually preload modules). If you see any misbehaviour, please point this out. I see everything working fine right now. > > This is why I strongly disagree with its usage here instead > third-party dependency or other module usage. But it is very > subjectively. Feel free to ignore. I hope I've answered everything above. BTW, I personally against a single third-party dependency instead of a tiny module provided via self-sufficient Lua file in the trunk. Ignoring. > <snipped> > > > > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt > > > > new file mode 100644 > > > > index 0000000..0be4b34 > > > > --- /dev/null > > > > +++ b/test/tarantool-tests/CMakeLists.txt > > > > @@ -0,0 +1,92 @@ <snipped> > > > See nothing bad to add `--verbose` option here. It's better to see the > > > name of the single test, not the test file only. > > > > No. It spoils the output too much. I propose the following change: > > > > ================================================================================ > > > > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt > > index c7a4ccc..9c9dabf 100644 > > --- a/test/tarantool-tests/CMakeLists.txt > > +++ b/test/tarantool-tests/CMakeLists.txt > > @@ -68,8 +68,13 @@ set(LUA_PATH > > "${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;${PROJECT_SOURCE_DIR}/tools/?.lua" > > ) > > set(LUA_TEST_SUFFIX .test.lua) > > +set(LUA_TEST_FLAGS --failures --shuffle) > > file(GLOB TEST_DEPS ${CMAKE_CURRENT_SOURCE_DIR}/*${LUA_TEST_SUFFIX}) > > > > +if(CMAKE_VERBOSE_MAKEFILE) > > + list(APPEND LUA_TEST_FLAGS --verbose) > > +endif() > > + > > # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list > > # with dependecies are set in scope of BuildTestLib macro. > > add_custom_command( > > @@ -84,7 +89,7 @@ add_custom_command( > > ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR} > > --exec ${LUAJIT_TEST_BINARY} > > --ext ${LUA_TEST_SUFFIX} > > - --failures --shuffle > > + ${LUA_TEST_FLAGS} > > && touch tests.ok > > WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} > > ) > > > > ================================================================================ > > > > If you're OK with it, I'll apply these changes to the branch. > > Yep. Applied. > <snipped> > > > > diff --git a/test/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua > > > > similarity index 98% > > > > rename from test/misclib-getmetrics-lapi.test.lua > > > > rename to test/tarantool-tests/misclib-getmetrics-lapi.test.lua > > > > index 3b3d1f8..959293d 100755 > > > > --- a/test/misclib-getmetrics-lapi.test.lua > > > > +++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua > > > > <snipped> > > > > > > @@ -49,7 +52,10 @@ test:test("gc-allocated-freed", function(subtest) > > > > subtest:plan(1) > > > > > > > > -- Force up garbage collect all dead objects. > > > > > > Please, add comments about testing as a third party for the Tarantool > > > and GC finalizers to describe this change. > > > > Well, isn't everything described above? We are forcing GC to collect > > *all* dead objects. In other words until collectgarbage("count") return > > value changes. > > Yes, but it is not necessary at all for LuaJIT itself -- there are no > cdata finalizers above to be running so it looks like only one > `collectgarbage()` is enough. But this is not true for the Tarantool. OK, adjusted the comment. ================================================================================ diff --git a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua index a005781..fd253ae 100644 --- a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua +++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua @@ -49,7 +49,8 @@ end) test:test("gc-allocated-freed", function(subtest) subtest:plan(1) - -- Force up garbage collect all dead objects. + -- Force up garbage collect all dead objects (even those + -- resurrected or postponed for custom finalization). repeat local count = collectgarbage("count") collectgarbage("collect") ================================================================================ > <snipped> > > > > diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua > > > > > > Side note: firstly I thought that <tap.lua> will be clashing with tap > > > inside other suites, but these conflicts will be easily resolved by > > > LUA_PATH. > > > > > > > new file mode 100644 > > > > index 0000000..fd03132 > > > > --- /dev/null > > > > +++ b/test/tarantool-tests/tap.lua <snipped> > > > > + local trace = traceback() > > > > > > Nit: Looks like this chunk can be separated inside one function > > > `dump_traceback()` back or whatever. > > > Feel free to ignore. > > > > Don't get this nit. > > It's about refactoring. I suggest to move this chunk (dumped traceback) > inside the other (new) function and just call this function here. > In my opinion, it increases code readability. > Feel free to ignore. As you can see, everything related to io.* is placed within the following functions: * <new> wrapper * <plan> * <diag> * <ok> The most of io.* usage is enclosed within the latter one (unfortunately, there is no way to gather it a single spot), so if there is something bad with TAP format it's likely an error in <ok> routine and there is no need to seek for all other places. So no, IMHO, it does not increase code readability and further maintenance. Ignoring. > <snipped> > > Thanks for the fixes. Will we add corresponding patch to tap.lua inside > Tarantool? Feel free to open an issue against Tarantool TAP module. > <snipped> > > > > +local function isudata(test, v, utype, message, extra) > > > > > > Side note: Looks like it does not test type(v) equals "udata", that's > > > > Em, what?.. > > Why this chunk is not enough? > | return is(test, type(v), 'userdata', message, extra) > > And check utype by the other function. Because it is another helper inherited from Tarantool TAP module. If you need a new metatable-agnostic one, feel free to file an issue to the queue. > > > > > > inconsistent with other checkers. But changing it equals to break > > > backward compatibility. > > > > With what? BTW, it is already broken after the changes in <isnil>. > > If tap.lua inside Tarantool is not the same as tap.lua inside LuaJIT > this comment is irrelevant. Please correct me if I'm wrong. I've pointed above that <isnil> works in a different way as a result of the changes you've requested. > <snipped> > > > > > + local level = parent ~= nil and parent.level + 1 or 0 > > > > + local test = setmetatable({ > > > > + parent = parent, > > > > + name = name, > > > > + level = level, > > > > + total = 0, > > > > + failed = 0, > > > > + planned = 0, > > > > + trace = parent == nil and true or parent.trace, > > > > + strict = false, > > > > > > What does strict mean and how it can be changed? > > > > Em, in a quite simple way? > > | test.strict = true > > I confused with mixing kinda "private" and "public" fields. > It's OK to change `test.strict`, but it is bad idea to change > `test.failed` spontaneously. Selectio naturalis: do not change the value that is not meant to be changed. Otherwise, you're "sam sebe zlobniy buratino". > > > > > > I see nothing bad to copy some rationale from documentation [5] here > > > as comments for each field. > > > > I found no rationale there, could you please specify the places you want > > to clarify in a more accurate way? > > I tell about the following description: > > | Set `taptest.strict=true` if `taptest:is()` and `taptest:isnt()` and > | `taptest:is_deeply()` must be compared strictly with `nil`. Set > | `taptest.strict=false` if `nil` and `box.NULL` both have the same > | effect. The default is `false`. For example, if and only if > | `taptest.strict=true` has happened, then > | `taptest:is_deeply({a = box.NULL}, {})` will return `false`. > OK, added the comment. ================================================================================ diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua index 134b501..acbf5f1 100644 --- a/test/tarantool-tests/tap.lua +++ b/test/tarantool-tests/tap.lua @@ -248,6 +248,13 @@ local function new(parent, name, fun, ...) failed = 0, planned = 0, trace = parent == nil and true or parent.trace, + -- Set test.strict = true if test:is, test:isnt and + -- test:is_deeply must be compared strictly with nil. + -- Set test.strict = false if nil and box.NULL both have the + -- same effect. The default is false. For example, if and only + -- if test.strict = true has happened, then the following + -- assertion will return false. + -- test:is_deeply({a = box.NULL}, {}) strict = false, }, test_mt) if fun == nil then ================================================================================ > > > > > > > > > > + }, test_mt) > > > > + if fun == nil then > > > > + return test > > > > + end > > > > + test:diag('%s', test.name) > > > > + fun(test, ...) > > > > + test:diag('%s: end', test.name) > > > > + return test:check() > > > > +end > > > > <snipped> > > > > > > +local function check(test) > > > > + if test.checked then > > > > + error('check called twice') > > > > > > Nit: eror_level = 2 is better here in my opinion. > > > Feel free to ignore. > > > > Unfortunately, nothing's changed: > > The first line is changed. Now I see, thanks! Applied the changes. > <snipped> > > > > > -- > > > > 2.25.0 > > > > > > > > > > [1]: https://luaunit.readthedocs.io/en/luaunit_v3_2_1/ > > > [2]: https://fperrad.frama.io/lua-TestMore/ > > > [3]: https://metacpan.org/pod/Test::Deep > > > [4]: https://www.tarantool.io/en/doc/latest/reference/reference_lua/tap/#taptest-is > > > [5]: https://www.tarantool.io/en/doc/latest/reference/reference_lua/tap/ > > > [6]: https://perldoc.perl.org/Test::More#done_testing > > > > > > -- > > > Best regards, > > > Sergey Kaplun > > > > > > > -- > > Best regards, > > IM > > -- > Best regards, > Sergey Kaplun [1]: https://github.com/tarantool/luajit/commit/8343862 [2]: https://github.com/tarantool/tarantool/issues/5844 -- Best regards, IM
next prev parent reply other threads:[~2021-02-28 18:19 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-02 20:57 [Tarantool-patches] [PATCH luajit 0/5] Self-sufficient LuaJIT testing environment Igor Munkin via Tarantool-patches 2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 1/5] build: preserve the original build system Igor Munkin via Tarantool-patches 2021-02-04 22:53 ` Timur Safin via Tarantool-patches 2021-02-08 15:56 ` Igor Munkin via Tarantool-patches 2021-02-09 11:38 ` Sergey Kaplun via Tarantool-patches 2021-02-09 12:47 ` Igor Munkin via Tarantool-patches 2021-02-09 14:45 ` Sergey Kaplun via Tarantool-patches 2021-02-09 15:28 ` Igor Munkin via Tarantool-patches 2021-02-10 9:35 ` Sergey Kaplun via Tarantool-patches 2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake Igor Munkin via Tarantool-patches 2021-02-04 22:53 ` Timur Safin via Tarantool-patches 2021-02-08 15:56 ` Igor Munkin via Tarantool-patches 2021-02-09 13:55 ` Timur Safin via Tarantool-patches 2021-02-09 15:09 ` Igor Munkin via Tarantool-patches 2021-02-11 19:23 ` Sergey Kaplun via Tarantool-patches 2021-02-16 15:28 ` Igor Munkin via Tarantool-patches 2021-02-18 9:56 ` Sergey Kaplun via Tarantool-patches 2021-02-20 19:18 ` Igor Munkin via Tarantool-patches 2021-02-27 10:48 ` Sergey Kaplun via Tarantool-patches 2021-02-28 18:18 ` Igor Munkin via Tarantool-patches 2021-02-13 3:47 ` Sergey Kaplun via Tarantool-patches 2021-02-16 15:32 ` Igor Munkin via Tarantool-patches 2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 3/5] test: run LuaJIT tests via CMake Igor Munkin via Tarantool-patches 2021-02-08 15:05 ` Timur Safin via Tarantool-patches 2021-02-08 16:29 ` Igor Munkin via Tarantool-patches 2021-02-09 8:16 ` Timur Safin via Tarantool-patches 2021-02-09 8:43 ` Igor Munkin via Tarantool-patches 2021-02-09 13:59 ` Timur Safin via Tarantool-patches 2021-02-09 15:10 ` Igor Munkin via Tarantool-patches 2021-02-14 18:48 ` Sergey Kaplun via Tarantool-patches 2021-02-19 19:04 ` Igor Munkin via Tarantool-patches 2021-02-27 13:50 ` Sergey Kaplun via Tarantool-patches 2021-02-28 18:18 ` Igor Munkin via Tarantool-patches [this message] 2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 4/5] test: fix warnings found with luacheck in misclib* Igor Munkin via Tarantool-patches [not found] ` <012f01d6fe1a$a2aa6890$e7ff39b0$@tarantool.org> [not found] ` <2c495492-50f4-acfd-ad66-2cb44abb5fa1@tarantool.org> 2021-02-08 15:40 ` Sergey Bronnikov via Tarantool-patches 2021-02-08 15:58 ` Igor Munkin via Tarantool-patches 2021-02-08 15:57 ` Igor Munkin via Tarantool-patches 2021-02-14 19:16 ` Sergey Kaplun via Tarantool-patches 2021-02-16 15:29 ` Igor Munkin via Tarantool-patches 2021-02-16 16:36 ` Sergey Kaplun via Tarantool-patches 2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 5/5] test: run luacheck static analysis via CMake Igor Munkin via Tarantool-patches 2021-02-04 22:52 ` Timur Safin via Tarantool-patches 2021-02-08 15:57 ` Igor Munkin via Tarantool-patches 2021-02-14 19:32 ` Sergey Kaplun via Tarantool-patches 2021-02-19 19:14 ` Igor Munkin via Tarantool-patches 2021-02-28 22:04 ` [Tarantool-patches] [PATCH luajit 0/5] Self-sufficient LuaJIT testing environment Igor Munkin 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=20210228181854.GH9042@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit 3/5] test: run LuaJIT tests via CMake' \ /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