Re: [tarantool-patches] Re: [PATCH] test: fix app-tap/tarantoolctl sporadic failure

[Serge Petrenko <sergepetrenko@tarantool.org>]

Hi! All fixed, new diff is below.

> 16 авг. 2018 г., в 22:42, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> On Thu, Aug 16, 2018 at 06:24:00PM +0300, Serge Petrenko wrote:
>> In rare cases this test failed on `tarantoolctl status` after
>> `tarantoolctl stop` due to `tarantoolctl stop` taking too much time to
>> unlink pid file.
> 
> This is too vague for an explanation. AFAIU the problem here is that the
> pid file is deleted by tarantool (from tarantool_free => pidfile_remove)
> while the control socket file is deleted by tarantoolctl so if
> `tarantoolctl status` is called immediately after `tarantoolctl stop`,
> there's a chance tarantool hasn't exited yet and so the pid file still
> exists.
> 
> Please update the comment.
> 
>> Fix this by waiting for pid file deletion in test.
>> 
>> Closes #3557
>> ---
>> https://github.com/tarantool/tarantool/issues/3557
>> https://github.com/tarantool/tarantool/tree/sp/gh-3557-tarantoolctl-test-fix
>> 
>> 
>> test/app-tap/tarantoolctl.test.lua | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>> 
>> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
>> index 6946c8312..83a8bfc37 100755
>> --- a/test/app-tap/tarantoolctl.test.lua
>> +++ b/test/app-tap/tarantoolctl.test.lua
>> @@ -34,6 +34,13 @@ local function recursive_rmdir(path)
>>     end
>> end
>> 
>> +local function wait_delete(path)
>> +    path = fio.abspath(path)
> 
> You don't need to call abspath here.
> 
>> +    while fio.path.exists(path) do
>> +	fiber.sleep(0.0001)
> 
> A tab! Please fix your editor.
> 
> Also, please use fiber.sleep(0.01) for consistency with other timeouts
> used in this script.
> 
>> +    end
>> +end
>> +
> 
> There's tctl_wait() helper that waits for tarantool to start.
> Let's rename it to tctl_wait_start() and introduce tctl_wait_stop()
> with the same signature that would wait for tarantool to stop.
> 
>> ffi.cdef[[
>> typedef int32_t pid_t;
>> int kill(pid_t pid, int sig);
>> @@ -157,6 +164,7 @@ test:plan(6)
>> do
>>     local dir = fio.tempdir()
>>     local code = [[ box.cfg{memtx_memory = 104857600} ]]
>> +    local pid_path = fio.pathjoin(dir, 'script.pid')
>>     create_script(dir, 'script.lua', code)
>> 
>>     local status, err = pcall(function()
>> @@ -168,6 +176,7 @@ do
>>             check_ok(test_i, dir, 'start',  'script', 1, nil, "is already running")
>>             check_ok(test_i, dir, 'status', 'script', 0, nil, "is running")
>>             check_ok(test_i, dir, 'stop',   'script', 0, nil, "Stopping")
>> +            wait_delete(pid_path)
>>             check_ok(test_i, dir, 'status', 'script', 1, nil, "is stopped")
>>             check_ok(test_i, dir, 'stop',   'script', 0, nil, "is not running")
>>             check_ok(test_i, dir, 'status', 'script', 1, nil, "is stopped" )
> 

 test/app-tap/tarantoolctl.test.lua | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index 6946c8312..cc4ca17b6 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -86,7 +86,7 @@ local function run_command(dir, command)
     return res/256, fstdout_e, fstderr_e
 end
 
-local function tctl_wait(dir, name)
+local function tctl_wait_start(dir, name)
     if name then
         local path = fio.pathjoin(dir, name .. '.control')
         while not fio.stat(path) do
@@ -113,6 +113,13 @@ local function tctl_wait(dir, name)
     end
 end
 
+local function tctl_wait_stop(dir, name)
+    local path = fio.pathjoin(dir, name .. '.pid')
+    while fio.path.exists(path) do
+        fiber.sleep(0.01)
+    end
+end
+
 local function tctl_command(dir, cmd, args, name)
     local pid = nil
     if not fio.stat(fio.pathjoin(dir, '.tarantoolctl')) then
@@ -163,11 +170,12 @@ do
         test:test("basic test", function(test_i)
             test_i:plan(16)
             check_ok(test_i, dir, 'start',  'script', 0, nil, "Starting instance")
-            tctl_wait(dir, 'script')
+            tctl_wait_start(dir, 'script')
             check_ok(test_i, dir, 'status', 'script', 0, nil, "is running")
             check_ok(test_i, dir, 'start',  'script', 1, nil, "is already running")
             check_ok(test_i, dir, 'status', 'script', 0, nil, "is running")
             check_ok(test_i, dir, 'stop',   'script', 0, nil, "Stopping")
+            tctl_wait_stop(dir, 'script')
             check_ok(test_i, dir, 'status', 'script', 1, nil, "is stopped")
             check_ok(test_i, dir, 'stop',   'script', 0, nil, "is not running")
             check_ok(test_i, dir, 'status', 'script', 1, nil, "is stopped" )
@@ -200,7 +208,7 @@ do
             check_ok(test_i, dir, 'start', 'bad_script', 1, nil,
                      'unexpected symbol near')
             check_ok(test_i, dir, 'start', 'good_script', 0)
-            tctl_wait(dir, 'good_script')
+            tctl_wait_start(dir, 'good_script')
             -- wait here
             check_ok(test_i, dir, 'eval',  'good_script bad_script.lua', 3,
                      nil, 'Error while reloading config:')
@@ -232,7 +240,7 @@ do
         test:test("check answers in case of call", function(test_i)
             test_i:plan(6)
             check_ok(test_i, dir, 'start', 'good_script', 0)
-            tctl_wait(dir, 'good_script')
+            tctl_wait_start(dir, 'good_script')
             check_ok(test_i, dir, 'eval',  'good_script bad_script.lua', 3, nil,
                      'Error while reloading config')
             check_ok(test_i, dir, 'eval',  'good_script ok_script.lua', 0,
-- 
2.15.2 (Apple Git-101.1)