[tarantool-patches] Re: test: switched on pretest_clean routine

[Alexander Turenko <alexander.turenko@tarantool.org>]

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@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?