Re: [Tarantool-patches] [PATCH luajit 2/3] test: make utils.selfrun usage easier

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

Sergey,

Thanks for your review!

On 06.04.21, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the patch!
> 
> LGTM, except a few nits below.

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

> 
> Nit: I suggest to use `` for codenames mentions (if it is a function, then use
> brackets, for example `function_name()`), and <> for filenames mentions
> in the commit message and comments in the code.

I personally don't like this style: it looks ugly at first. This is used
neither for rendering commit messages (even on GitHub web interface) nor
for doc related postprocessing, so I see no motivation to write
something like this.

> This style has already been used for lua-Harness suite.

I would like ask you not to use this notation there, but we had no words
regarding it in our Tarantool style guide, so I couldn't stop you from
writing commit messages the way you want. Finally, we have no guide for
LuaJIT sources and patches for now.

> Thoughts?

It would be nice to come to agreement about using the similar style in
writing plain text messages, but definitely not such ugly one.

> 
> On 05.04.21, Igor Munkin wrote:
> > Previous implementation of <utils.selfrun> implicitly obliges the
> > developer to pass the arguments to the test script for the second run.
> > This has been unnoticed, since the existing tests are invoked for two
> > different cases, so the script arguments were kinda obligatory, but they
> > are not required in a general case.
> > 
> > As a result the function signals the testing system that the script is
> > being run the second time via TEST_SELFRUN environment variable.
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  .../gh-4427-ffi-sandwich.test.lua             | 51 +++++++++----------
> >  .../lj-flush-on-trace.test.lua                | 51 +++++++++----------
> >  test/tarantool-tests/utils.lua                |  7 +++
> >  3 files changed, 57 insertions(+), 52 deletions(-)
> > 
> > diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> > index 56363608..64df5dbd 100644
> > --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> > +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> > @@ -1,31 +1,30 @@
> 
> <skipped>
> 
> > +----- Test payload. ------------------------------------------------------------
> 
> Typo? comment length is too long, isn't it?

Well... I did it unintentionally: just typed 80a- in my text editor :)

> But, in other side, we already have the 80-symbols comments inside
> <lj_memprof.c>...
> But, we have no strict code style for LuaJIT part of the code. So:

Yes, we have not, but let's follow Tarantool guidelines for now. I left
the comment but stripped it up to 66 symbols in both chunks.

> Feel free to ignore.
> 
> >  
> >  local cfg = {
> >    hotloop = arg[1] or 1,

<snipped>

> > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> > index aebbf6ac..d2dd71b0 100644
> > --- a/test/tarantool-tests/utils.lua
> > +++ b/test/tarantool-tests/utils.lua
> > @@ -14,6 +14,12 @@ local function luacmd(args)
> >  end
> >  
> >  function M.selfrun(arg, checks)
> > +  -- If TEST_SELFRUN is set, just execute the test payload below
> > +  -- <selfrun> call, ...
> > +  if os.getenv('TEST_SELFRUN') then return end
> 
> Typo? should we avoid inline return, shouldn't we?

No, it's not. There is the similar usage in <utils.skipcond>, and I just
follow the style. BTW, there are comments around this block, so I hope
this is not a problem for the reader.

> 
> > +
> > +  -- ... otherwise run this chunk via <io.popen>.
> > +
> >    local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
> >  
> >    test:plan(#checks)
> > @@ -28,6 +34,7 @@ function M.selfrun(arg, checks)
> >    local cmd = string.gsub('LUA_PATH="<PATH>/?.lua;$LUA_PATH" ' ..
> >                            'LUA_CPATH="<PATH>/?.<SUFFIX>;$LUA_CPATH" ' ..
> >                            'LD_LIBRARY_PATH=<PATH>:$LD_LIBRARY_PATH ' ..
> > +                          'TEST_SELFRUN=1' ..
> >                            '<LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
> >  
> >    for _, ch in pairs(checks) do
> > -- 
> > 2.25.0
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM