Re: [Tarantool-patches] [PATCH v2 luajit 03/30] test: adapt Lua 5.1 suite for out-of-source build

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

Sergey,

On 01.04.21, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the review!
> 
> On 01.04.21, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for the patch! Honestly, I'm not fond of the solution you
> > implemented, though it allows to make as little changes as possible.
> > I propose to implement something similar to the solution you've made
> > within lua-Harness series:
> > * You need two <dofile> functions: one for launching source files for
> >   additional checks and another one for running temporary Lua chunks.
> > * You also need two <loadstring> functions for the same reason.
> > 
> > Mind the fact <dofile> is implemented via <loadstring>, so basically,
> > there might be no so much changes made for <dofile>. Thoughts?
> 
> I prefer to use one function, and two wrappers around it.
> For example, let it be `absolute_dofile()` and `cwd_dofile()`.

Why do you need the second one?

> 
> But honestly, it is not the single one problem of this suite (no test
> isolation, global variables, huge excess output, hardcoded limit
> values, etc).

Yes, the whole suite is a single problem, but you've already fixed
something. This patch can fix the base problem we've resolved while
adopting lua-Harness. Why do you want to omit it here?

> It looks like we need to refactor this suite later (plus adapt test
> for LuaJIT extensions from Lua 5.2+). I propose to do all this work
> via that refactoring, not now. Otherwise, it looks like a patch, which
> will still need to be refactored later.

It's not about refactoring, it's about the general approach. François
proposed an elegant solution with <_dofile> that can be overloaded in
luajit-test-init.lua (and we have to introduce one into LuaJIT then), so
I see no reason to postpone such simple change. Of course this issue can
be done within #5790 (or not, again, since it relates to another suite),
so if you don't want to do this, let's close the eyes and leave such
unsighty workaround.

Consider you have my LGTM here if nobody else is bothered. I'll just
glance the patch once more.

> 
> Thoughts?
> 
> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM