Re: [Tarantool-patches] [PATCH luajit 3/5] test: run LuaJIT tests via CMake

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

Igor,

On 19.02.21, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch!
> 
> On 14.02.21, Sergey Kaplun wrote:
> > Hi, 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.

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.

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

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.

> 
> > 

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

> > > +# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
> > > +# with dependecies are set in scope of BuildTestLib macro.
> > > +add_custom_command(
> > > +  COMMENT "Running Tarantool tests"
> > > +  OUTPUT tests.ok
> > > +  DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS} ${TEST_DEPS}
> > > +  COMMAND
> > > +  env
> > > +    LUA_PATH="${LUA_PATH}\;\;"
> > > +    LUA_CPATH="${LUA_CPATH}\;\;"
> > > +    LD_LIBRARY_PATH="${LD_LIBRARY_PATH}"
> > > +    ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR}
> > > +      --exec ${LUAJIT_TEST_BINARY}
> > > +      --ext ${LUA_TEST_SUFFIX}
> > > +      --failures --shuffle
> > 
> > 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.

> 

<snipped>

> > >          1, -- hotloop (arg[1])
> > >          2, -- trigger (arg[2])
> > >        },
> > > -      res = 'Lua VM re-entrancy is detected while executing the trace',
> > > -      msg = 'Trace is recorded',
> > > +      test = 'like',
> > 
> > What does it mean?
> 
> The type of test used for verification: either pattern matching ('like')
> or exact match ('is').
> 
> > 
> > > +      res  = 'Lua VM re%-entrancy is detected while executing the trace',
> > 
> > This % is redundant.
> 
> Any proof for it?

Sorry, my bad. But very unobvious that it's a regex result (for the
first look).

> 
> > 
> > > +      msg  = 'Trace is recorded',
> > >      },
> > >    })
> > >  end
> 
> <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.

> 
> > 
> > > -    collectgarbage("collect")
> > > +    repeat
> > > +        local count = collectgarbage("count")
> > > +        collectgarbage("collect")
> > > +    until collectgarbage("count") == count
> > >  
> > >      -- Bump getmetrics table and string keys allocation.
> > >      local old_metrics = misc.getmetrics()
> 
> <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
> > 
> > Feel free to stop me from refactoring anytime, but I try to dump all my
> > thoughts about this module now to be referred later.
> > 
> > Side note: It's a pity that there is no such module as Perl's Test::Deep [3]
> > in Lua (or at least I haven't seen one).

<snipped>

> > > +local ffi = require('ffi')
> > > +local NULL = ffi.new('void *')
> > 
> > Nit: What if LuaJIT is built without FFI?
> 
> Lol, these tests do not respect such configuration. I have the following
> change to support testing with FFI disabled, but most of the tests are
> simply skipped.
> 
> ================================================================================

<snipped>

> ================================================================================
> 
> BTW, nothing works also if LuaJIT is build with JIT support disabled.
> Furthermore, I believe LuaJIT suite also doesn't fit to this particular
> configuration. I propose to adjust the tests in scope of another issue,
> if you want.

Yes, sure. Though, I doubt that it will be done soon :).

> 

<snipped>

> > 
> > > +    return false
> > > +  end
> > > +
> > > +  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.

