Re: [Tarantool-patches] [PATCH luajit 1/5] test: introduce `samevalues()` TAP checker

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

Hi, Sergey!
Thanks for the review!
Please, consider my comments below.

On 18.08.23, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> 
> Thanks for the patch! See my comments inline.
> 
> 
> On 8/15/23 12:36, Sergey Kaplun wrote:
> > The introduced `samevalues()` helper checks that values in range from
> > 1, to `table.maxn()` of the given table are exactly the same. It may be
> > usefull for test consistency of JIT and VM behaviour. Originally, the
> > `arr_is_consistent()` function was introduced in the
> > <tarantool-tests/gh-6163-min-max.test.lua>. `samevalues()` has the same
> > functionallity (except usage of `table.maxn()` instead `#` operator to
> > be sure, that the table we check isn't a sparse array).
> 
> I would rename samevalues to something like assert_equals or 
> assert_items_equals just because
> 
> similar functions are named in unit testing frameworks and helpers with 
> prefix assert_

As you can see we use naming without _ for exported function in the
<tap.lua> module, so additional one with strange naming will be
inconsistent.

Also, discussed this naming with Igor and Max offline and this name is
OK for them, feel free also to CC Igor to discuss:).

> 
> more readable from my point of view. See names for assertions in luatest 
> [1] and JUnit (popular unit testing framework).
> 
> 
> 1. https://github.com/tarantool/luatest#list-of-luatest-functions
> 
> 2. 
> https://junit.org/junit5/docs/5.0.1/api/org/junit/jupiter/api/Assertions.html
> 
> > ---
> >   test/tarantool-tests/gh-6163-min-max.test.lua | 52 ++++++++-----------
> >   test/tarantool-tests/tap.lua                  | 14 +++++
> >   2 files changed, 37 insertions(+), 29 deletions(-)
> >
> > diff --git a/test/tarantool-tests/gh-6163-min-max.test.lua b/test/tarantool-tests/gh-6163-min-max.test.lua
> > index 63437955..4bc6155c 100644
> > --- a/test/tarantool-tests/gh-6163-min-max.test.lua
> > +++ b/test/tarantool-tests/gh-6163-min-max.test.lua
> > @@ -2,25 +2,17 @@ local tap = require('tap')
> >   local test = tap.test('gh-6163-jit-min-max'):skipcond({
> >     ['Test requires JIT enabled'] = not jit.status(),
> >   })
> > +
> Nit: Unneeded change.

The code with additional local variable `DUMMY_TABLE` becomes less
readable, so I added the empty line.
Ignoring.

> 
> <snipped>
> 
> 
> > diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
> > index 8559ee52..af1d4b20 100644
> > --- a/test/tarantool-tests/tap.lua
> > +++ b/test/tarantool-tests/tap.lua
> > @@ -254,6 +254,19 @@ local function iscdata(test, v, ctype, message, extra)
> >     return ok(test, ffi.istype(ctype, v), message, extra)
> >   end
> >   
> > +local function isnan(v)
> > +  return v ~= v
> > +end
> > +
> 
> I would put isnan() to utils, because it is not related to TAP module 
> actually and

Don't agree here, since <tap.lua> module should be self-sufficient (as
it is now), so it doesn't require additional modules like utils.

> 
> can be useful for other tests too and exporting it in tap module will be 

Opposite: it's OK to export it from this module within a check, since we
may want to check some value is NaN or not.

> strange.
> 
> 
> > +local function samevalues(test, got, message, extra)
> > +  for i = 1, table.maxn(got) - 1 do
> > +    if got[i] ~= got[i + 1] and not (isnan(got[i]) and isnan(got[i + 1])) then
> > +      return fail(test, message, extra)
> > +    end
> > +  end
> > +  return ok(test, true, message, extra)
> > +end
> > +
> >   local test_mt
> >   
> >   local function new(parent, name, fun, ...)
> > @@ -372,6 +385,7 @@ test_mt = {
> >       isudata    = isudata,
> >       iscdata    = iscdata,
> >       is_deeply  = is_deeply,
> > +    samevalues = samevalues,
> >       like       = like,
> >       unlike     = unlike,
> >     }

-- 
Best regards,
Sergey Kaplun