Re: [Tarantool-patches] [PATCH v11 8/8] test: add replication/applier-rollback

[Cyrill Gorcunov <gorcunov@gmail.com>]

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