> 
> > 
> > > +  local tindent = indent(4 + 4 * test.level)
> > > +  io.write(tindent, ('filename:\t%s\n'):format(trace[#trace].filename))
> > > +  io.write(tindent, ('line:\t%s\n'):format(trace[#trace].line))
> > > +  for frameno, frame in ipairs(trace) do
> > > +    io.write(tindent, ('frame #%d\n'):format(frameno))
> > 

<snipped>

> > > +    visited_keys[i] = true
> 
> I believe this doesn't handle your case. This variable is introduced for
> checking whether there are extra keys in <expected> table.
> 
> > 
> > This is not good enough, visited keys should be cashed better:
> > 
> > | tarantool> local test = tap.test("<T>") local t1 = {1, 2, 3} t1[4] = {t1} local t2 = {1, 2, 3} t2[4] = {t2} return test:is_deeply(t1, t2), t1, t2
> > | TAP version 13
> > | ---
> > | - error: stack overflow
> 
> Anyway, nice catch, thanks! Fixed this, diff is below:
> 
> ================================================================================

<snipped>

> ================================================================================

Thanks for the fixes. Will we add corresponding patch to tap.lua inside
Tarantool?

> 
> > 
> > > +    extra.path = path .. '/' .. i
> > > +    if not cmpdeeply(v, expected[i], extra) then
> > > +      return false
> > > +    end
> > > +  end
> > > +
> > > +  -- Check if expected contains more keys then got.
> > > +  for i, v in pairs(expected) do
> 
> <snipped>
> 
> > > +local function is(test, got, expected, message, extra)
> > > +  extra = extra or { }
> > > +  extra.got = got
> > > +  extra.expected = expected
> > > +  local rc = (test.strict == false or type(got) == type(expected))
> > 
> > Nit: It is nice to leave the comment here to provide description why
> > just `==` is not enough. Also, I don't like this behaviour -- it
> > disrespects `eq` metamethod, and there is not mentioned in the doc [4].
> 
> In what sense this "disrespects" eq metamethod? Unfortunately, these
> docs are quite bad...

Sorry, I meant that `got == expected` is enough. But I forgot about FFI
semantics, FFI cast, and that with FFI "different types" are not enough
to say that objects are different. Just ignore this comment.

> 
> > 
> > > +             and got == expected
> > > +  return ok(test, rc, message, extra)
> > > +end
> > > +
> > > +local function isnt(test, got, unexpected, message, extra)
> > > +  extra = extra or { }
> > > +  extra.got = got
> > > +  extra.unexpected = unexpected
> > > +  local rc = (test.strict == true and type(got) ~= type(unexpected))
> > 
> > Ditto.
> > 
> > > +             or got ~= unexpected

<snipped>

> > > +local function isnumber(test, v, message, extra)
> > > +  return is(test, type(v), 'number', message, extra)
> > > +end
> > > +
> > > +local function isstring(test, v, message, extra)
> > > +  return is(test, type(v), 'string', message, extra)
> > > +end
> > > +
> > > +local function istable(test, v, message, extra)
> > > +  return is(test, type(v), 'table', message, extra)
> > > +end
> > > +
> > > +local function isboolean(test, v, message, extra)
> > > +  return is(test, type(v), 'boolean', message, extra)
> > > +end
> > > +
> > > +local function isfunction(test, v, message, extra)
> > > +  return is(test, type(v), 'function', message, extra)
> > > +end
> > > +
> > > +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.

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

> 
> > 
> > > +  extra = extra or { }
> > > +  extra.expected = ('userdata<%s>'):format(utype)
> > > +  if type(v) ~= 'userdata' then
> 
> ... Here it is.
> 
> > > +    extra.got = type(v)
> > > +    return fail(test, message, extra)
> > > +  end
> > > +  extra.got = ('userdata<%s>'):format(getmetatable(v))
> > > +  return ok(test, getmetatable(v) == utype, message, extra)
> > 
> > Does it assume that compared udata-s should have the same metatable?
> 
> Otherwise there is nothing else to compare. BTW, I strongly doubt this
> is rather useful.

Me too!

> 
> > 
> > > +end
> > > +
> > > +local function iscdata(test, v, ctype, message, extra)
> > 
> > Ditto about inconsistency.
> 
> Still don't get what inconsistency you're talking about.

See the explanation above.

> 

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

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


> 
> > 
> > > +  }, 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.

> 
> ================================================================================
> 
> $ luajit t.lua
> luajit: ./tap.lua:261: check called twice
          ^^^^^^^^^^^^^
Before.

> stack traceback:
> 	[C]: in function 'error'
> 	./tap.lua:261: in function 'check'
> 	t.lua:9: in main chunk
> 	[C]: at 0x5563639c4eb0
> TAP version 13
> 1..1
> ok - nil
> $ git stash pop
> <snipped>
> $ git diff
> diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
> index e2149a7..575e7bd 100644
> --- a/test/tarantool-tests/tap.lua
> +++ b/test/tarantool-tests/tap.lua
> @@ -258,7 +258,7 @@ end
>  
>  local function check(test)
>    if test.checked then
> -    error('check called twice')
> +    error('check called twice', 2)
>    end
>    test.checked = true
>    if test.planned ~= test.total then
> $ luajit t.lua
> luajit: t.lua:9: check called twice
          ^^^^^^^
After.

> stack traceback:
> 	[C]: in function 'error'
> 	./tap.lua:261: in function 'check'
> 	t.lua:9: in main chunk
> 	[C]: at 0x55cea3cfceb0
> TAP version 13
> 1..1
> ok - nil
> 
> ================================================================================
> 
> Please correct me if I've done something wrong. Ignoring for now.

All's good.

> 
> > 

<snipped>

> > > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> > > new file mode 100644
> > > index 0000000..197b138
> > > --- /dev/null
> > > +++ b/test/tarantool-tests/utils.lua
> > > @@ -0,0 +1,43 @@

<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