From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0018745C304 for ; Mon, 14 Dec 2020 17:11:40 +0300 (MSK) References: <4f40bc8e-57ba-2cdc-4792-83e5713cea57@tarantool.org> From: Artem Message-ID: <3776cda0-9fe5-d29c-9f32-7cebfa5ffa19@tarantool.org> Date: Mon, 14 Dec 2020 17:11:39 +0300 MIME-Version: 1.0 In-Reply-To: <4f40bc8e-57ba-2cdc-4792-83e5713cea57@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: ru Subject: Re: [Tarantool-patches] [PATCH 2/2] test: add test for tarantool -e assert(false) List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Vasiliev Cc: tarantool-patches@dev.tarantool.org 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) > >>