From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Date: Sun, 21 Oct 2018 23:41:02 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Subject: Re: [PATCH v2 1/5] test: cleanup replication tests
Message-ID: <20181021204102.z7aoyfcjqzoj45om@tkn_work_nb>
References: <20181019161721.49560-1-sergw@tarantool.org>
 <20181019161721.49560-2-sergw@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20181019161721.49560-2-sergw@tarantool.org>
To: Sergei Voronezhskii <sergw@tarantool.org>
Cc: tarantool-patches@freelists.org, Vladimir Davydov <vdavydov.dev@gmail.com>
List-ID: <tarantool-patches.dev.tarantool.org>

Ok except minor comments below.

It would be glab if you'll answer to review messages, cite reviewer
comments and answer what was changed or why it didn't changed. It is
hard to manually track all questions were raised, esp. when the patch is
relatively big.

For small changes (eps. for a relatively big patch) you can just answer
for comments. Ideally also attach diff from the previous version at end
of the email (just call `git diff` before commit your changes and
paste). I can see the whole diff on the branch and paste related hunks
to comment it. Other reviewers can ask you to paste the whole new diff
explicitly, I personally prefer to run git diff and paste things I want
to comment.

WBR, Alexander Turenko.

On Fri, Oct 19, 2018 at 07:17:17PM +0300, Sergei Voronezhskii wrote:
> - at the end of tests which create any replication config need to call:
>   * `test_run:cmd('delete server ...')` removes server object
> from `TestState.servers` list, this behaviour was taken
> from `test_run:drop_cluster()` function

Please, indent it like so (for ease reading):

- xxx
  * yyy
    zzz
    aaa
- fff
  eee
  * ggg
    hhh

> diff --git a/test/replication/local_spaces.test.lua b/test/replication/local_spaces.test.lua
> index 06e2b0bd2..633cc9f1a 100644
> --- a/test/replication/local_spaces.test.lua
> +++ b/test/replication/local_spaces.test.lua
> @@ -76,6 +76,7 @@ box.space.test3:select()
>  test_run:cmd("switch default")
>  test_run:cmd("stop server replica")
>  test_run:cmd("cleanup server replica")
> +test_run:cleanup_cluster()
>  box.schema.user.revoke('guest', 'replication')
>  
>  s1:select()

Missed test_run:cmd("delete server replica")?