Re: [Tarantool-patches] [PATCH] test: use grep_log in tests

[Olga Arkhangelskaia <arkholga@tarantool.org>]

Hi, Alexander!

I have fixed things you have pointed out and pushed it to branch

https://github.com/tarantool/tarantool/tree/OKriw/refactoring_of_panic_on_broken_lsn.test.lua

The branch can not be tested without

https://github.com/tarantool/test-run/tree/OKriw/gh-196-Extend-grep_log-for-crashed-instances

04.03.2020 18:31, Alexander Turenko пишет:
> I like the simplification of the test you made. Please, consider several
> comments below.
>
> Please, see test-run patch comments first (see [1]), because I proposed
> to rename the option from `path` to `filename`.
>
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/014638.html
>
> WBR, Alexander Turenko.
>
>> test: use grep_log in tests

[PATCH] tests: use grep_log opt.filename in panic_on_broken_lsn

Now we use filename option from grep_log instead of custom log
search.

> Nit: I would make the commit header a bit more certain and point the
> test name right here.
>
>> Refactoring: replace grep_for_broken_lsn with grep_log from test_run.
>> ---
>>   test/xlog/panic_on_broken_lsn.result   | 50 +++++++++-----------------
>>   test/xlog/panic_on_broken_lsn.test.lua | 29 ++++-----------
>>   2 files changed, 22 insertions(+), 57 deletions(-)
>>   -- Check that log contains the mention of broken LSN and the request printout
>> -grep_broken_lsn(fio.pathjoin(fio.cwd(), 'panic.log'), 1)
>> +path = fio.pathjoin(fio.cwd(), 'panic.log')
>> +---
>> +...
>> +str = "LSN for 1 is used twice or COMMIT order is broken: confirmed: 1, new: 1"
>>   ---
>> -- '{type: ''REPLACE'', replica_id: 1, space_id: 272, index_id: 0, tuple: ["t0", "v1"]}'
>> +...
> I would keep the request information as proof that we found the log
> entry generated by the errinj above and not something else. May be
> implemented like so:
>
>   | path = fio.pathjoin(fio.cwd(), 'replica.log')
>   | str = string.format("LSN for 1 is used twice or COMMIT order is broken: confirmed: %d, new: %d, req: .*", lsn, lsn)
>   | found = test_run:grep_log(nil, str, 256, {path = path})
>   | (found:gsub('^.*, req: ', ''):gsub('lsn: %d+', 'lsn: <lsn>'))
>
> (See below re 256.)
>
> (Parenthesis are to discard the second return value of string.gsub().)

+filename = fio.pathjoin(fio.cwd(), 'panic.log')
+str = string.format("LSN for 1 is used twice or COMMIT order is broken: 
confirmed: 1, new: 1, req: .*")
+found = test_run:grep_log(nil, str, 256, {filename = filename})
+(found:gsub('^.*, req: ', ''):gsub('lsn: %d+', 'lsn: <lsn>'))


+filename = fio.pathjoin(fio.cwd(), 'replica.log')
+str = string.format("LSN for 1 is used twice or COMMIT order is broken: 
confirmed: %d, new: %d, req: .*", lsn, lsn)
+found = test_run:grep_log(nil, str, 256, {filename = filename})
+(found:gsub('^.*, req: ', ''):gsub('lsn: %d+', 'lsn: <lsn>'))

>>   -- Check that log contains the mention of broken LSN and the request printout
>> -grep_broken_lsn(fio.pathjoin(fio.cwd(), 'panic.log'), 1)
>> -
>> +path = fio.pathjoin(fio.cwd(), 'panic.log')
>> +str = "LSN for 1 is used twice or COMMIT order is broken: confirmed: 1, new: 1"
>> +test_run:grep_log(nil, str, nil, {path = path})~= nil
> Nit: spaces are needed around '~='.
>
> I would keep 265 bytes window (there is `bytes` option in grep_log()).
> Lesser value is more safe: if the expected log entry is not appear we
> can occasionally found the entry from another test if the window is big
> enough for this. It means that the test may pass even when is should
> not. So it is good to keep the window small.