[tarantool-patches] Re: test: switched on pretest_clean routine
Alexander Turenko
alexander.turenko at tarantool.org
Mon Mar 11 08:26:54 MSK 2019
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" <avtikhon at tarantool.org>
> 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?
More information about the Tarantool-patches
mailing list