[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)
>>