Re: [Tarantool-patches] [test-run] Add options for upgrade testing

[Sergey Bronnikov <sergeyb@tarantool.org>]

Hi,

On 14:13 Tue 02 Jun , Alexander Turenko wrote:
> (Just looked at answers, don't see the new version of the patch for
> now.)
> 
> On Thu, May 28, 2020 at 12:39:34PM +0300, Sergey Bronnikov wrote:
> > Alexander,
> > 
> > thanks for review. See my comments inline.
> > All changes on top of branch.
> 
> It is ligurio/gh-4801-add-snapshot-option, right? I see some 'tmp',
> 'temp' commits. Please, squash fixups if they are not intended to land
> as separate commits into master.

Sorry, forgot to put branch in order. Now there is a single commit on
top of branch and it is ready for review.

> > > > diff --git a/lib/options.py b/lib/options.py
> > > > index 8bacb4a..d627eb2 100644
> > > > --- a/lib/options.py
> > > > +++ b/lib/options.py
> > > > @@ -201,6 +201,20 @@ class Options:
> > > >                  help="""Run the server under 'luacov'.
> > > >                  Default: false.""")
> > > >  
> > > > +        parser.add_argument(
> > > > +                "--snapshot-version",
> > > > +                dest='snapshot_version',
> > > > +                default=None,
> > > > +                help="""Version of Tarantool snapshot loaded before
> > > > +                testing.""")
> > > 
> > > I would accept a path to a snapshot file here. This way does not require
> > > knowledge from a user where test-run expect snapshots to be placed (and
> > > that '.snap' should not be added). It also more flexible: a user may
> > > choose where it is better to place snapshots within its project (or even
> > > outside it).
> > 
> > Don't forget we need a way to run upgrade testing for several tarantool
> > versions automatically. With your suggestion we should specify one
> > option several times and I don't like this approach, because it will
> > overload command line for running tests.
> 
> For several snapshots? Each is a separate ./test-run.py run I guess. The
> option (as it is shown in the snippet above) does not allow to be
> specified several times (it would use `nargs='*'` if it would do).
> However I don't sure that specifying this option several times would be
> intuitive: this way ./test-run.py should run the whole testing several
> times with different snapshots. Usually test-run.py run testing only
> once.
> 
> Just example, I want to verify starting from a snapshot using existing
> tests on my own commit:
> 
>  | $ (cd test && for s in snapshots/*.snap; do ./test-run.py --snapshot $s; done)
> 
> It does not work. Ouch. Okay...
> 
>  | $ (cd test && for s in snapshots/*.snap; do ./test-run.py s=${s#snapshots/} --snapshot-verson ${s%.snap}; done)
> 
> Doable. Now I got some snapshot from a client and want to answer whether
> it is safe to update to a new tarantool version.
> 
>  | $ (cd test && ./test-run.py --snapshot ../0000<...>1234.snap)
> 
> Ugh, again, I need some additional actions:
> 
>  | $ mv 0000<...>1234.snap test/snapshots
>  | $ (cd test && ./test-run.py --snapshot-version 0000<...>1234)
> 
> Doable too. But do you find it convenient?

Discussed it orally and decided to make an option that accept path to a snapshot file.
Implemented in a branch.

> > > BTW, CI (`make lint`) show errors:
> > > https://travis-ci.com/github/tarantool/test-run/builds/156536350
> > 
> > Most warnings fixed.
> 
> Aren't it is better to keep CI green and fix them all? Or lift the limit
> up and keep CI green? I vote for the former, but okay, any way is
> acceptable.

Fixed all warnings and now CI is green.

> > > > +
> > > > +    def extract_schema_from_snapshot(self):
> > > 
> > > Nit: We can make it a class method.
> > 
> > What do you mean? It's already a class method.
> 
> @classmethod

function moved to another place and no sense to make it as classmethod.

Sergey