[Tarantool-patches] [PATCH v2] test: set unix sockets for iproto at core = app

[Tarantool-patches] [PATCH v2] test: set unix sockets for iproto at core = app

[Alexander V. Tikhonov]

Set hard-coded unix sockets for iproto connections at core = app.
Added its option in *-tap/suites.ini files and fixed test for it.
Fix helped to handle the problem with 'Address already in use' error.
Check the previous commit that set the use of sockets:

60f84cbf ('test: use unix sockets for iproto connections')

Closes #4459
---

Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4008-unix_sockets_iproto_sql_tap
Issue: https://github.com/tarantool/tarantool/issues/4459

 test/app-tap/suite.ini             | 1 +
 test/app-tap/tarantoolctl.test.lua | 7 ++-----
 test/box-tap/suite.ini             | 1 +
 test/sql-tap/suite.ini             | 1 +
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/test/app-tap/suite.ini b/test/app-tap/suite.ini
index 9629dfad5..daababef5 100644
--- a/test/app-tap/suite.ini
+++ b/test/app-tap/suite.ini
@@ -4,3 +4,4 @@ description = application server tests (TAP)
 lua_libs = lua/require_mod.lua lua/serializer_test.lua
 is_parallel = True
 pretest_clean = True
+use_unix_sockets_iproto = True
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index 4d7059559..dd90c8a25 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -465,12 +465,9 @@ else
     local remote_path = create_script(dir, 'remote.lua', remote_code)
     test_run:cmd(("create server remote with script='%s'"):format(remote_path))
     test_run:cmd("start server remote")
-    local port = tonumber(
-        test_run:eval("remote",
-                      "return require('uri').parse(box.cfg.listen).service")[1]
-    )
+    local admin_socket = test_run:eval("remote", "return box.cfg.listen")[1]
 
