From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 8041127B3D for ; Mon, 11 Mar 2019 01:26:59 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 1_JPouP32hzN for ; Mon, 11 Mar 2019 01:26:59 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B451221112 for ; Mon, 11 Mar 2019 01:26:58 -0400 (EDT) Date: Mon, 11 Mar 2019 08:26:54 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: test: switched on pretest_clean routine Message-ID: <20190311052653.ocq7luhdanj5wz2f@tkn_work_nb> References: <1551862069.344113893@f454.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1551862069.344113893@f454.i.mail.ru> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Alexander Tikhonov Cc: tarantool-patches Please, use git format-patch and git send-email to produce plain text patches. It is easier to comment and, despite we have a copy on a branch, allows to apply a patch right for an email. I think that it worth to add a detailed description of the feature into test-run's README, because it is hard to sell the feature to developers if the behaviour is not clear. I had filed https://github.com/tarantool/test-run/issues/147 on it and then decided to do it myself, see PR #148. So no actions are needed from you :) cfg_filter() is used in two test: box/admin.test.lua and box.cfg.test.lua. The latter seems to supersedes the case in admin.test.lua, so I think the case in admin.test.lua can be removed and the cfg_filter() function can be added right into cfg.test.lua. Please, verify this with one of maintainers and do. I also dig a bit deeper and found the commit where the 'print box.cfg, box.cfg{}, print box.cfg' case was introduced: 4bb7d70a06. It seems that 'must be read-only' comment should be under 'box.cfg.nosuchoption = 1' :) WBR, Alexander Turenko. On Wed, Mar 06, 2019 at 11:47:49AM +0300, Alexander Tikhonov wrote: > > test: switched on pretest_clean routine > > Switched on pretest_clean routine which helps to cleanup > the test environment before test start. Also moved the global > functions in box suite into the test_helpers module, to make > it possible to use it in each of the test while global functions > are removing with pretest_clean routine. > > --- > Github pull request: https://github.com/tarantool/tarantool/pull/4029   For the record, branch: gh-3863-runtest-clean. > Travis-CI: https://travis-ci.org/tarantool/tarantool/builds/502431423   > > From acbd981d79fe7b2ad5cdf5c2c1e9c1d3613d89c9 Mon Sep 17 00:00:00 2001 > From: "Alexander V. Tikhonov" > Date: Wed, 6 Mar 2019 10:17:47 +0300 > Subject: [PATCH] test: switched on pretest_clean routine > > Switched on pretest_clean routine which helps to cleanup > the test environment before test start. Also moved the global > functions in box suite into the test_helpers module, to make > it possible to use it in each of the test while global functions > are removing with pretest_clean routine. You are mention gh-3863 in the branch name, but don't mention it in the commit message. Whether the issue fixed by your commit? If so, please add 'Fixes #3863' or 'Closes #3863' and the end of the commit message. > diff --git a/test/box/admin.test.lua b/test/box/admin.test.lua > index 4e7a78f5b..353afea9c 100644 > --- a/test/box/admin.test.lua > +++ b/test/box/admin.test.lua > @@ -6,7 +6,8 @@ space = box.schema.space.create('tweedledum') >  index = space:create_index('primary') >   >  help() > -cfg_filter(box.cfg) > +test_helpers = require('test_helpers') > +test_helpers.cfg_filter(box.cfg) >  space:insert{1, 'tuple'} >  box.snapshot() >  space:delete{1} Nit: I prefer to have all requires at top of a file when possible. > diff --git a/test/box/lua/test_helpers.lua b/test/box/lua/test_helpers.lua > new file mode 100644 > index 000000000..bb231e73b > --- /dev/null > +++ b/test/box/lua/test_helpers.lua > > <...> > > +function compare(a,b) > +    return a[1] < b[1] > +end It would be better to make this function local: -function compare(a,b) +local function compare(a, b) But I think it should be part of a test, see below. > + > +function test_helpers.sorted(data) > +    table.sort(data, compare) > +    return data > +end > + It is used only in box/sql.test.lua. I think it is better to just add it to the test. It is almost one-liner. > diff --git a/test/engine_long/delete_insert.result b/test/engine_long/delete_insert.result > index b1d504271..2cc46486a 100644 > --- a/test/engine_long/delete_insert.result > +++ b/test/engine_long/delete_insert.result > @@ -1,3 +1,6 @@ > +el_mod = require("suite") You are use three names for this module: 'el_mod' for a variable, 'suite' for a file and 'engine_long_module' inside the suite.lua file. I prefer to use one where possible. I would use the name 'suite' everywhere, because all places where it is used are in the scope of the test suite and the name should not confuse anybody. I would also do the following in corresponding tests: ``` suite = require('suite') delete_insert = suite.delete_insert ``` ``` suite = require('suite') delete_replace_update = delete_replace_update ``` This will decrease the diff much. Also I think it worth to ask Vladimir why these function were factored out from corresponding tests. Maybe it worth to moved them into the tests?