[tarantool-patches] Re: [PATCH v2 1/2] test: tunned timeouts and added statuses checks

[Alexander Turenko]

In short: the patch too large to review each individual change (say,
timeouts or wait_cond's).

Also there are changes I doubt about (fast_replica usage for a single
instance creation and fast_replica changes). I see, it was the big
effort, but I don't see gains.

Comments are inlined below.

WBR, Alexander Turenko.

On Thu, Mar 21, 2019 at 09:03:05AM +0300, Alexander V. Tikhonov wrote:
> Tunned timeouts to the common value. Added for status checks the
> the wait_cond loops with diagnostic, changed naming of the tests
> replicas, separated replicas between subtests in tests, changed
> hardcoded replicas creation/deletion to the standalone routine.

Tunned -> tuned. In the commit header too.

> Tunned timeouts to the common value.

How one can review it within this large patch? Need to be factored out
to allow one to look into them.

> Added for status checks the the wait_cond loops with diagnostic.

Ok.

> Changed naming of the tests replicas.

Is it about file names (and test-run instance names)? I think that it
would be better to create one instance.lua or replica.lua (if possible)
and use symlinks. It is not good to duplicate code.

> Separated replicas between subtests in tests.

Is it part of the previous item?

> Changed hardcoded replicas creation/deletion to the standalone
> routine.

It does not become less 'hardcoded' when becomes wrapped. AFAIU, you
wrap 'create server', 'start server' and other test-run's commands into
'fast_replica' even when we start just one server. So complexity of
tests and these wrappes (fast_replica.lua) both were increased. I don't
understand gains of this change.

> @@ -485,26 +481,26 @@ fiber = require('fiber')
>  while box.info.replication[1].upstream.status ~= 'follow' do fiber.sleep(0.0001) end
>  ---
>  ...
> -box.info.replication[1].upstream.status -- follow
> +test_run:wait_cond(function() return box.info.replication[1].upstream.status == 'follow' end) or box.info.replication[1].upstream.status
>  ---
> -- follow
> +- true

'while' loop above do the same thing. It should be removed I guess.

>  ...
>  for i = 0, 15 do fiber.sleep(0.01) if box.info.replication[1].upstream.status ~= 'follow' then break end end
>  ---
>  ...
> -box.info.replication[1].upstream.status -- follow
> +test_run:wait_cond(function() return box.info.replication[1].upstream.status == 'follow' end) or box.info.replication[1].upstream.status

Same here.

> --- a/test/replication/lua/fast_replica.lua
> +++ b/test/replication/lua/fast_replica.lua
> @@ -1,18 +1,23 @@
>  
> -function join(inspector, n)
> -    local path = os.getenv('TARANTOOL_SRC_DIR')
> +function create(inspector, name, replica)
> +    replica = replica or 'replica'

So now name of an instance file can be arbirtary, but different from
names in a test. It will make investigation of a test fail more complex.

>  function unregister(inspector, id)
> -    box.space._cluster:delete{id}
> +    id = id or 2

I understood that you try to simplify 'one instance' case, but this
default looks really counter-intuitive. I don't think that it worth to
use 'fast_replica' for that case as I wrote above.

> +function hibernate(inspector, name, id)
> +    return stop(inspector, name, id) and cleanup(inspector, name, id)
>  end

I have to remember now what is create, start, stop and clean. Enough
terms I think.

> -function delete(inspector, id)
> -    inspector:cmd('stop server replica'..tostring(id - 1))
> -    inspector:cmd('delete server replica'..tostring(id - 1))
> +function prune(inspector, name, id)
> +    return unregister(inspector, id) and drop(inspector, name, id)
> +end

Same here.

> diff --git a/test/replication/suite.ini b/test/replication/suite.ini
> index 6e9e3edd0..aa79f68f3 100644
> --- a/test/replication/suite.ini
> +++ b/test/replication/suite.ini
> @@ -7,5 +7,6 @@ release_disabled = catch.test.lua errinj.test.lua gc.test.lua gc_no_space.test.l
>  config = suite.cfg
>  lua_libs = lua/fast_replica.lua lua/rlimit.lua
>  use_unix_sockets = True
> +use_unix_sockets_iproto = True

It is not mentioned in the commit message. And should be factored out
into its own patch.