-    local command_base = ('tarantoolctl play localhost:%d filler/00000000000000000000.xlog'):format(port)
+    local command_base = ('tarantoolctl play %s filler/00000000000000000000.xlog'):format(admin_socket)
 
     local status, err = pcall(function()
         test:test("fill and test play output", function(test_i)
diff --git a/test/box-tap/suite.ini b/test/box-tap/suite.ini
index 8d9e32d3f..3cdef2131 100644
--- a/test/box-tap/suite.ini
+++ b/test/box-tap/suite.ini
@@ -3,5 +3,6 @@ core = app
 description = Database tests with #! using TAP
 is_parallel = True
 pretest_clean = True
+use_unix_sockets_iproto = True
 fragile = cfg.test.lua     ; gh-4344
           key_def.test.lua ; gh-4252
diff --git a/test/sql-tap/suite.ini b/test/sql-tap/suite.ini
index 8f3c3eab1..0e8498875 100644
--- a/test/sql-tap/suite.ini
+++ b/test/sql-tap/suite.ini
@@ -29,5 +29,6 @@ long_run = gh-3332-tuple-format-leak.test.lua, gh-3083-ephemeral-unref-tuples.te
 config = engine.cfg
 show_reproduce_content = False
 pretest_clean = True
+use_unix_sockets_iproto = True
 fragile = gh-4077-iproto-execute-no-bind.test.lua ; gh-4459
           selectG.test.lua                        ; gh-4458
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v2] test: set unix sockets for iproto at core = app

[Alexander Turenko]

LGTM.

(But should NOT be pushed to master until I'll push test-run fix from PR
#211.)

Left a couple of nits, however. See below.

WBR, Alexander Turenko.

> 60f84cbf ('test: use unix sockets for iproto connections')

Nit: We mention commits in the form
0000000000000000000000000000000000000000 ('commit message header'): full
40-digits hash and the commit message header. You'll need to carry the
header when will use full commit hash and it is okay.

Personally I think that short hashes lookes better when used inline
within a text and that our style will recommend them sooner or later.
But now it is not so.

> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
> index 4d7059559..dd90c8a25 100755
> --- a/test/app-tap/tarantoolctl.test.lua
> +++ b/test/app-tap/tarantoolctl.test.lua
> @@ -465,12 +465,9 @@ else
>      local remote_path = create_script(dir, 'remote.lua', remote_code)
>      test_run:cmd(("create server remote with script='%s'"):format(remote_path))
>      test_run:cmd("start server remote")
> -    local port = tonumber(
> -        test_run:eval("remote",
> -                      "return require('uri').parse(box.cfg.listen).service")[1]
> -    )
> +    local admin_socket = test_run:eval("remote", "return box.cfg.listen")[1]

Nit: Admin port / socket usually refer a console port / socket. I would
name the variable `listen_url` or `remote_uri`. The former is used
several times across tests and also is part of `return_listen_uri`
option name.

Re: [Tarantool-patches] [PATCH v2] test: set unix sockets for iproto at core = app

[Alexander V. Tikhonov]

Alexander, thanks for the review, I've corrected all as you suggested.
Sure I'll control that test-run must be pushed before this patch.

On Thu, May 21, 2020 at 02:29:33PM +0300, Alexander Turenko wrote:
> LGTM.
> 
> (But should NOT be pushed to master until I'll push test-run fix from PR
> #211.)
> 
> Left a couple of nits, however. See below.
> 
> WBR, Alexander Turenko.
> 
> > 60f84cbf ('test: use unix sockets for iproto connections')
> 
> Nit: We mention commits in the form
> 0000000000000000000000000000000000000000 ('commit message header'): full
> 40-digits hash and the commit message header. You'll need to carry the
> header when will use full commit hash and it is okay.
> 
> Personally I think that short hashes lookes better when used inline
> within a text and that our style will recommend them sooner or later.
> But now it is not so.
> 
> > diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
> > index 4d7059559..dd90c8a25 100755
> > --- a/test/app-tap/tarantoolctl.test.lua
> > +++ b/test/app-tap/tarantoolctl.test.lua
> > @@ -465,12 +465,9 @@ else
> >      local remote_path = create_script(dir, 'remote.lua', remote_code)
> >      test_run:cmd(("create server remote with script='%s'"):format(remote_path))
> >      test_run:cmd("start server remote")
> > -    local port = tonumber(
> > -        test_run:eval("remote",
> > -                      "return require('uri').parse(box.cfg.listen).service")[1]
> > -    )
> > +    local admin_socket = test_run:eval("remote", "return box.cfg.listen")[1]
> 
> Nit: Admin port / socket usually refer a console port / socket. I would
> name the variable `listen_url` or `remote_uri`. The former is used
> several times across tests and also is part of `return_listen_uri`
> option name.

Re: [Tarantool-patches] [PATCH v2] test: set unix sockets for iproto at core = app

[Alexander Turenko]

> Closes #4459
> ---

> diff --git a/test/sql-tap/suite.ini b/test/sql-tap/suite.ini
> index 8f3c3eab1..0e8498875 100644
> --- a/test/sql-tap/suite.ini
> +++ b/test/sql-tap/suite.ini
> @@ -29,5 +29,6 @@ long_run = gh-3332-tuple-format-leak.test.lua, gh-3083-ephemeral-unref-tuples.te
>  config = engine.cfg
>  show_reproduce_content = False
>  pretest_clean = True
> +use_unix_sockets_iproto = True
>  fragile = gh-4077-iproto-execute-no-bind.test.lua ; gh-4459
>            selectG.test.lua                        ; gh-4458

We added gh-4077-iproto-execute-no-bind.test.lua test into fragile test
list because of the 'Address already in use' problem, which we're going
to workaround here. So it worth to remove the test from the fragile test
list.

Re: [Tarantool-patches] [PATCH v2] test: set unix sockets for iproto at core = app

[Alexander V. Tikhonov]

Alexander, thank you for the correction, removed the test from the
fragiled list and mentioned it in the commit message.

On Thu, May 21, 2020 at 08:53:03PM +0300, Alexander Turenko wrote:
> > Closes #4459
> > ---
> 
> > diff --git a/test/sql-tap/suite.ini b/test/sql-tap/suite.ini
> > index 8f3c3eab1..0e8498875 100644
> > --- a/test/sql-tap/suite.ini
> > +++ b/test/sql-tap/suite.ini
> > @@ -29,5 +29,6 @@ long_run = gh-3332-tuple-format-leak.test.lua, gh-3083-ephemeral-unref-tuples.te
> >  config = engine.cfg
> >  show_reproduce_content = False
> >  pretest_clean = True
> > +use_unix_sockets_iproto = True
> >  fragile = gh-4077-iproto-execute-no-bind.test.lua ; gh-4459
> >            selectG.test.lua                        ; gh-4458
> 
> We added gh-4077-iproto-execute-no-bind.test.lua test into fragile test
> list because of the 'Address already in use' problem, which we're going
> to workaround here. So it worth to remove the test from the fragile test
> list.

Re: [Tarantool-patches] [PATCH v2] test: set unix sockets for iproto at core = app

[Oleg Piskunov]

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


LGTM.

  
>Четверг, 21 мая 2020, 14:38 +03:00 от Alexander V. Tikhonov <avtikhon@tarantool.org>:
> 
>Alexander, thanks for the review, I've corrected all as you suggested.
>Sure I'll control that test-run must be pushed before this patch.
>
>On Thu, May 21, 2020 at 02:29:33PM +0300, Alexander Turenko wrote:
>> LGTM.
>>
>> (But should NOT be pushed to master until I'll push test-run fix from PR
>> #211.)
>>
>> Left a couple of nits, however. See below.
>>
>> WBR, Alexander Turenko.
>>
>> > 60f84cbf ('test: use unix sockets for iproto connections')
>>
>> Nit: We mention commits in the form
>> 0000000000000000000000000000000000000000 ('commit message header'): full
>> 40-digits hash and the commit message header. You'll need to carry the
>> header when will use full commit hash and it is okay.
>>
>> Personally I think that short hashes lookes better when used inline
>> within a text and that our style will recommend them sooner or later.
>> But now it is not so.
>>
>> > diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
>> > index 4d7059559..dd90c8a25 100755
>> > --- a/test/app-tap/tarantoolctl.test.lua
>> > +++ b/test/app-tap/tarantoolctl.test.lua
>> > @@ -465,12 +465,9 @@ else
>> > local remote_path = create_script(dir, 'remote.lua', remote_code)
>> > test_run:cmd(("create server remote with script='%s'"):format(remote_path))
>> > test_run:cmd("start server remote")
>> > - local port = tonumber(
>> > - test_run:eval("remote",
>> > - "return require('uri').parse(box.cfg.listen).service")[1]
>> > - )
>> > + local admin_socket = test_run:eval("remote", "return box.cfg.listen")[1]
>>
>> Nit: Admin port / socket usually refer a console port / socket. I would
>> name the variable `listen_url` or `remote_uri`. The former is used
>> several times across tests and also is part of `return_listen_uri`
>> option name. 
 
 
--
Oleg Piskunov
 

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

Re: [Tarantool-patches] [PATCH v2] test: set unix sockets for iproto at core = app

[Kirill Yukhin]

Hello,

On 21 май 10:45, Alexander V. Tikhonov wrote:
> Set hard-coded unix sockets for iproto connections at core = app.
> Added its option in *-tap/suites.ini files and fixed test for it.
> Fix helped to handle the problem with 'Address already in use' error.
> Check the previous commit that set the use of sockets:
> 
> 60f84cbf ('test: use unix sockets for iproto connections')
> 
> Closes #4459
> ---
> 
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4008-unix_sockets_iproto_sql_tap
> Issue: https://github.com/tarantool/tarantool/issues/4459

I've checked your patch into 1.10, 2.3, 2.4 and master.

--
Regards, Kirill Yukhin