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

[Alexander Turenko <alexander.turenko@tarantool.org>]

(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.

> > > 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?

> > 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.

> > > +
> > > +    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