Re: [Tarantool-patches] [PATCH v5] test: fix flaky socket test

["Ilya Kosarev" <i.kosarev@tarantool.org>]

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

Hi!

Thanks for the comments.

Sent v6 of the patch considering them.

>Четверг, 26 декабря 2019, 2:43 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:
>
>Now it works like a charm with 30 jobs and mostly works with 50 jobs in
>parallel on my machine. This is the good change.
>
>A few notes below.
>
>WBR, Alexander Turenko.
>
>> Branch:  https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4426-4451-fix-socket-test
>> Issues:  https://github.com/tarantool/tarantool/issues/4426
>>  https://github.com/tarantool/tarantool/issues/4451
>>  https://github.com/tarantool/tarantool/issues/4469
>> @@ -230,7 +230,7 @@ function aexitst(ai, hostnames, port)
>>  end;
>> 
>> 
>> -aexitst( socket.getaddrinfo('localhost', 'http', {  protocol = 'tcp',
>> +aexitst( socket.getaddrinfo('127.0.0.1', 'http', {  protocol = 'tcp',
>>      type = 'SOCK_STREAM'}), {'127.0.0.1', '::1'}, 80 );
>
>This is the test case for getaddrinfo() itself. It is better to check
>here that 'localhost' actually resolved to either 127.0.0.1 or ::1
>(typically both, but it depends of IPv4 / IPv6 support on a particular
>system). 
Right, got too far here.
>
>
>> @@ -546,7 +535,7 @@ socket.getaddrinfo('host', 'port', { flags = 'WRONG' }) == nil and errno() == er
>>  test_run:cmd("setopt delimiter ';'")
>>  f = fiber.create(function()
>>      while true do
>> -        local result = socket.getaddrinfo('localhost', '80')
>> +        local result = socket.getaddrinfo('127.0.0.1', '80')
>>          fiber.sleep(0)
>>      end
>>  end);
>
>Same here.
>
>> @@ -955,17 +964,17 @@ client:write('world')
>>  client:read(5, 5) == 'world'
>>  -- cancel fiber
>>  fiber.cancel(echo_fiber)
>> -client:read(1, 5) == ''
>> +client:read(1, TIMEOUT) == ''
>>  server:close()
>
>It worth to use TIMEOUT constant instead of 5 in two near :read() calls
>above this one:
>
> | client:read(5, 5) == 'hello'
> | client:read(5, 5) == 'world'
>
>There is also other case where it is applicable:
>
> | sc:send('Hello')
> | sc:send(', world')
> | sc:send("\\nnew line")
> | sa:read('\\n', 1)                           -- !!
> | sa:read({ chunk = 1, delimiter = 'ine'}, 1) -- !!
> | sa:read('ine', 1)                           -- !!
> | sa:read('ine', 0.1)
>
>All reads except the last one should use the common TIMEOUT. The last
>one expected to block on 0.1 seconds and then return nil, so don't
>increase timeout here.
>
>And the case right after this one:
>
> | sc:send('Hello, world')
> | sa:read(',', 1)                             -- !!
> | sc:shutdown('W')
> | sa:close()
> | sc:close()
>
>This way the change will be consistent within the file, at least in
>context of :read() calls with a timeout.
>
>Those cases do not fail for me (I don't know why), so it is purely for
>consistency. Existing test cases will show our practices, that's good
>for ones who'll write more cases in a future.
Right, i also didn't see any fails for those reads and decided not to
interfere with them. However i agree that consistency is also important.
>


-- 
Ilya Kosarev

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