From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E651C469710 for ; Tue, 2 Jun 2020 14:14:10 +0300 (MSK) Date: Tue, 2 Jun 2020 14:13:51 +0300 From: Alexander Turenko Message-ID: <20200602111351.fctlh3aly3euuo4m@tkn_work_nb> References: <20200330144823.GA55948@pony.bronevichok.ru> <20200515161727.jtlqplcdkbbcsgh2@tkn_work_nb> <20200528093934.GA19447@pony.bronevichok.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200528093934.GA19447@pony.bronevichok.ru> Subject: Re: [Tarantool-patches] [test-run] Add options for upgrade testing List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Bronnikov Cc: o.piskunov@tarantool.org, tarantool-patches@dev.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