Re: [Tarantool-patches] [WIP luajit 00/15] Adapt LuaVela test suites

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergos,

On 10.03.21, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch!
> 
> > On 10 Mar 2021, at 02:59, Igor Munkin <imun@tarantool.org> wrote:
> > 
> > Sergey,
> > 
> > I've precisely reviewed patches 04-09 and they look good. I've polished
> > a bit[1] the series, so it can be applied to the trunk out of the order.
> > CI[2] is green.
> > 
> > BTW, I would like to leave some major points related to my review.
> > 
> > 1. The suite has been taken intact. To check this I used the following
> > recipe:
> > | $ pwd
> > | /LuaJIT-test-cleanup/test
> > | $ git remote -v
> > | origin  https://github.com/LuaJIT/LuaJIT-test-cleanup.git (fetch)
> > | origin  https://github.com/LuaJIT/LuaJIT-test-cleanup.git (push)
> > | $ git lo -1
> > | 014708b (HEAD -> master, origin/master, origin/HEAD) Add test for BC_VARG slot revival
> > | $ find . -type f | sort | xargs md5sum > ~/vanilla
> 
> Why did you use sort here? I recall using sort -u to remove duplicates, but it is not…

For the same reason people run tests several times in a row: to be
*more* sure I do everything right... If find yields the same order every
time, sort can be omitted.

> 
> > | <...>
> > | $ pwd
> > | /tarantool-luajit/test/LuaJIT-test
> > | $ git remote -v
> > | origin  git@github.com:tarantool/luajit (fetch)
> > | origin  git@github.com:tarantool/luajit (push)
> > | $ git lo -1
> > | cc0967c (HEAD) test: add LuaJIT-test-cleanup test suite
> > | $ find . -type f | sort | xargs md5sum > ~/tarantool
> > | $ diff ~/vanilla ~/tarantool
> > | > 91cb30f369b83d7626a8ae852781ebb5  ./CMakeLists.txt
> > 
> > 2. I've introduced a new CMake option: LUAJIT_TEST_INIT. You can read
> > the description here[3]. It allows to enclose the partial LUA_INIT
> > emulation, implemented via '-e dofile[[${LUAJIT_TEST_INIT}]]' and
> > obliges user to set the name of the Lua script to be run prior to the
> > testing suite[4]. By default it does nothing by reading from /dev/null.
> > 
> I would like to complain that you’re open extra file each test run,
> but there are only 500 tests and AFAIU it is a temporary solution. 
> What is the plan for the final one?

Well... There is nothing more permanent than temporary solution, isn't
it?. In "nop" case these manipulations can be omitted in CMake, *but*
this doesn't make the situation better for Tarantool CI. So, let's see
if there are issues with such extra activity and measure its impact on
Tarantool testing and then decide whether LuaJIT testing machinery need
to be boosted.

Anyway, here are some numbers for the test target (`cmake . && make -j`
is already finished before measures):
* HEAD^ (with no extra files)
| $ time make -j test
| real    0m0.842s
| user    0m1.018s
| sys     0m0.255s
* HEAD (with /dev/null access)
| $ time make -j test
| real    0m0.844s
| user    0m1.021s
| sys     0m0.235s

> 
> > 3. Since CLI flags can be used for LuaJIT testing now, <utils.selfrun>
> > need to be adjusted[5] to respect the passed flags in child process.
> > 
> > This part LGTM, so if nobody is against the version I've checked to the
> > branch[1], then I apply it to the trunk after your and Sergos approval.
> > 
> 
> LGTM.

Added your tags to all patches:
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>

> Sergos
> 
> 

<snipped>

> > 
> > [1]: https://github.com/tarantool/luajit/tree/imun/tarantool-test
> > [2]: https://github.com/tarantool/tarantool/tree/imun/luajit-test
> > [3]: https://github.com/tarantool/luajit/commit/9712cc9#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR291-R307
> > [4]: https://github.com/tarantool/tarantool/commit/03b04a7
> > [5]: https://github.com/tarantool/luajit/commit/72f87b2
> > 
> > -- 
> > Best regards,
> > IM
> 

-- 
Best regards,
IM