[Tarantool-patches] [PATCH v3] test: flaky box/net.box_wait_connected_gh-3856

[Tarantool-patches] [PATCH v3] test: flaky box/net.box_wait_connected_gh-3856

[Alexander V. Tikhonov]

Found issue running test on FreeBSD VBox host:

 [011] --- box/net.box_wait_connected_gh-3856.result	Mon Jun 15 09:39:49 2020
 [011] +++ box/net.box_wait_connected_gh-3856.reject	Fri May  8 08:23:30 2020
 [011] @@ -12,7 +12,8 @@
 [011]  - opts:
 [011]      wait_connected: false
 [011]    host: 8.8.8.8
 [011] -  state: initial
 [011] +  state: error
 [011] +  error: Invalid argument
 [011]    port: '123456'
 [011]  ...
 [011]  c:close()

A. Turenko made deep investigation and found that the reason of the
fail was that getaddrinfo() returned EIA_SERVICE for an incorrect
TCP/IP port on FreeBSD, but crops it as modulo of 65536 on Linux/glibc.
Checked with his local script './getaddrinfo':

  (Linux/glibc) $ ./getaddrinfo 8.8.8.8 123456
  ----
  family: AF_INET
  socktype: SOCK_STREAM
  protocol: IPPROTO_TCP
  host: 8.8.8.8
  serv: 57920

  (FreeBSD) $ ./getaddrinfo 8.8.8.8 123456
  getaddrinfo: Service was not recognized for socket type

So obvious fix is to change 123456 to something less or equal to
65535. Say, 1234.

The test depended on an order in which fibers were scheduled
(net_box.connect() creates a separate fiber for connecting in background
using fiber.create(), which yields). Unlikely our fiber were not get
execution time during the connection attempt, so it was more like a
formal thing.

But we can decrease probability of this situation even more if we'll
grab all connection fields just when net_box.connect() returns, not
after yield in console (which is due to waiting a next command from
test-run).

Closes #5083

Co-authored-by: Alexander Turenko <alexander.turenko@tarantool.org>
Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>
---

Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-5083-net-box-google-dns
Issue: https://github.com/tarantool/tarantool/issues/5083

 .../box/net.box_wait_connected_gh-3856.result | 25 +++++++++++++------
 .../net.box_wait_connected_gh-3856.test.lua   | 14 ++++++++---
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/test/box/net.box_wait_connected_gh-3856.result b/test/box/net.box_wait_connected_gh-3856.result
index 9234e6cb9..ec20df462 100644
--- a/test/box/net.box_wait_connected_gh-3856.result
+++ b/test/box/net.box_wait_connected_gh-3856.result
@@ -1,20 +1,29 @@
 net = require('net.box')
 ---
 ...
+test_run = require('test_run').new()
+---
+...
 --
 -- gh-3856: wait_connected = false is ignored.
 --
-c = net.connect('8.8.8.8:123456', {wait_connected = false})
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+function check_connection()
+    local c = net.connect('8.8.8.8:1234', {wait_connected = false})
+    local cstate = c.state
+    c:close()
+    return cstate
+end;
 ---
 ...
-c
+test_run:cmd("setopt delimiter ''");
 ---
-- opts:
-    wait_connected: false
-  host: 8.8.8.8
-  state: initial
-  port: '123456'
+- true
 ...
-c:close()
+check_connection()
 ---
+- initial
 ...
diff --git a/test/box/net.box_wait_connected_gh-3856.test.lua b/test/box/net.box_wait_connected_gh-3856.test.lua
index 29e997fb5..7721d3379 100644
--- a/test/box/net.box_wait_connected_gh-3856.test.lua
+++ b/test/box/net.box_wait_connected_gh-3856.test.lua
@@ -1,8 +1,16 @@
 net = require('net.box')
+test_run = require('test_run').new()
 
 --
 -- gh-3856: wait_connected = false is ignored.
 --
-c = net.connect('8.8.8.8:123456', {wait_connected = false})
-c
-c:close()
+test_run:cmd("setopt delimiter ';'")
+function check_connection()
+    local c = net.connect('8.8.8.8:1234', {wait_connected = false})
+    local cstate = c.state
+    c:close()
+    return cstate
+end;
+test_run:cmd("setopt delimiter ''");
+
+check_connection()
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v3] test: flaky box/net.box_wait_connected_gh-3856

[Alexander Turenko]

LGTM.

I have a few comments, but it is just for appearance, nothing critical.
No need to re-review with me if you'll change it in the proposed ways.

WBR, Alexander Turenko.

> Co-authored-by: Alexander Turenko <alexander.turenko@tarantool.org>
> Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>

Typo: no space before '<'.

> diff --git a/test/box/net.box_wait_connected_gh-3856.test.lua b/test/box/net.box_wait_connected_gh-3856.test.lua
> index 29e997fb5..7721d3379 100644
> --- a/test/box/net.box_wait_connected_gh-3856.test.lua
> +++ b/test/box/net.box_wait_connected_gh-3856.test.lua
> @@ -1,8 +1,16 @@
>  net = require('net.box')
> +test_run = require('test_run').new()

It is not necessary anymore: you can just use backslash and don't use
delimiters. Up to you, but this way is shorter and looks more elegant.

> +test_run:cmd("setopt delimiter ';'")
> +function check_connection()
> +    local c = net.connect('8.8.8.8:1234', {wait_connected = false})
> +    local cstate = c.state
> +    c:close()
> +    return cstate
> +end;

It looks okay, but we can do it without defining a function.

 | do                      \
 |     local c = <...>     \
 |     local res = c.state \
 |     c:close()           \
 |     return res          \
 | end

Up to you.