[Tarantool-patches] [PATCH 2/2] test: add test for tarantool -e assert(false)

[Artem]

Hi, thanks for the review!

See my 5 comments below.

10.12.2020 21:11, Leonid Vasiliev пишет:
> Hi! Thank you for the patch.
>
> The code is fine but I have some questions:
>
> On 09.12.2020 22:28, Artem Starshov wrote:
>> Closes #4983
>
> AFAIK test usually added to the same commit with fixes. If there is no
> reason to send the test in separate commit. In this case, the patch is
> easier to backport.
> If you are adding a test by a separate commit, mark it "Follows up".
>
1. > the patch is easier to backport.

Agree with this statement. There will be patch v2 with one commit.

>> ---
>>   .../gh-4983-tnt-e-assert-false-hangs.test.lua | 99 +++++++++++++++++++
>>   1 file changed, 99 insertions(+)
>>   create mode 100755 
>> test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua
>>
>> diff --git a/test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua 
>> b/test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua
>> new file mode 100755
>> index 000000000..2213b1363
>> --- /dev/null
>> +++ b/test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua
>> @@ -0,0 +1,99 @@
>> +#!/usr/bin/env tarantool
>> +
>> +local fiber = require('fiber')
>> +local clock = require('clock')
>> +local ffi = require('ffi')
>> +local fio = require('fio')
>> +local errno = require('errno')
>> +local tap = require('tap')
>
> Please, add a blank line to separate `require...` from code.
2. Done.
>
>> +local test = tap.test('gh-4983-tnt-e-assert-false-hangs')
>> +
>> +--
>> +-- gh-4983: tarantool -e 'assert(false)' hangs
>> +--
>> +
>> +--
>> +-- For process_is_alive.
>> +--
>
> What style guide did you get this version of comments from?
> (Here and below)
> -- 
> -- comment
> -- 

3. I took it from popen.test.lua and tap.test.lua. But there are a lot 
of variants

of commenting in tests code.

> Versions known to me:
> -- comment
>
> or
>
> --- Function description
> -- some trololo
>
4. Chose this variant(--- Function description), because in some editors 
it will help to

read description for functions.

>> +ffi.cdef([[
>> +    int kill(pid_t pid, int sig);
>> +]])
>> +
>> +--
>> +--  Verify whether a process is alive.
>> +--
>> +local function process_is_alive(pid)
>> +    local rc = ffi.C.kill(pid, 0)
>> +    return rc == 0 or errno() ~= errno.ESRCH
>> +end
>> +
>> +--
>> +--  Return true if process completed before timeout.
>> +--  Return false otherwise.
>> +--
>> +local function wait_process_completion(pid, timeout)
>> +    local start_time = clock.monotonic()
>> +    local process_completed = false
>> +    while clock.monotonic() - start_time < timeout do
>> +        if not process_is_alive(pid) then
>> +            process_completed = true
>> +            break
>> +        end
>> +        clock.monotonic()
>
> What for this `clock.monotonic()`?
>
It's my mistake. Removed.
>> +    end
>> +    return process_completed
>> +end
>> +
>> +--
>> +-- Open file on reading with timeout.
>> +--
>> +local function open_with_timeout(filename, timeout)
>> +    local fh
>> +    local start_time = clock.monotonic()
>> +    while not fh and clock.monotonic() - start_time < timeout do
>> +        fh = fio.open(filename, {'O_RDONLY'})
>> +    end
>> +    return fh
>> +end
>> +
>> +--
>> +-- Try to read from file with timeout
>> +-- with interval.
>> +--
>> +local function read_with_timeout(fh, timeout, interval)
>> +    local data = ''
>> +    local start_time = clock.monotonic()
>> +    while #data == 0 and clock.monotonic() - start_time < timeout do
>> +        data = fh:read()
>> +        if #data == 0 then fiber.sleep(interval) end
>> +    end
>> +    return data
>> +end
>> +
>
> Up to you. I propose to move all this stuff into a separate
> function (exclude os.exit...).
> Maybe like it's done in "gh-4761-json-per-call-options.test.lua".
>
5. Done.
>> +local TARANTOOL_PATH = arg[-1]
>> +local output_file = fio.abspath('out.txt')
>> +local line = ('%s -e "assert(false)" > %s 2>&1 & echo 
>> $!'):format(TARANTOOL_PATH, output_file)
>> +local process_waiting_timeout = 2.0
>> +local file_read_timeout = 2.0
>> +local file_read_interval = 0.2
>> +local file_open_timeout = 2.0
>> +
>> +test:plan(2)
>> +
>> +local pid = tonumber(io.popen(line):read("*line"))
>> +assert(pid, "pid of proccess can't be recieved")
>> +
>> +local process_completed = wait_process_completion(pid, 
>> process_waiting_timeout)
>> +test:ok(process_completed, ('tarantool process with pid = %d 
>> completed'):format(pid))
>> +
>> +-- Kill process if hangs.
>> +if not process_completed then ffi.C.kill(pid, 9) end
>> +
>> +local fh = open_with_timeout(output_file, file_open_timeout)
>> +assert(fh, 'error while opening ' .. output_file)
>> +
>> +local data = read_with_timeout(fh, file_read_timeout, 
>> file_read_interval)
>> +test:like(data, "assertion failed", "assertion failure is displayed")
>> +
>> +fh:close()
>> +test:check()
>
> os.exit(test:check() and 0 or 1)
>
>>