Re: [Tarantool-patches] [PATCH luajit 1/4] test: add descriptions to sysprof testcases

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

Hi, Sergey!
Thanks for the patch!
LGTM, with a bunch of nits below.

On 04.02.25, Sergey Bronnikov wrote:
> The patch add descriptions to sysprof testcases to make TAP report

Typo: s/add/adds/
Typo: s/TAP/the TAP/

> more usable.
> ---
>  .../profilers/misclib-sysprof-lapi.test.lua   | 36 ++++++++++---------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> index b2c38c49..581fb7fd 100644
> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> @@ -65,34 +65,35 @@ end
>  
>  -- Wrong profiling mode.
>  local res, err, errno = misc.sysprof.start{ mode = "A" }
> -test:ok(res == nil and err:match("profiler misuse"))
> -test:ok(type(errno) == "number")
> +test:ok(res == nil and err:match("profiler misuse"), "res with no parameters")

Minor: s/res/result status/
Here and below.
Feel free to ignore.

It is not the "no parameters", but rather the wrong profiler mode.

> +test:ok(type(errno) == "number", "errno with no parameters")

It is not the "no parameters", but rather the wrong profiler mode.

>  
>  -- Already running.

<snipped>

>  -- Not running.

<snipped>

>  -- Bad path.
>  res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH })
> -test:ok(res == nil and err:match("No such file or directory"))
> -test:ok(type(errno) == "number")
> +test:ok(res == nil and err:match("No such file or directory"), "res and error with bad path")

Nit: Line length is more than 80 symbols.

> +test:ok(type(errno) == "number", "errno with bad path")
>  
>  -- Bad interval.
>  res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
> -test:ok(res == nil and err:match("profiler misuse"))
> -test:ok(type(errno) == "number")
> +test:ok(res == nil and err:match("profiler misuse"), "res and error with bad interval")

Nit: Line length is more than 80 symbols.

> +test:ok(type(errno) == "number", "errno with bad interval")
>  
>  -- DEFAULT MODE
>  
> @@ -102,20 +103,23 @@ end
>  
>  local report = misc.sysprof.report()
>  
> --- Check the profile is not empty
> -test:ok(report.samples > 0)
> +-- Check the profile is not empty.
> +test:ok(report.samples > 0, "number of samples is greater than 0")

I would also add the following:
"... than 0 for the default payload"

>  -- There is a Lua function with FNEW bytecode in it. Hence there
>  -- are only three possible sample types:
>  -- * LFUNC -- Lua payload is sampled.
>  -- * GC -- Lua GC machinery triggered in scope of FNEW bytecode
>  --   is sampled.
>  -- * INTERP -- VM is in a specific state when the sample is taken.
> -test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0)
> +test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0,
> +        "total number of LFUNC, GC and INTERP samples is greater than 0")

I would also add the following:
"... than 0 for the default payload"

>  -- There is no fast functions and C function in default payload.
> -test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0)
> +test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0,
> +        "total number of FFUNC and CFUNC samples is equal to 0")

I would also add the following:
"... to 0 for the default payload"

>  -- Check all JIT-related VM states are not sampled.
>  for _, vmstate in pairs({ 'TRACE', 'RECORD', 'OPT', 'ASM', 'EXIT' }) do
> -  test:ok(report.vmstate[vmstate] == 0)
> +  local msg = ("total number of VM state %s is equal to 0"):format(vmstate)

I would also add the following:
"... to 0 for the default payload"

Minor: I would avoid the tmp variable here, since it is used only once
anyway.

> +  test:ok(report.vmstate[vmstate] == 0, msg)
>  end
>  
>  -- With very big interval.
> @@ -124,7 +128,7 @@ if not pcall(generate_output, { mode = "D", interval = 1000 }) then
>  end
>  
>  report = misc.sysprof.report()
> -test:ok(report.samples == 0)
> +test:ok(report.samples == 0, "total number of samples is equal to 0")

I would also add the following:
"... to 0 for the too big sampling interval"

>  
>  -- LEAF MODE
>  check_mode("L", 100)
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun