From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 5A081469710 for ; Wed, 18 Nov 2020 12:18:34 +0300 (MSK) 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> <20201116164006.fw3jwes4dwbx7nsd@tkn_work_nb> From: Sergey Bronnikov Message-ID: <69928974-0f4d-6093-e169-368e02a9d1b0@tarantool.org> Date: Wed, 18 Nov 2020 12:18:32 +0300 MIME-Version: 1.0 In-Reply-To: <20201116164006.fw3jwes4dwbx7nsd@tkn_work_nb> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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: tarantool-patches@dev.tarantool.org Hi, Alexander! thanks for review! I have updated the whole patch and I'll send it as reply to this mail. Below are my answers regarding each your comment. On 16.11.2020 19:40, Alexander Turenko wrote: > 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). Yes, it was renamed as you suggested previous time, but not everywhere. Fixed it. +++ b/lib/options.py @@ -210,9 +210,8 @@ class Options:                  Default: false.""")          parser.add_argument( -                "--snapshot-path", +                "--snapshot",                  dest='snapshot_path', -                default=None,                  help="""Path to a Tarantool snapshot that will be                  loaded before testing.""") @@ -241,17 +240,23 @@ class Options:                  color_stdout(format_str.format(op1, op2), schema='error')                  check_error = True -        if getattr(self, 'disable_schema_upgrade', '') and \ -                not (self, 'snapshot_path', ''): -            color_stdout("option --disable-schema-upgrade \ -                depends on --snapshot-version\n", schema='error') +        snapshot_path = getattr(self.args, 'snapshot_path', '') +        if self.args.disable_schema_upgrade and not snapshot_path: +            color_stdout("option --disable-schema-upgrade requires --snapshot\n", +                        schema='error')              check_error = True > > (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. > Function test_debug() implemented in utils.py and called in lib.tarantool_server.py. Functions test_debug() and test_get_option() are moved from lib/app_server.py. @@ -182,17 +174,3 @@ class AppServer(Server):                                       test_suite.ini))          test_suite.tests = tests - -    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 >> + 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. Fixed. @@ -241,17 +240,23 @@ class Options:                  color_stdout(format_str.format(op1, op2), schema='error')                  check_error = True -        if getattr(self, 'disable_schema_upgrade', '') and \ -                not (self, 'snapshot_path', ''): -            color_stdout("option --disable-schema-upgrade \ -                depends on --snapshot-version\n", schema='error') +        snapshot_path = getattr(self.args, 'snapshot_path', '') +        if self.args.disable_schema_upgrade and not snapshot_path: +            color_stdout("option --disable-schema-upgrade requires --snapshot\n", +                        schema='error')              check_error = True -        if getattr(self, 'snapshot_path', ''): -            snapshot_path = getattr(self, 'snapshot_path', '') +        if snapshot_path:              if not os.path.exists(snapshot_path): -                color_stdout("path {} not exist\n", snapshot_path, schema='error') +                color_stdout("Path {} not exists\n".format(snapshot_path), schema='error')                  check_error = True +            else: +                self.args.snapshot_path = os.path.abspath(snapshot_path)          if check_error:              exit(-1) BTW code with checking conflicting options fixed too in a separate patch. > > Fourth, the first message contains extra whitespaces: > > | option --disable-schema-upgrade depends on --snapshot-version -        if getattr(self, 'disable_schema_upgrade', '') and \ -                not (self, 'snapshot_path', ''): -            color_stdout("option --disable-schema-upgrade \ -                depends on --snapshot-version\n", schema='error') +        snapshot_path = getattr(self.args, 'snapshot_path', '') +        if self.args.disable_schema_upgrade and not snapshot_path: +            color_stdout("option --disable-schema-upgrade requires --snapshot\n", +                        schema='error')              check_error = True > 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*'.) Thanks! Missed it. Format string uses {}, but method "format()" with argument is missed. Fixed. -                color_stdout("path {} not exist\n", snapshot_path, schema='error') +               color_stdout("Path {} not exists\n".format(snapshot_path), schema='error')                  check_error = True > > 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. Fixed! @@ -241,17 +240,23 @@ class Options:                  color_stdout(format_str.format(op1, op2), schema='error')                  check_error = True -        if getattr(self, 'disable_schema_upgrade', '') and \ -                not (self, 'snapshot_path', ''): -            color_stdout("option --disable-schema-upgrade \ -                depends on --snapshot-version\n", schema='error') +        snapshot_path = getattr(self.args, 'snapshot_path', '') +        if self.args.disable_schema_upgrade and not snapshot_path: +            color_stdout("option --disable-schema-upgrade requires --snapshot\n", +                        schema='error')              check_error = True -        if getattr(self, 'snapshot_path', ''): -            snapshot_path = getattr(self, 'snapshot_path', '') +        if snapshot_path:              if not os.path.exists(snapshot_path): -                color_stdout("path {} not exist\n", snapshot_path, schema='error') +                color_stdout("Path {} not exists\n".format(snapshot_path), schema='error')                  check_error = True +            else: +                self.args.snapshot_path = os.path.abspath(snapshot_path)          if check_error:              exit(-1) > >> -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.      DEFAULT_INSPECTOR = 0      TEST_RUN_DIR = os.path.abspath(os.path.join(os.path.dirname(__file__),                                                  "..")) +    # assert(false) hangs due to gh-4983, added fiber.sleep(0) to workaround  it      DISABLE_AUTO_UPGRADE = "require('fiber').sleep(0) \          assert(box.error.injection.set('ERRINJ_AUTO_UPGRADE', true) == 'ok', \          'no such errinj')" >> - corecls = __import__(cls.mdlname, >> - fromlist=cls.clsname).__dict__[cls.clsname] >> + corecls = __import__(cls.mdlname, fromlist=cls.clsname).__dict__[cls.clsname] > Unrelated change. Reverted back. >> + 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. checks for arguments moved to __init__.py and options.py. --- a/lib/__init__.py +++ b/lib/__init__.py @@ -6,6 +6,7 @@ from lib.options import Options  from lib.tarantool_server import TarantoolServer  from lib.unittest_server import UnittestServer  from utils import warn_unix_sockets_at_start +from utils import test_debug  __all__ = ['Options'] @@ -56,6 +57,9 @@ def module_init():      TarantoolServer.find_exe(args.builddir)      UnittestServer.find_exe(args.builddir) +    is_debug = test_debug(TarantoolServer().version()) +    Options().check_schema_upgrade_option(is_debug) + lib.options.py: @@ -241,17 +240,23 @@ class Options:                  color_stdout(format_str.format(op1, op2), schema='error')                  check_error = True -        if getattr(self, 'disable_schema_upgrade', '') and \ -                not (self, 'snapshot_path', ''): -            color_stdout("option --disable-schema-upgrade \ -                depends on --snapshot-version\n", schema='error') +        snapshot_path = getattr(self.args, 'snapshot_path', '') +        if self.args.disable_schema_upgrade and not snapshot_path: +            color_stdout("option --disable-schema-upgrade requires --snapshot\n", +                        schema='error')              check_error = True -        if getattr(self, 'snapshot_path', ''): -            snapshot_path = getattr(self, 'snapshot_path', '') +        if snapshot_path:              if not os.path.exists(snapshot_path): -                color_stdout("path {} not exist\n", snapshot_path, schema='error') +                color_stdout("Path {} not exists\n".format(snapshot_path), schema='error')                  check_error = True +            else: +                self.args.snapshot_path = os.path.abspath(snapshot_path)          if check_error:              exit(-1) + +    def check_schema_upgrade_option(self, is_debug): +        if self.args.disable_schema_upgrade and not is_debug: +            color_stdout("Can't disable schema upgrade on release build\n", schema='error') +            exit(-1) > >> 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. Fixed in updated patch. In short: code with copying snapshot moved from base class Server to AppServer and TarantoolServer classes. Code in both classes are similar but I think it is no sense to add a common function for it. >> 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. Fixed in a whole patch. >> @@ -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. Reverted change back. >> @@ -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? arguments validation code moved to __init__.py and options.py. > > 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. There is a similar condition in method print_exe():     @classmethod     def print_exe(cls):         color_stdout('Installing the server ...\n', schema='serv_text')         if cls.binary:             color_stdout('    Found executable   at ', schema='serv_text')             color_stdout(cls.binary + '\n', schema='path')         if cls.ctl_path:             color_stdout('    Found tarantoolctl at ', schema='serv_text')             color_stdout(cls.ctl_path + '\n', schema='path') So I kept change as is. > >> + 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. all checks for arguments moved to options.py and Exceptions replaced with "exit(1)". > >> @@ -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. Reverted change back. > >> @@ -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. Reverted change back.