From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp10.mail.ru (smtp10.mail.ru [94.100.181.92]) (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 0DCAC469710 for ; Wed, 10 Jun 2020 14:30:32 +0300 (MSK) Date: Wed, 10 Jun 2020 14:29:19 +0300 From: Sergey Bronnikov Message-ID: <20200610112919.GA78626@pony.bronevichok.ru> References: <20200330144823.GA55948@pony.bronevichok.ru> <20200515161727.jtlqplcdkbbcsgh2@tkn_work_nb> <20200528093934.GA19447@pony.bronevichok.ru> <20200602111351.fctlh3aly3euuo4m@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200602111351.fctlh3aly3euuo4m@tkn_work_nb> 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: Alexander Turenko Cc: o.piskunov@tarantool.org, tarantool-patches@dev.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