Re: [Tarantool-patches] [PATCH 2/4] replication: add advanced tests for sync replication

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

On 08/07/2020 14:07, Sergey Bronnikov wrote:
> On 22:57 Tue 07 Jul , Vladislav Shpilevoy wrote:
>>>>> test_run:switch('default')
>>>>> box.cfg{replication_synchro_quorum=NUM_INSTANCES, replication_synchro_timeout=0.1}
>>>>> _ = box.schema.space.create('sync', {is_sync=true, engine=engine})
>>>>> _ = box.space.sync:create_index('pk')
>>>>> -- Testcase body.
>>>>> box.space.sync:insert{1}
>>>>> box.space.sync:insert{2}
>>>>> box.space.sync:insert{3}
>>>>> box.space.sync:select{} -- 1, 2, 3
>>>>> test_run:switch('replica')
>>>>> box.space.sync:select{} -- 1, 2, 3
>>>>> -- Testcase cleanup.
>>>>> test_run:switch('default')
>>>>> box.space.sync:drop()
>>>>>
>>>>> -- Synchro timeout is not bigger than replication_synchro_timeout value.
>>>>> -- Testcase setup.
>>>>> test_run:switch('default')
>>>>> box.cfg{replication_synchro_quorum=BROKEN_QUORUM, replication_synchro_timeout=orig_synchro_timeout}
>>>>> _ = box.schema.space.create('sync', {is_sync=true, engine=engine})
>>>>> _ = box.space.sync:create_index('pk')
>>>>> -- Testcase body.
>>>>> start = os.time()
>>>>> box.space.sync:insert{1}
>>>>> (os.time() - start) == box.cfg.replication_synchro_timeout -- true
>>>>
>>>> 7. Очень плохая идея. Если процесс подвиснет тут ненадолго, то эта проверка
>>>> упадет. Не должно быть тестов, которые полагаются на то, что процесс будет
>>>> выполняться стабильно.
>>>
>>> Ты предлагаешь не проверять или есть более надежные способы проверки,
>>> что таймаут именно такой величины, каким его выставили?
>>
>> Если тебе надо проверить, что таймаут провалился, то надо проверять,
>> что прошедшее время >= timeout, но точно не == timeout. Второе очень
>> ненадежно.
>>
> 
> Мне не нравится эта проверка, потому что тест должен проверять, что "timeout
> not bigger than replication_synchro_timeout value".
> Сделал так:
> 
> box.space.sync:insert{1}
> -(os.time() - start) == box.cfg.replication_synchro_timeout -- true
> +-- We assume that the process may freeze and the timeout will be slightly
> +-- larger than the set value.
> +POSSIBLE_ERROR = 2
> +(os.time() - start) < box.cfg.replication_synchro_timeout + POSSIBLE_ERROR -- true
>  -- Testcase cleanup.

Во-первых, твой тест как раз проверяет падение таймаута. Потому что ты
пытаешься писать с BROKEN_QUORUM. Так что проверка походу неверна.

Во-вторых, даже если бы кворум был, то хак с добавкой нескольких секунд
тоже не дает гарантий, а значит тест становится flaky. Не надо так делать,
пожалуйста.

>>>>> test_run:cmd("setopt delimiter ';'")
>>>>> _ = fiber.create(function()
>>>>>     box.space.sync:insert{2}
>>>>> end);
>>>>> test_run:cmd("setopt delimiter ''");
>>>>> -- Disable synchronous mode.
>>>>> disable_sync_mode()
>>>>> -- Space is in async mode now.
>>>>> box.space.sync:insert{3} -- async operation must wait sync one
>>>>> box.error.injection.set('ERRINJ_SYNC_TIMEOUT', false)
>>>>> box.space.sync:select{} -- 1
>>>>> test_run:cmd('switch replica')
>>>>> box.space.sync:select{} -- 1
>>>>> -- Testcase cleanup.
>>>>> test_run:switch('default')
>>>>> box.space.sync:drop()
>>>>>
>>>>> -- Warn user when setting `replication_synchro_quorum` to a value
>>>>> -- greater than number of instances in a cluster, see gh-5122.
>>>>> box.cfg{replication_synchro_quorum=BROKEN_QUORUM} -- warning
>>>>
>>>> 13. Этот тест походу вообще ничего не проверяет. Варнинг сейчас не пишется,
>>>> и тест проходит.
>>>
>>> Обычный процесс такой: если тест падает, то, пока есть открытая
>>> проблема, добавляют XFAIL и при изменении поведения XFAIL меняется на
>>> XPASS, чтобы убрать XFAIL.  У нас нет такого механизма, поэтому добавил
>>> тест на будущее и когда варнинг добавят, то тест сломается и result файл
>>> обновят. Мне кажется, что это вполне себе ок.
>>
>> Это было бы ок, если бы коммент говорил, что тест пока невалидный. Но что
>> еще важнее - тест все равно пройдет даже когда добавится ворнинг. Потому
>> что он пойдет в лог, и в выводе теста его не будет. Так что тест пройдет,
>> хоть и не должен.
> 
> У нас автоматические тесты с бинарным статусом PASS или FAIL и человек
> обычно смотрит результат выполнения тестов, а не комментарии в
> исходнике. Поэтому это не сильно меняет дело. Но я обновил комментарий:
> 
>  -- greater than number of instances in a cluster, see gh-5122.
>  -box.cfg{replication_synchro_quorum=BROKEN_QUORUM} -- warning
>  +box.cfg{replication_synchro_quorum=BROKEN_QUORUM} -- expected warning, to be add in gh-5122

Ты видимо не прочитал, что я написал(. Печать ворнинга не изменит вообще
ничего. Ворнинги - это логи, они не идут в дифф. Они будут в лог файле,
а не в выводе теста. И этот тест все равно пройдет.