[tarantool-patches] [PATCH 1/1] test: fix flaky swim/errinj.test.lua

[tarantool-patches] [PATCH 1/1] test: fix flaky swim/errinj.test.lua

[Vladislav Shpilevoy]

In one place that test sends a packet and expects that it has
arrived two lines below. Under high load it may take more time.
The patch makes the test explicitly wait for the packet arrival.

Closes #4392
---
Issue: https://github.com/tarantool/tarantool/issues/4392
Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4392-swim-flaky-test

 test/swim/errinj.result   | 3 +++
 test/swim/errinj.test.lua | 1 +
 2 files changed, 4 insertions(+)

diff --git a/test/swim/errinj.result b/test/swim/errinj.result
index efeab87a8..cf26b0a4a 100644
--- a/test/swim/errinj.result
+++ b/test/swim/errinj.result
@@ -60,6 +60,9 @@ errinj.set("ERRINJ_SWIM_FD_ONLY", false)
 ---
 - ok
 ...
+while s1_view:incarnation().version ~= 2 do fiber.sleep(0.01) end
+---
+...
 s1_view:payload()
 ---
 - null
diff --git a/test/swim/errinj.test.lua b/test/swim/errinj.test.lua
index cf0cb2dd4..6c73f9f3d 100644
--- a/test/swim/errinj.test.lua
+++ b/test/swim/errinj.test.lua
@@ -26,6 +26,7 @@ s1:self():incarnation()
 errinj.set("ERRINJ_SWIM_FD_ONLY", true)
 s1:probe_member(s2:self():uri())
 errinj.set("ERRINJ_SWIM_FD_ONLY", false)
+while s1_view:incarnation().version ~= 2 do fiber.sleep(0.01) end
 s1_view:payload()
 s1_view:incarnation()
 
-- 
2.20.1 (Apple Git-117)

[tarantool-patches] Re: [PATCH 1/1] test: fix flaky swim/errinj.test.lua

[Alexander Turenko]

LGTM. Kirill, please, proceed.

On Tue, Jul 30, 2019 at 09:34:35PM +0200, Vladislav Shpilevoy wrote:
> In one place that test sends a packet and expects that it has
> arrived two lines below. Under high load it may take more time.
> The patch makes the test explicitly wait for the packet arrival.
> 
> Closes #4392
> ---
> Issue: https://github.com/tarantool/tarantool/issues/4392
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4392-swim-flaky-test
> 
>  test/swim/errinj.result   | 3 +++
>  test/swim/errinj.test.lua | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/test/swim/errinj.result b/test/swim/errinj.result
> index efeab87a8..cf26b0a4a 100644
> --- a/test/swim/errinj.result
> +++ b/test/swim/errinj.result
> @@ -60,6 +60,9 @@ errinj.set("ERRINJ_SWIM_FD_ONLY", false)
>  ---
>  - ok
>  ...
> +while s1_view:incarnation().version ~= 2 do fiber.sleep(0.01) end
> +---
> +...
>  s1_view:payload()
>  ---
>  - null
> diff --git a/test/swim/errinj.test.lua b/test/swim/errinj.test.lua
> index cf0cb2dd4..6c73f9f3d 100644
> --- a/test/swim/errinj.test.lua
> +++ b/test/swim/errinj.test.lua
> @@ -26,6 +26,7 @@ s1:self():incarnation()
>  errinj.set("ERRINJ_SWIM_FD_ONLY", true)
>  s1:probe_member(s2:self():uri())
>  errinj.set("ERRINJ_SWIM_FD_ONLY", false)
> +while s1_view:incarnation().version ~= 2 do fiber.sleep(0.01) end

Nit: Maybe it will be a bit better to stop after a certain time period
if a condition is not meet after, say, a minute: this will allow
test-run to continue with the test and possibly show more compact
miscompare fail and also will allow continue with other tests that was
already scheduled to the same worker. We use test_run:wait_cond() for
this. Anyway, this is a thing of preference and I don't push anyone to
one or another way: this message it just to let you know about both ways
and the difference between them. (From the other hand explicit is better
then implicit.)

>  s1_view:payload()
>  s1_view:incarnation()
>  
> -- 
> 2.20.1 (Apple Git-117)
> 

[tarantool-patches] Re: [PATCH 1/1] test: fix flaky swim/errinj.test.lua

[Vladislav Shpilevoy]

