[Tarantool-patches] [PATCH luajit 3/5] test: run LuaJIT tests via CMake
Igor Munkin
imun at tarantool.org
Sun Feb 28 21:18:54 MSK 2021
Sergey,
Thanks for your review, added your tag:
| Reviewed-by: Sergey Kaplun <skaplun at 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
More information about the Tarantool-patches
mailing list