From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gorcunov@gmail.com>
Received: from mail-lf1-f65.google.com (mail-lf1-f65.google.com
 [209.85.167.65])
 (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id E810F4696C3
 for <tarantool-patches@dev.tarantool.org>;
 Tue,  7 Apr 2020 13:55:08 +0300 (MSK)
Received: by mail-lf1-f65.google.com with SMTP id f8so1966463lfe.12
 for <tarantool-patches@dev.tarantool.org>;
 Tue, 07 Apr 2020 03:55:08 -0700 (PDT)
Date: Tue, 7 Apr 2020 13:55:05 +0300
From: Cyrill Gorcunov <gorcunov@gmail.com>
Message-ID: <20200407105505.GD3072@uranus>
References: <20200404161524.7466-1-gorcunov@gmail.com>
 <20200404161524.7466-9-gorcunov@gmail.com>
 <AED406E3-B951-422A-8204-77A2E13A2462@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <AED406E3-B951-422A-8204-77A2E13A2462@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v11 8/8] test: add
 replication/applier-rollback
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Serge Petrenko <sergepetrenko@tarantool.org>
Cc: tml <tarantool-patches@dev.tarantool.org>

On Tue, Apr 07, 2020 at 01:26:34PM +0300, Serge Petrenko wrote:
> > test/replication/applier-rollback.test.lua  |  81 ++++++++++
> 
> It’s a bugfix, so please name the test gh-4730-something-something.test.lua

OK, will do.

> > --- /dev/null
> > +++ b/test/replication/applier-rollback-slave.lua
> 
> Better name it replica_applier_rollback.lua for the sake of consistency
> with other instance file names.

ok. I don't have any preference on naming, will do.

> > +
> > +errinj = box.error.injection
> 
> You don’t need the errinj on master, only on replica, AFAICS.

yea, sorry, thanks!

> 
> > +engine = test_run:get_cfg('engine')
> 
> Why test both engines? I suggest you run the test with no arguments.
> (you’ll have to add a line to replication/suite.cfg for that)

Will take a look, thanks!

> > +--
> > +-- To make sure we're running
> > +box.info.status
> > +
> 
> This should always evaluate to ‘running’, you’ve just inserted some
> values and got a result back, so I’d omit this check.

Indeed, thanks!

> > +--
> > +-- Cleanup
> > +test_run:cmd("stop server replica_slave")
> > +test_run:cmd("delete server replica_slave")
> > +box.cfg{replication=""}
> 
> You didn’t set box.cfg.replication, so you shouldn’t reset it.

+1

	Cyrill