From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id EBE9D469719 for ; Tue, 10 Mar 2020 14:35:10 +0300 (MSK) References: <20200219064411.37542-1-arkholga@tarantool.org> <20200304153127.uhssittpe3obggq3@tkn_work_nb> From: Olga Arkhangelskaia Message-ID: Date: Tue, 10 Mar 2020 14:35:09 +0300 MIME-Version: 1.0 In-Reply-To: <20200304153127.uhssittpe3obggq3@tkn_work_nb> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH] test: use grep_log in tests List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.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: ')) > > (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: ')) +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: ')) >> -- 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.