From: Leonid Vasiliev <lvasiliev@tarantool.org> To: Artem Starshov <artemreyt@tarantool.org>, Alexander Turenko <alexander.turenko@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH 2/2] test: add test for tarantool -e assert(false) Date: Thu, 10 Dec 2020 21:11:49 +0300 [thread overview] Message-ID: <4f40bc8e-57ba-2cdc-4792-83e5713cea57@tarantool.org> (raw) In-Reply-To: <a24f8875f64ab9831fd2c83b4286ee71cf7d1a93.1607541187.git.artemreyt@tarantool.org> 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". > --- > .../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. > +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 -- Versions known to me: -- comment or --- Function description -- some trololo > +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()`? > + 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". > +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) >
next prev parent reply other threads:[~2020-12-10 18:12 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-09 19:28 [Tarantool-patches] [PATCH 0/2] tarantool -e 'assert(false)' hangs Artem Starshov 2020-12-09 19:28 ` [Tarantool-patches] [PATCH 1/2] src/lua: fix running init lua script Artem Starshov 2020-12-10 17:38 ` Leonid Vasiliev 2020-12-10 17:42 ` Leonid Vasiliev 2020-12-09 19:28 ` [Tarantool-patches] [PATCH 2/2] test: add test for tarantool -e assert(false) Artem Starshov 2020-12-10 18:11 ` Leonid Vasiliev [this message] 2020-12-14 14:11 ` Artem 2020-12-10 17:33 ` [Tarantool-patches] [PATCH 0/2] tarantool -e 'assert(false)' hangs Leonid Vasiliev
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4f40bc8e-57ba-2cdc-4792-83e5713cea57@tarantool.org \ --to=lvasiliev@tarantool.org \ --cc=alexander.turenko@tarantool.org \ --cc=artemreyt@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 2/2] test: add test for tarantool -e assert(false)' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox