From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 CA19B469719 for ; Mon, 16 Nov 2020 19:40:00 +0300 (MSK) Date: Mon, 16 Nov 2020 19:40:06 +0300 From: Alexander Turenko Message-ID: <20201116164006.fw3jwes4dwbx7nsd@tkn_work_nb> References: <20200330144823.GA55948@pony.bronevichok.ru> <20200515161727.jtlqplcdkbbcsgh2@tkn_work_nb> <20200528093934.GA19447@pony.bronevichok.ru> <20200602111351.fctlh3aly3euuo4m@tkn_work_nb> <20200610112919.GA78626@pony.bronevichok.ru> <20201110095546.GA3880@pony.bronevichok.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201110095546.GA3880@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: tarantool-patches@dev.tarantool.org On Tue, Nov 10, 2020 at 12:55:46PM +0300, Sergey Bronnikov wrote: > Hi, Alexander! > > I just want to ask you to take a look on updated patch again. > I have fixed code according to your comments and also > fixed a bug when path for snapshot copying was invalid. > > Branch: https://github.com/tarantool/test-run/tree/ligurio/gh-4801-add-snapshot-option > CI: https://travis-ci.com/github/tarantool/test-run/builds/199375391 > > On 14:29 Wed 10 Jun , Sergey Bronnikov wrote: > > Hi, Hi! Sorry for the delay. I looked over the code and didn't perform any testing of the changes. I hope the success path works as expected, however there are certainly broken error paths and there are code hunks for test-run.py parameters validation, which should not spread far away from options.py. See details below. WBR, Alexander Turenko. > Option '--snapshot-version' specifies a path to snapshot > that will be loaded in Tarantool before testing. I would name it --snapshot, because the argument of the option is path to a snapshot, not a version of some kind. Ouch, I see, it is --snapshot-path now, but the commit message does not reflect this change. --snapshot-path is okay for me too (despite that personally I would prefer --snapshot). (lib/app_server.py) > + def test_option_get(self, option_list_str, silent=False): > + args = [self.binary] + shlex.split(option_list_str) > + if not silent: > + print " ".join([os.path.basename(self.binary)] + args[1:]) > + output = subprocess.Popen(args, > + stdout=subprocess.PIPE, > + stderr=subprocess.STDOUT).stdout.read() > + return output > + > + def test_debug(self): > + if re.findall(r"-Debug", self.test_option_get("-V", True), re.I): > + return True > + return False I don't see a reason to duplicate the code, it is already present in TarantoolServer. We run all tests against the same tarantool. We should move test-run.py parameters validation code out from the *Server classes and the need to have this code in AppServer will gone. > + if getattr(self, 'disable_schema_upgrade', '') and \ > + not (self, 'snapshot_path', ''): > + color_stdout("option --disable-schema-upgrade \ > + depends on --snapshot-version\n", schema='error') > + check_error = True > + > + if getattr(self, 'snapshot_path', ''): > + snapshot_path = getattr(self, 'snapshot_path', '') > + if not os.path.exists(snapshot_path): > + color_stdout("path {} not exist\n", snapshot_path, schema='error') > + check_error = True First, it does not work at all as well as the validation for conflicting --gdb, --lldb and so options above. It tries to obtain values from `self` instead of `self.args`. Second, there is no reason to use getattr() for options that have default values. Third, there is no --snapshot-version option anymore. Fourth, the first message contains extra whitespaces: | option --disable-schema-upgrade depends on --snapshot-version The second one is printed in two lines without leading newline character: | path {} not exist | fooalex@tkn_work_nb test-run ((df4a0e8...) *) (It should be 'not exist*s*'.) This code would not work even if getattr() from `self` would do, because of missed getattr(): `not (self, 'snapshot_path', '')`. It is also strange to use '' as default value for a boolean option. > -from lib.colorer import color_stdout > +from lib.colorer import color_stdout, color_log > +from lib.options import Options Nit: Most of imports are function-per-line. I would keep the style instead of randomly using one or another. > + DISABLE_AUTO_UPGRADE = "require('fiber').sleep(0) \ > + assert(box.error.injection.set('ERRINJ_AUTO_UPGRADE', true) == 'ok', \ > + 'no such errinj')" You and me knows why fiber.sleep() is here. Nobody else will understand this code. Please, refer the relevant issue and mention that it is workaround for it. > - corecls = __import__(cls.mdlname, > - fromlist=cls.clsname).__dict__[cls.clsname] > + corecls = __import__(cls.mdlname, fromlist=cls.clsname).__dict__[cls.clsname] Unrelated change. > + self.disable_schema_upgrade = Options().args.disable_schema_upgrade > + if self.snapshot_path: > + self.snapshot_path = os.path.abspath(self.snapshot_path) > + if os.path.exists(self.snapshot_path): > + self.snapshot_basename = os.path.basename(self.snapshot_path) > + else: > + color_stdout("\nSnapshot {} have been not found\n".format(self.snapshot_path), > + schema='error') I don't understand this logic. If there is no requested snapshot file, we'll print the warning and continue? In my understanding we should give an error and stop, if an incorrect option is passed. And parameters verification should not be grounded somewhere in unrelated code. Preferably it should be in options.py. If some test-run's facilities are used to verify the option, it should be somewhere after options parsing in some top-level code. Maybe in module_init(). However here we just verify whether the file exists, so let's move to options.py. > def install(self, binary=None, vardir=None, mem=None, silent=True): > - pass > + if self.snapshot_basename: > + (instance_name, _) = os.path.splitext(os.path.basename(self.script)) > + instance_dir = os.path.join(self.vardir, instance_name) > + if not os.path.exists(instance_dir): > + os.mkdir(instance_dir) > + snapshot_dest = os.path.join(instance_dir, "00000000000000000000.snap") > + color_log("Copying snapshot {} to {}\n".format( > + self.snapshot_path, snapshot_dest)) > + shutil.copy(self.snapshot_path, snapshot_dest) We assume tarantoolctl layout (as it is defined in test/.tarantoolctl): | vardir/ | + instance_name/ | + *.{snap,xlog} | + instance_name.log | + instance_name.control It is not relevant for AppServer and UnittestServer. > from lib.preprocessor import TestState > from lib.server import Server > from lib.test import Test > -from lib.utils import find_port > +from lib.utils import find_port, extract_schema_from_snapshot > from lib.utils import format_process > from lib.utils import signame > from lib.utils import warn_unix_socket Nit: Most of imports are function-per-line, I would keep it this way. It looks more accurate. > @@ -609,7 +609,6 @@ def __init__(self, _ini=None, test_suite=None): > } > ini.update(_ini) > Server.__init__(self, ini, test_suite) > - self.testdir = os.path.abspath(os.curdir) > self.sourcedir = os.path.abspath(os.path.join(os.path.basename( > sys.argv[0]), "..", "..")) > self.name = "default" I see that it is moved to the parent class, but: 1. It does not look related to your changes, but if it is related, you should describe a reason in the commit message. 2. If it is a side refactoring, you should move it to its own commit with explained motivation. For now, I don't see any reason for this change. > @@ -641,6 +640,10 @@ def __init__(self, _ini=None, test_suite=None): > if 'test_run_current_test' in caller_globals.keys(): > self.current_test = caller_globals['test_run_current_test'] > > + if self.disable_schema_upgrade and not self.test_debug(): > + color_stdout("Can't disable schema upgrade on release build\n", > + schema='error') > + Should not we verify it on the upper level, because it is like options validation? Say, right in options.py or at least in module_init()? Should not we give an error and don't start testing in the case? BTW, there is `debug` property, which calls test_debug(). > @@ -861,6 +873,18 @@ def start(self, silent=True, wait=True, wait_load=True, rais=True, args=[], > self.admin = CON_SWITCH[self.tests_type]('localhost', port) > self.status = 'started' > > + if not self.ctl_path: > + self.ctl_path = self.find_exe(self.builddir) 1. Are there situations, when there is no `ctl_path` value at starting a server? So `prepare_args()` call above is incorrect? I don't get the trick. I guess `lib/__init__.py` call to `find_exe()` should be enough to fill `cls.ctl_path`. 2. `find_exe()` sets `cls.ctl_path` on its own. > + if self.disable_schema_upgrade: > + version = extract_schema_from_snapshot(self.ctl_path, > + self.builddir, self.snapshot_path) > + current_ver = yaml.safe_load(self.admin.execute( > + 'box.space._schema:get{"version"}')) > + if version == current_ver: > + raise_msg = 'Versions are inequal ({} vs {})'.format( > + version, current_ver) > + raise Exception(raise_msg) > + AFAIS, the usual recommendation is to raise standard or user-defined Exception subclasses. It seems, RuntimeError fits best here. However, I guess, RuntimeError and TarantoolStartError will lead to different behaviour. The former just fails the test, but the latter stops testing at whole. Should not we stop testing in the case? Will the new server left running in the case? It is undesirable situation. > @@ -1066,7 +1090,6 @@ def test_option_get(self, option_list_str, silent=False): > if not silent: > print " ".join([os.path.basename(self.binary)] + args[1:]) > output = subprocess.Popen(args, > - cwd=self.vardir, > stdout=subprocess.PIPE, > stderr=subprocess.STDOUT).stdout.read() > return output 1. It looks unrelated. 2. There is not any described reason for this change. > @@ -1126,7 +1149,7 @@ def is_correct(run_name): > def get_param(self, param=None): > if param is not None: > return yaml.safe_load(self.admin("box.info." + param, > - silent=True))[0] > + silent=True))[0] > return yaml.safe_load(self.admin("box.info", silent=True)) Unrelated change.