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

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

[-- Attachment #1: Type: text/plain, Size: 4677 bytes --]

Hi, Sergey,

thanks for review!

On 10.02.2025 17:51, Sergey Kaplun via Tarantool-patches wrote:
> 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/

Fixed both typos.


>> 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/
Fixed.
> Here and below.
> Feel free to ignore.
>
> It is not the "no parameters", but rather the wrong profiler mode.
Fixed.
>
>> +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.
Fixed and sent a separate patch that limits max len by 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.
Fixed.
>
>> +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"
Fixed.
>
>>   -- 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"
Fixed.
>>   -- 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"
Fixed.
>
>>   -- 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.
Fixed and moved out of loop.
>> +  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"

Fixed.


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

[-- Attachment #2: Type: text/html, Size: 8162 bytes --]