On 31/07/2019 00:44, Alexander Turenko wrote:
> LGTM. Kirill, please, proceed.
> 
> On Tue, Jul 30, 2019 at 09:34:35PM +0200, Vladislav Shpilevoy wrote:
>> In one place that test sends a packet and expects that it has
>> arrived two lines below. Under high load it may take more time.
>> The patch makes the test explicitly wait for the packet arrival.
>>
>> Closes #4392
>> ---
>> Issue: https://github.com/tarantool/tarantool/issues/4392
>> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4392-swim-flaky-test
>>
>>  test/swim/errinj.result   | 3 +++
>>  test/swim/errinj.test.lua | 1 +
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/test/swim/errinj.result b/test/swim/errinj.result
>> index efeab87a8..cf26b0a4a 100644
>> --- a/test/swim/errinj.result
>> +++ b/test/swim/errinj.result
>> @@ -60,6 +60,9 @@ errinj.set("ERRINJ_SWIM_FD_ONLY", false)
>>  ---
>>  - ok
>>  ...
>> +while s1_view:incarnation().version ~= 2 do fiber.sleep(0.01) end
>> +---
>> +...
>>  s1_view:payload()
>>  ---
>>  - null
>> diff --git a/test/swim/errinj.test.lua b/test/swim/errinj.test.lua
>> index cf0cb2dd4..6c73f9f3d 100644
>> --- a/test/swim/errinj.test.lua
>> +++ b/test/swim/errinj.test.lua
>> @@ -26,6 +26,7 @@ s1:self():incarnation()
>>  errinj.set("ERRINJ_SWIM_FD_ONLY", true)
>>  s1:probe_member(s2:self():uri())
>>  errinj.set("ERRINJ_SWIM_FD_ONLY", false)
>> +while s1_view:incarnation().version ~= 2 do fiber.sleep(0.01) end
> 
> Nit: Maybe it will be a bit better to stop after a certain time period
> if a condition is not meet after, say, a minute: this will allow
> test-run to continue with the test and possibly show more compact
> miscompare fail and also will allow continue with other tests that was
> already scheduled to the same worker. We use test_run:wait_cond() for
> this. Anyway, this is a thing of preference and I don't push anyone to
> one or another way: this message it just to let you know about both ways
> and the difference between them. (From the other hand explicit is better
> then implicit.)

Such places are widespread in our tests. Perhaps it is worth introducing
a test-run helper for that. For example:

    test_run:wait(function() ... end, timeout)

The function should return true, when the condition is met. This case
would look like this:

    test_run:wait(function()                                \
        return s1_view:incarnation().version == 2           \
    end, 60)

>>  s1_view:payload()
>>  s1_view:incarnation()
>>  
>> -- 
>> 2.20.1 (Apple Git-117)
>>

[tarantool-patches] Re: [PATCH 1/1] test: fix flaky swim/errinj.test.lua

[Alexander Turenko]

> >> diff --git a/test/swim/errinj.test.lua b/test/swim/errinj.test.lua
> >> index cf0cb2dd4..6c73f9f3d 100644
> >> --- a/test/swim/errinj.test.lua
> >> +++ b/test/swim/errinj.test.lua
> >> @@ -26,6 +26,7 @@ s1:self():incarnation()
> >>  errinj.set("ERRINJ_SWIM_FD_ONLY", true)
> >>  s1:probe_member(s2:self():uri())
> >>  errinj.set("ERRINJ_SWIM_FD_ONLY", false)
> >> +while s1_view:incarnation().version ~= 2 do fiber.sleep(0.01) end
> > 
> > Nit: Maybe it will be a bit better to stop after a certain time period
> > if a condition is not meet after, say, a minute: this will allow
> > test-run to continue with the test and possibly show more compact
> > miscompare fail and also will allow continue with other tests that was
> > already scheduled to the same worker. We use test_run:wait_cond() for
> > this. Anyway, this is a thing of preference and I don't push anyone to
> > one or another way: this message it just to let you know about both ways
> > and the difference between them. (From the other hand explicit is better
> > then implicit.)
> 
> Such places are widespread in our tests. Perhaps it is worth introducing
> a test-run helper for that. For example:
> 
>     test_run:wait(function() ... end, timeout)
> 
> The function should return true, when the condition is met. This case
> would look like this:
> 
>     test_run:wait(function()                                \
>         return s1_view:incarnation().version == 2           \
>     end, 60)

I have mentioned it above: test_run:wait_cond(func, timeout, delay).
https://github.com/tarantool/test-run/blob/947c65cda7f26ceb5f601bbf154e70f9c3f755e9/test_run.lua#L387

[tarantool-patches] Re: [PATCH 1/1] test: fix flaky swim/errinj.test.lua

[Kirill Yukhin]

Hello,

On 30 Jul 21:34, Vladislav Shpilevoy wrote:
> In one place that test sends a packet and expects that it has
> arrived two lines below. Under high load it may take more time.
> The patch makes the test explicitly wait for the packet arrival.
> 
> Closes #4392
> ---
> Issue: https://github.com/tarantool/tarantool/issues/4392
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4392-swim-flaky-test

I've checked the patch into 2.2 and master.

--
Regards, Kirill Yukhin

[tarantool-patches] Re: [PATCH 1/1] test: fix flaky swim/errinj.test.lua

[Alexander Turenko]

On Fri, Aug 09, 2019 at 01:18:34PM +0300, Kirill Yukhin wrote:
> Hello,
> 
> On 30 Jul 21:34, Vladislav Shpilevoy wrote:
> > In one place that test sends a packet and expects that it has
> > arrived two lines below. Under high load it may take more time.
> > The patch makes the test explicitly wait for the packet arrival.
> > 
> > Closes #4392
> > ---
> > Issue: https://github.com/tarantool/tarantool/issues/4392
> > Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4392-swim-flaky-test
> 
> I've checked the patch into 2.2 and master.

I don't see this commit in 2.2.