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

[Tarantool-patches] [PATCH v1] 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 tests for it.
Fix helped to handle the problem with 'Address already in use' error.
Check the previous commit that set the use of sockets:

60f84cbfca24e3a91cea067c923e006b44ee589f ('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/init_script.test.lua                    | 7 ++++++-
 test/app-tap/suite.ini                               | 1 +
 test/app-tap/tarantoolctl.test.lua                   | 4 ++--
 test/box-tap/auth.test.lua                           | 6 +++++-
 test/box-tap/session.test.lua                        | 7 +++++--
 test/box-tap/suite.ini                               | 1 +
 test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua | 6 +++++-
 test/sql-tap/suite.ini                               | 1 +
 8 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/test/app-tap/init_script.test.lua b/test/app-tap/init_script.test.lua
index 155f149a7..d7cea7c40 100755
--- a/test/app-tap/init_script.test.lua
+++ b/test/app-tap/init_script.test.lua
@@ -2,8 +2,12 @@
 --
 -- Testing init script
 --
+
+local LISTEN_SOCKET = 'init_script.listen.sock'
+
+os.remove(LISTEN_SOCKET)
 box.cfg{
-    listen = os.getenv("LISTEN"),
+    listen = 'unix/:./' .. LISTEN_SOCKET,
     pid_file = "box.pid",
     memtx_memory=107374182,
     log="tarantool.log"
@@ -86,4 +90,5 @@ fiber.sleep(0.0)
 assert (require ~= nil)
 
 space:drop()
+os.remove(LISTEN_SOCKET)
 os.exit()
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..675e511dd 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -465,12 +465,12 @@ 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(
+    local admin_socket = tostring(
         test_run:eval("remote",
                       "return require('uri').parse(box.cfg.listen).service")[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/auth.test.lua b/test/box-tap/auth.test.lua
index 4e9879408..5d6687a33 100755
--- a/test/box-tap/auth.test.lua
+++ b/test/box-tap/auth.test.lua
@@ -6,8 +6,11 @@ local tap = require('tap')
 local netbox = require('net.box')
 local urilib = require('uri')
 
+local LISTEN_SOCKET = 'auth.listen.sock'
+os.remove(LISTEN_SOCKET)
+
 box.cfg {
-    listen = os.getenv('LISTEN');
+    listen = 'unix/:./' .. LISTEN_SOCKET,
     log="tarantool.log";
     memtx_memory=100*1024*1024;
 }
@@ -163,4 +166,5 @@ space:drop()
 box.schema.user.drop('test', { if_exists = true})
 box.schema.user.drop("test2", { if_exists = true})
 
+os.remove(LISTEN_SOCKET)
 os.exit(test:check() == true and 0 or -1)
diff --git a/test/box-tap/session.test.lua b/test/box-tap/session.test.lua
index 5d4965533..333736d72 100755
--- a/test/box-tap/session.test.lua
+++ b/test/box-tap/session.test.lua
@@ -4,11 +4,13 @@ local tap = require('tap')
 local test = tap.test('session')
 local fiber = require('fiber')
 
+local LISTEN_SOCKET = 'session.listen.sock'
+
+os.remove(LISTEN_SOCKET)
 box.cfg{
-    listen = os.getenv('LISTEN');
+    listen = 'unix/:./' .. LISTEN_SOCKET,
     log="tarantool.log";
 }
-
 local uri = require('uri').parse(box.cfg.listen)
 local HOST, PORT = uri.host or 'localhost', uri.service
 session = box.session
@@ -217,4 +219,5 @@ box.schema.user.revoke('guest', 'execute', 'function', 'f2')
 
 inspector:cmd('stop server session with cleanup=1')
 session = nil
+os.remove(LISTEN_SOCKET)
 os.exit(test:check() == true and 0 or -1)
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/gh-4077-iproto-execute-no-bind.test.lua b/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua
index d4b597e35..412649df8 100755
--- a/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua
+++ b/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua
@@ -15,8 +15,11 @@ local IPROTO_OK                 = 0x00
 local IPROTO_SCHEMA_VERSION     = 0x05
 local IPROTO_STATUS_KEY         = 0x00
 
+local LISTEN_SOCKET = 'gh-4077.listen.sock'
+
+os.remove(LISTEN_SOCKET)
 box.cfg({
-    listen = os.getenv('LISTEN') or 'localhost:3301',
+    listen = 'unix/:./' .. LISTEN_SOCKET,
 })
 
 box.schema.user.grant('guest', 'read,write,execute', 'universe')
@@ -69,4 +72,5 @@ test:is_deeply(res, exp_res, 'verify inserted data')
 box.execute('drop table T')
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 
+os.remove(LISTEN_SOCKET)
 os.exit(test:check() == true and 0 or 1)
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 v1] test: set unix sockets for iproto at core = app

[Sergey Bronnikov]

Hi, Alexander

On 11:06 Thu 14 May , 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 tests for it.
> Fix helped to handle the problem with 'Address already in use' error.
> Check the previous commit that set the use of sockets:
> 
> 60f84cbfca24e3a91cea067c923e006b44ee589f ('test: use unix sockets for iproto connections')
> 
> Closes #4459
> ---

LGTM

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

[Alexander Turenko]

The patch may be much simpler. I got 46 lines diff in result (see below
the email).

Please, consider comments below.

NB: Updated test-run with [1] on master, 2.4, 2.3, 1.10.

[1]: https://github.com/tarantool/test-run/pull/210

WBR, Alexander Turenko.

On Thu, May 14, 2020 at 11:06:06AM +0300, 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 tests for it.
> Fix helped to handle the problem with 'Address already in use' error.
> Check the previous commit that set the use of sockets:
> 
> 60f84cbfca24e3a91cea067c923e006b44ee589f ('test: use unix sockets for iproto connections')

Nit: over 72 symbols.

> 
> 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/init_script.test.lua                    | 7 ++++++-
>  test/app-tap/suite.ini                               | 1 +
>  test/app-tap/tarantoolctl.test.lua                   | 4 ++--
>  test/box-tap/auth.test.lua                           | 6 +++++-
>  test/box-tap/session.test.lua                        | 7 +++++--
>  test/box-tap/suite.ini                               | 1 +
>  test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua | 6 +++++-
>  test/sql-tap/suite.ini                               | 1 +
>  8 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/test/app-tap/init_script.test.lua b/test/app-tap/init_script.test.lua
> index 155f149a7..d7cea7c40 100755
> --- a/test/app-tap/init_script.test.lua
> +++ b/test/app-tap/init_script.test.lua
> @@ -2,8 +2,12 @@
>  --
>  -- Testing init script
>  --
> +
> +local LISTEN_SOCKET = 'init_script.listen.sock'
> +
> +os.remove(LISTEN_SOCKET)
>  box.cfg{
> -    listen = os.getenv("LISTEN"),
> +    listen = 'unix/:./' .. LISTEN_SOCKET,
>      pid_file = "box.pid",
>      memtx_memory=107374182,
>      log="tarantool.log"
> @@ -86,4 +90,5 @@ fiber.sleep(0.0)
>  assert (require ~= nil)
>  
>  space:drop()
> +os.remove(LISTEN_SOCKET)
>  os.exit()

You implemented `use_unix_sockets_iproto` option for app tests in
test-run ([1]), which sets LISTEN environment variable to a Unix socket
path (instead of a TCP port).

The option is implemented in the way that always give full
(non-relative) path and commonly used `box.cfg({listen =
os.getenv("LISTEN")})` statement should not be broken by the option.
That's neat: we can reduce changes in tests to its minimum.

This patch enables the option for tarantool's app tests. And we should
fix rare cases, where box.cfg.listen is assumed to be a TCP port number.

But here you made very strange thing: you throw out the fresly
implemented feature to a trash can and hardcode a unix path. Well, this
way is also acceptable, but if we'll going this way, why we implemented
the feature in test-run? Let's choose one way, not both at once.

This change is suspectful. If you unable to use just implemented
feature, then maybe it is not usable and should be fixed (or should not
be implemented at all?)

Anyway, I tried to just enable the option and everything works fine
here. We just need to left it unchanged.

[1]: https://github.com/tarantool/test-run/pull/210

> 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..675e511dd 100755
> --- a/test/app-tap/tarantoolctl.test.lua
> +++ b/test/app-tap/tarantoolctl.test.lua
> @@ -465,12 +465,12 @@ 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(
> +    local admin_socket = tostring(
>          test_run:eval("remote",
>                        "return require('uri').parse(box.cfg.listen).service")[1]
>      )

Nit: A console socket is often called 'admin socket' (see code block
just above this one), not a binary protocol socket.

Nit: tostring() is not needed, it is already string.

> -    local command_base = ('tarantoolctl play localhost:%d filler/00000000000000000000.xlog'):format(port)
> +    local command_base = ('tarantoolctl play %s filler/00000000000000000000.xlog'):format(admin_socket)

I asked the question to myself: what is the idea of this (original)
code?

* Ask remote instance how we can connect to it (URI or kinda).
* Pass this value to `tarantoolctl play` argument.

But why the code is so complex?

When TCP socket is used, test-run set LISTEN value to just port, say,
'3301' (all environment variables are strings). So box.cfg() is called
with {listen = '<port number>'} option. `box.cfg.listen` gives just
passed value (convert it to string if it is number, but it does not
matter here).

However author of the code know that box.cfg.listen can be in form
'<host>:<port>' and decided to write general code, which will not lean
on test-run peculiars much. But (s)he just don't keep unix sockets in
mind.

Can we keep the code general and make it even more general: support unix
sockets -- without increasing complexity? Yes!

What is URI in `tarantoolctl play URI FILE` command? It just passed to
net_box.connect(), which in fact support all box.cfg.listen schemas:

* '3301'
* 'localhost:3301'
* '/full/path/to/listen.socket'
* 'unix/:/full/patch/to/listen.socket'
* 'unix/:./relative/path/to/listen.socket'

So we need just don't modify box.cfg.listen value and pass it to the
`tarantoolctl play` argument. Everything will work.

This is neat, because the code becomes simpler and more
self-descriptive: just get URI and connect to it.

 |      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 listen_uri = 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(listen_uri)

It works with TCP and Unix sockets both.

----

Side notes.

While I thinking how to support both TCP and Unix sockets and how to
don't make the code more complex, several variants came into my mind.
They are not so neat as one above, but I would note them (maybe it will
be useful somewhere else).

1. Before I found that just '3301' is okay to pass to tarantoolct play I
   was looking how to construct either 'unix/:<...>' or
   'localhost:<port>' URI. There is the following way.

   We can use 'uri' module to reconstruct URI (TCP or Unix) from
   `box.cfg.listen` value.

    | local urilib = require('uri')
    | local uri = urilib.parse(box.cfg.listen)
    | uri.host = uri.host or 'localhost'
    | local listen_uri = urilib.format(uri)

   It is not a one-liner, so I considered it too complex for use in the
   test.

2. 'create server' test-run command has 'return_listen_uri' option. When
   it is passed, the command returns URI that test-run will pass to
   instance's environment via LISTEN variable.

   The returned URI has form 'localhost:3301' or
   '/path/to/listen.socket' and so suitable for the tarantoolctl
   argument.

   This variant is as good as passing `box.cfg.listen` w/o changes and
   maybe even better, but it brings the new entity to the patch (the
   test-run option), so make the patch harder to understand by a reader.
   So I would use this way in a new test, but keep test_run:eval() in an
   old test is possible.

> diff --git a/test/box-tap/auth.test.lua b/test/box-tap/auth.test.lua
> index 4e9879408..5d6687a33 100755
> --- a/test/box-tap/auth.test.lua
> +++ b/test/box-tap/auth.test.lua

Not needed, same as app-tap/init_script.test.lua.

> diff --git a/test/box-tap/session.test.lua b/test/box-tap/session.test.lua
> index 5d4965533..333736d72 100755
> --- a/test/box-tap/session.test.lua
> +++ b/test/box-tap/session.test.lua

Not needed, same as app-tap/init_script.test.lua.

> diff --git a/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua b/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua
> index d4b597e35..412649df8 100755
> --- a/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua
> +++ b/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua

Not needed, same as app-tap/init_script.test.lua.

----

Resulting patch:

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..9f1464617 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 listen_uri = 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(listen_uri)
 
     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..a2818fe62 100644
--- a/test/box-tap/suite.ini
+++ b/test/box-tap/suite.ini
@@ -5,3 +5,4 @@ is_parallel = True
 pretest_clean = True
 fragile = cfg.test.lua     ; gh-4344
           key_def.test.lua ; gh-4252
+use_unix_sockets_iproto = True
diff --git a/test/sql-tap/suite.ini b/test/sql-tap/suite.ini
index 8f3c3eab1..9a80ab7f9 100644
--- a/test/sql-tap/suite.ini
+++ b/test/sql-tap/suite.ini
@@ -31,3 +31,4 @@ show_reproduce_content = False
 pretest_clean = True
 fragile = gh-4077-iproto-execute-no-bind.test.lua ; gh-4459
           selectG.test.lua                        ; gh-4458
+use_unix_sockets_iproto = True

Verified it locally on Linux:

$ cmake .                          \
      -DCMAKE_BUILD_TYPE=Debug     \
      -DENABLE_BACKTRACE=ON        \
      -DENABLE_DIST=ON             \
      -DENABLE_BUNDLED_LIBCURL=OFF \
      && make -j
$ (cd test && ./test-run.py --force --long)
<...>
Statistics:
* disabled: 53
* pass: 1091

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

[Alexander V. Tikhonov]

Hi Alexander, thanks a lot for the review, I'll try to find the other
way to fix it. Unfortunately the patch you suggested at the end was the
same that I've started from, please check:

https://github.com/tarantool/tarantool/commit/ee080600df32f177dcdd5432217d081d3efc22aa

but in this way the tests still fail like I showed at the issue:

https://github.com/tarantool/tarantool/issues/4459#issuecomment-628430495

that is why I had to find some other way in addition to the fix.


On Thu, May 21, 2020 at 06:15:58AM +0300, Alexander Turenko wrote:
> The patch may be much simpler. I got 46 lines diff in result (see below
> the email).
> 
> Please, consider comments below.
> 
> NB: Updated test-run with [1] on master, 2.4, 2.3, 1.10.
> 
> [1]: https://github.com/tarantool/test-run/pull/210
> 
> WBR, Alexander Turenko.
> 
> On Thu, May 14, 2020 at 11:06:06AM +0300, 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 tests for it.
> > Fix helped to handle the problem with 'Address already in use' error.
> > Check the previous commit that set the use of sockets:
> > 
> > 60f84cbfca24e3a91cea067c923e006b44ee589f ('test: use unix sockets for iproto connections')
> 
> Nit: over 72 symbols.
> 
> > 
> > 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/init_script.test.lua                    | 7 ++++++-
> >  test/app-tap/suite.ini                               | 1 +
> >  test/app-tap/tarantoolctl.test.lua                   | 4 ++--
> >  test/box-tap/auth.test.lua                           | 6 +++++-
> >  test/box-tap/session.test.lua                        | 7 +++++--
> >  test/box-tap/suite.ini                               | 1 +
> >  test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua | 6 +++++-
> >  test/sql-tap/suite.ini                               | 1 +
> >  8 files changed, 26 insertions(+), 7 deletions(-)
> > 
> > diff --git a/test/app-tap/init_script.test.lua b/test/app-tap/init_script.test.lua
> > index 155f149a7..d7cea7c40 100755
> > --- a/test/app-tap/init_script.test.lua
> > +++ b/test/app-tap/init_script.test.lua
> > @@ -2,8 +2,12 @@
> >  --
> >  -- Testing init script
> >  --
> > +
> > +local LISTEN_SOCKET = 'init_script.listen.sock'
> > +
> > +os.remove(LISTEN_SOCKET)
> >  box.cfg{
> > -    listen = os.getenv("LISTEN"),
> > +    listen = 'unix/:./' .. LISTEN_SOCKET,
> >      pid_file = "box.pid",
> >      memtx_memory=107374182,
> >      log="tarantool.log"
> > @@ -86,4 +90,5 @@ fiber.sleep(0.0)
> >  assert (require ~= nil)
> >  
> >  space:drop()
> > +os.remove(LISTEN_SOCKET)
> >  os.exit()
> 
> You implemented `use_unix_sockets_iproto` option for app tests in
> test-run ([1]), which sets LISTEN environment variable to a Unix socket
> path (instead of a TCP port).
> 
> The option is implemented in the way that always give full
> (non-relative) path and commonly used `box.cfg({listen =
> os.getenv("LISTEN")})` statement should not be broken by the option.
> That's neat: we can reduce changes in tests to its minimum.
> 
> This patch enables the option for tarantool's app tests. And we should
> fix rare cases, where box.cfg.listen is assumed to be a TCP port number.
> 
> But here you made very strange thing: you throw out the fresly
> implemented feature to a trash can and hardcode a unix path. Well, this
> way is also acceptable, but if we'll going this way, why we implemented
> the feature in test-run? Let's choose one way, not both at once.
> 
> This change is suspectful. If you unable to use just implemented
> feature, then maybe it is not usable and should be fixed (or should not
> be implemented at all?)
> 
> Anyway, I tried to just enable the option and everything works fine
> here. We just need to left it unchanged.
> 
> [1]: https://github.com/tarantool/test-run/pull/210
> 
> > 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..675e511dd 100755
> > --- a/test/app-tap/tarantoolctl.test.lua
> > +++ b/test/app-tap/tarantoolctl.test.lua
> > @@ -465,12 +465,12 @@ 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(
> > +    local admin_socket = tostring(
> >          test_run:eval("remote",
> >                        "return require('uri').parse(box.cfg.listen).service")[1]
> >      )
> 
> Nit: A console socket is often called 'admin socket' (see code block
> just above this one), not a binary protocol socket.
> 
> Nit: tostring() is not needed, it is already string.
> 
> > -    local command_base = ('tarantoolctl play localhost:%d filler/00000000000000000000.xlog'):format(port)
> > +    local command_base = ('tarantoolctl play %s filler/00000000000000000000.xlog'):format(admin_socket)
> 
> I asked the question to myself: what is the idea of this (original)
> code?
> 
> * Ask remote instance how we can connect to it (URI or kinda).
> * Pass this value to `tarantoolctl play` argument.
> 
> But why the code is so complex?
> 
> When TCP socket is used, test-run set LISTEN value to just port, say,
> '3301' (all environment variables are strings). So box.cfg() is called
> with {listen = '<port number>'} option. `box.cfg.listen` gives just
> passed value (convert it to string if it is number, but it does not
> matter here).
> 
> However author of the code know that box.cfg.listen can be in form
> '<host>:<port>' and decided to write general code, which will not lean
> on test-run peculiars much. But (s)he just don't keep unix sockets in
> mind.
> 
> Can we keep the code general and make it even more general: support unix
> sockets -- without increasing complexity? Yes!
> 
> What is URI in `tarantoolctl play URI FILE` command? It just passed to
> net_box.connect(), which in fact support all box.cfg.listen schemas:
> 
> * '3301'
> * 'localhost:3301'
> * '/full/path/to/listen.socket'
> * 'unix/:/full/patch/to/listen.socket'
> * 'unix/:./relative/path/to/listen.socket'
> 
> So we need just don't modify box.cfg.listen value and pass it to the
> `tarantoolctl play` argument. Everything will work.
> 
> This is neat, because the code becomes simpler and more
> self-descriptive: just get URI and connect to it.
> 
>  |      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 listen_uri = 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(listen_uri)
> 
> It works with TCP and Unix sockets both.
> 
> ----
> 
> Side notes.
> 
> While I thinking how to support both TCP and Unix sockets and how to
> don't make the code more complex, several variants came into my mind.
> They are not so neat as one above, but I would note them (maybe it will
> be useful somewhere else).
> 
> 1. Before I found that just '3301' is okay to pass to tarantoolct play I
>    was looking how to construct either 'unix/:<...>' or
>    'localhost:<port>' URI. There is the following way.
> 
>    We can use 'uri' module to reconstruct URI (TCP or Unix) from
>    `box.cfg.listen` value.
> 
>     | local urilib = require('uri')
>     | local uri = urilib.parse(box.cfg.listen)
>     | uri.host = uri.host or 'localhost'
>     | local listen_uri = urilib.format(uri)
> 
>    It is not a one-liner, so I considered it too complex for use in the
>    test.
> 
> 2. 'create server' test-run command has 'return_listen_uri' option. When
>    it is passed, the command returns URI that test-run will pass to
>    instance's environment via LISTEN variable.
> 
>    The returned URI has form 'localhost:3301' or
>    '/path/to/listen.socket' and so suitable for the tarantoolctl
>    argument.
> 
>    This variant is as good as passing `box.cfg.listen` w/o changes and
>    maybe even better, but it brings the new entity to the patch (the
>    test-run option), so make the patch harder to understand by a reader.
>    So I would use this way in a new test, but keep test_run:eval() in an
>    old test is possible.
> 
> > diff --git a/test/box-tap/auth.test.lua b/test/box-tap/auth.test.lua
> > index 4e9879408..5d6687a33 100755
> > --- a/test/box-tap/auth.test.lua
> > +++ b/test/box-tap/auth.test.lua
> 
> Not needed, same as app-tap/init_script.test.lua.
> 
> > diff --git a/test/box-tap/session.test.lua b/test/box-tap/session.test.lua
> > index 5d4965533..333736d72 100755
> > --- a/test/box-tap/session.test.lua
> > +++ b/test/box-tap/session.test.lua
> 
> Not needed, same as app-tap/init_script.test.lua.
> 
> > diff --git a/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua b/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua
> > index d4b597e35..412649df8 100755
> > --- a/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua
> > +++ b/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua
> 
> Not needed, same as app-tap/init_script.test.lua.
> 
> ----
> 
> Resulting patch:
> 
> 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..9f1464617 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 listen_uri = 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(listen_uri)
>  
>      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..a2818fe62 100644
> --- a/test/box-tap/suite.ini
> +++ b/test/box-tap/suite.ini
> @@ -5,3 +5,4 @@ is_parallel = True
>  pretest_clean = True
>  fragile = cfg.test.lua     ; gh-4344
>            key_def.test.lua ; gh-4252
> +use_unix_sockets_iproto = True
> diff --git a/test/sql-tap/suite.ini b/test/sql-tap/suite.ini
> index 8f3c3eab1..9a80ab7f9 100644
> --- a/test/sql-tap/suite.ini
> +++ b/test/sql-tap/suite.ini
> @@ -31,3 +31,4 @@ show_reproduce_content = False
>  pretest_clean = True
>  fragile = gh-4077-iproto-execute-no-bind.test.lua ; gh-4459
>            selectG.test.lua                        ; gh-4458
> +use_unix_sockets_iproto = True
> 
> Verified it locally on Linux:
> 
> $ cmake .                          \
>       -DCMAKE_BUILD_TYPE=Debug     \
>       -DENABLE_BACKTRACE=ON        \
>       -DENABLE_DIST=ON             \
>       -DENABLE_BUNDLED_LIBCURL=OFF \
>       && make -j
> $ (cd test && ./test-run.py --force --long)
> <...>
> Statistics:
> * disabled: 53
> * pass: 1091

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

[Alexander Turenko]

On Thu, May 21, 2020 at 07:30:20AM +0300, Alexander V. Tikhonov wrote:
> Hi Alexander, thanks a lot for the review, I'll try to find the other
> way to fix it. Unfortunately the patch you suggested at the end was the
> same that I've started from, please check:
> 
> https://github.com/tarantool/tarantool/commit/ee080600df32f177dcdd5432217d081d3efc22aa
> 
> but in this way the tests still fail like I showed at the issue:
> 
> https://github.com/tarantool/tarantool/issues/4459#issuecomment-628430495
> 
> that is why I had to find some other way in addition to the fix.

os.putenv() from a non-default server (say, 'session_storage' instance)
rewrites LISTEN environment variable within the same process, where
AppServer is run. But AppServer is not like TarantoolServer: it spawns a
new child process (tarantool) for each test.

We can save necessary port/path and put it to environment just before
spawning a new process:

diff --git a/lib/app_server.py b/lib/app_server.py
index c2d2ea9..2cb8a87 100644
--- a/lib/app_server.py
+++ b/lib/app_server.py
@@ -19,6 +19,7 @@ from test import TestRunGreenlet, TestExecutionError
 
 
 def run_server(execs, cwd, server, logfile, retval):
+    os.putenv("LISTEN", server.iproto)
     server.process = Popen(execs, stdout=PIPE, stderr=PIPE, cwd=cwd)
     stdout, stderr = server.process.communicate()
     sys.stdout.write(stdout)
@@ -108,9 +109,9 @@ class AppServer(Server):
         if self.use_unix_sockets_iproto:
             path = os.path.join(self.vardir, self.name + ".socket-iproto")
             warn_unix_socket(path)
-            os.putenv("LISTEN", path)
+            self.iproto = path
         else:
-            os.putenv("LISTEN", str(find_port()))
+            self.iproto = str(find_port())
         shutil.copy(os.path.join(self.TEST_RUN_DIR, 'test_run.lua'),
                     self.vardir)

WBR, Alexander Turenko.

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

[Alexander V. Tikhonov]

Alexander, great, you found the root cause of the issue in my patch to
test-run, many thanks. I've checked your fix and there is no need for
now in my additional changes, so sure I've reverted my patch as you
suggested to its initial version. Please check 2nd version of the
patch with all your suggestions to tarantoolctl test fix.

As about test-run, I've made the new pull request with your patch which
I've checked:

https://github.com/tarantool/test-run/pull/211

On Thu, May 21, 2020 at 09:19:58AM +0300, Alexander Turenko wrote:
> On Thu, May 21, 2020 at 07:30:20AM +0300, Alexander V. Tikhonov wrote:
> > Hi Alexander, thanks a lot for the review, I'll try to find the other
> > way to fix it. Unfortunately the patch you suggested at the end was the
> > same that I've started from, please check:
> > 
> > https://github.com/tarantool/tarantool/commit/ee080600df32f177dcdd5432217d081d3efc22aa
> > 
> > but in this way the tests still fail like I showed at the issue:
> > 
> > https://github.com/tarantool/tarantool/issues/4459#issuecomment-628430495
> > 
> > that is why I had to find some other way in addition to the fix.
> 
> os.putenv() from a non-default server (say, 'session_storage' instance)
> rewrites LISTEN environment variable within the same process, where
> AppServer is run. But AppServer is not like TarantoolServer: it spawns a
> new child process (tarantool) for each test.
> 
> We can save necessary port/path and put it to environment just before
> spawning a new process:
> 
> diff --git a/lib/app_server.py b/lib/app_server.py
> index c2d2ea9..2cb8a87 100644
> --- a/lib/app_server.py
> +++ b/lib/app_server.py
> @@ -19,6 +19,7 @@ from test import TestRunGreenlet, TestExecutionError
>  
>  
>  def run_server(execs, cwd, server, logfile, retval):
> +    os.putenv("LISTEN", server.iproto)
>      server.process = Popen(execs, stdout=PIPE, stderr=PIPE, cwd=cwd)
>      stdout, stderr = server.process.communicate()
>      sys.stdout.write(stdout)
> @@ -108,9 +109,9 @@ class AppServer(Server):
>          if self.use_unix_sockets_iproto:
>              path = os.path.join(self.vardir, self.name + ".socket-iproto")
>              warn_unix_socket(path)
> -            os.putenv("LISTEN", path)
> +            self.iproto = path
>          else:
> -            os.putenv("LISTEN", str(find_port()))
> +            self.iproto = str(find_port())
>          shutil.copy(os.path.join(self.TEST_RUN_DIR, 'test_run.lua'),
>                      self.vardir)
> 
> WBR, Alexander Turenko.

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

[Alexander Turenko]

On Thu, May 21, 2020 at 10:59:05AM +0300, Alexander V. Tikhonov wrote:
> Alexander, great, you found the root cause of the issue in my patch to
> test-run, many thanks. I've checked your fix and there is no need for
> now in my additional changes, so sure I've reverted my patch as you
> suggested to its initial version. Please check 2nd version of the
> patch with all your suggestions to tarantoolctl test fix.
> 
> As about test-run, I've made the new pull request with your patch which
> I've checked:
> 
> https://github.com/tarantool/test-run/pull/211

Updated description, pushed to test-run master.

Promoted the test-run submodule in tarantool on master, 2.4, 2.3 and
1.10 branch.

WBR, Alexander Turenko.