Re: [Tarantool-patches] [PATCH luajit 1/3] test: add PUC-Rio Lua 5.1 test suite

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

Sergos,

Thanks for you review!

On 16.03.21, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch!
> 
> 
> 
> > On 12 Mar 2021, at 08:36, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > This patch adds PUC-Rio Lua 5.1 test suite as a part of the LuaJIT
> > test suite. Source code taken verbatim from
> > https://www.lua.org/tests/lua5.1-tests.tar.gz.
> > 
> > <lib1.c> and <lib2.c> is slightly modified to be consistent with the
> > current LuaJIT's LuaC API.
> > 
> 
> This should be a different commit, isn’t it?

Yes, it does.

> 
> > Some tests may fail after this commit. They will be disabled
> > or adapted in the next patches.
> 
> The fact patch causes - even temporarily - some fails is no good. The
> only exception is initial commit, that brings original test suite.
> Can you please make a consistent change that will left the tests disabled
> until they adapted and enabled back?

I believe such juggling with enabling/disabling is quite unprofitable.
We iteratively fix broken tests within the series making CI green as a
result. Even if lib[12].c fails to compile -- it is not a problem, since
these libraries are test dependencies, not build ones. Sergey can simply
mention the fact the build of suite-related tests is broken, so one can
find the comment in commit message while e.g. bisecting the bug.
Otherwise, the patches are spoiled with unnecessary changes.

> 
> regards,
> Sergos
> 
> > 

<snipped>

> > -- 
> > 2.28.0
> > 
> 

-- 
Best regards,
IM