From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 13 Mar 2019 20:43:34 +0300 From: Alexander Turenko Subject: Re: [tarantool-patches] Re[3]: [PATCH] test: enable parallel for python tests and long Message-ID: <20190313173244.qykmwx5eke6nk3n7@tkn_work_nb> References: <1551340410.442783626@f536.i.mail.ru> <1551875238.777965213@f534.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1551875238.777965213@f534.i.mail.ru> To: Sergei Voronezhskii Cc: tarantool-patches , Konstantin Osipov , Kirill Yukhin , Vladimir Davydov List-ID: I have splitted the patch into two (long and python test suites), made minor changes in the commit message, made minor changes in the code (w/o behaviour changes, see below). Pushed to 2.1. CCed maintainers just in case. WBR, Alexander Turenko. ---- diff --git a/test/box-py/iproto.result b/test/box-py/iproto.result index eb84eaf1c..900e6e24f 100644 --- a/test/box-py/iproto.result +++ b/test/box-py/iproto.result @@ -137,9 +137,9 @@ space:drop() space2:drop() --- ... -box.space._cluster:delete(2) +box.space._cluster:delete{2} ~= nil --- -- [2, '0d5bd431-7f3e-4695-a5c2-82de0a9cbc95'] +- true ... space = box.schema.create_space('gh1280', { engine = 'vinyl' }) --- diff --git a/test/box-py/iproto.test.py b/test/box-py/iproto.test.py index ad788eef4..77637d8ed 100644 --- a/test/box-py/iproto.test.py +++ b/test/box-py/iproto.test.py @@ -321,7 +321,7 @@ c.close() admin("space:drop()") admin("space2:drop()") -admin("box.space._cluster:delete(2)") +admin("box.space._cluster:delete{2} ~= nil") # # gh-1280 Segmentation fault on space.select(tuple()) or space.select([2]) diff --git a/test/replication-py/cluster.test.py b/test/replication-py/cluster.test.py index dcb05a517..b328b1c89 100644 --- a/test/replication-py/cluster.test.py +++ b/test/replication-py/cluster.test.py @@ -289,7 +289,5 @@ print '-------------------------------------------------------------' # Cleanup sys.stdout.pop_filter() - master.admin("box.schema.user.revoke('guest', 'replication')") - master.admin('box.space._cluster:delete{2} ~= nil') On Wed, Mar 06, 2019 at 03:27:18PM +0300, Sergei Voronezhskii wrote: > Hello, I've updated branch, and rebase on 2.1 > > Removed vclock: > diff --git a/test/replication-py/multi.test.py b/test/replication-py/multi.test.py > index 224332266..15d5a409a 100644 > --- a/test/replication-py/multi.test.py > +++ b/test/replication-py/multi.test.py > @@ -55,7 +55,6 @@ for server in cluster: > fiber.sleep(0.01) > end;""", server2.id) > print 'server', server.id, "connected" > - server.admin("box.info.vclock") > > print 'done' > > diff --git a/test/replication-py/multi.result b/test/replication-py/multi.result > index b4f16b699..f90ab22a0 100644 > --- a/test/replication-py/multi.result > +++ b/test/replication-py/multi.result > @@ -17,24 +17,12 @@ Make a full mesh > server 1 connected > server 1 connected > server 1 connected > -box.info.vclock > ---- > -- {1: 4} > -... > server 2 connected > server 2 connected > server 2 connected > -box.info.vclock > ---- > -- {1: 4} > -... > server 3 connected > server 3 connected > server 3 connected > -box.info.vclock > ---- > -- {1: 4} > -... > done > ---------------------------------------------------------------------- > Test inserts > > Fixed typo: > diff --git a/test/replication-py/multi.test.py b/test/replication-py/multi.test.py > index 224332266..15d5a409a 100644 > @@ -84,7 +83,7 @@ print 'Synchronize' > for server1 in cluster: > for server2 in cluster: > server1.wait_lsn(server2.id, server2.get_lsn(server2.id)) > - print 'server', server.id, 'done' > + print 'server', server1.id, 'done' > print 'done' > print > > diff --git a/test/replication-py/multi.result b/test/replication-py/multi.result > index b4f16b699..f90ab22a0 100644 > --- a/test/replication-py/multi.result > +++ b/test/replication-py/multi.result > @@ -54,8 +42,8 @@ Insert records > inserted 60 records > > Synchronize > -server 3 done > -server 3 done > +server 1 done > +server 2 done > server 3 done > done > > Squash wait for whole data: > diff --git a/test/box-py/iproto.test.py b/test/box-py/iproto.test.py > index 81cdddb61..ad788eef4 100644 > --- a/test/box-py/iproto.test.py > +++ b/test/box-py/iproto.test.py > @@ -226,6 +226,10 @@ def receive_response(): > resp_len = s.recv(5) > resp_len = msgpack.loads(resp_len) > resp_headerbody = s.recv(resp_len) > + # wait for the whole data > + while len(resp_headerbody) < resp_len: > + chunk = s.recv(resp_len - len(resp_headerbody)) > + resp_headerbody = resp_headerbody + chunk > unpacker = msgpack.Unpacker(use_list = True) > unpacker.feed(resp_headerbody) > resp_header = unpacker.unpack() > > > >Четверг, 28 февраля 2019, 10:53 +03:00 от Sergei Voronezhskii : > > > > > >Updated branch: https://github.com/tarantool/tarantool/commits/sergw/enable-parallel-test-py-long-fixups > >Test passed: https://travis-ci.org/tarantool/tarantool/builds/499664259 > > > >>Воскресенье, 23 декабря 2018, 14:04 +03:00 от Alexander Turenko < alexander.turenko@tarantool.org >: > >> > >>Hi! > >> > >>See comments below and fixups on the > >>sergw/enable-parallel-test-py-long-fixups branch. > >> > >>I tested it with the command below and after the fixup it passes. > >> > >>``` > >>$ ./test-run.py --long -- $(for i in $(seq 1 10); do echo -n "-py/ "; done) > >>``` > >> > >>WBR, Alexander Turenko. > >> > >>On Tue, Dec 18, 2018 at 11:43:00AM +0300, Sergei Voronezhskii wrote: > >>> Fixed cleanup for python tests: > >>> - box-py/iproto.test.py need to cleanup created cluster > >>> reproduce: > >>> - [box-py/iproto.test.py, null] > >>> - [box-py/bootstrap.test.py, null] > >>> - box-py/boostrap.test.py should not restart server because of next > >>> box-py/call.test.py got error: > >>> `NetworkError: (104, 'Connection reset by peer')` > >>> at `iproto.authenticate('test', 'test')` > >>> reproduce: > >>> - [box-py/iproto.test.py, null] > >>> - [box-py/bootstrap.test.py, null] > >>> - [box-py/call.test.py, null] > >>> - replication-py/multi.test.py should not relay on hardcoded server.id > >> > >>relay -> rely > >fixed > > > >>Why it should not? You cleaned up _cluster in box-py/iproto.test.py. > >>Why don't clean up it in replication-py/cluster.test.py too? > >> > >>> because previous test can create some and `id` will autoincremented, > >>> instead this we need to calculate vclock_diff > > > >>You use hardcoded server id 1. 'previous test can create some' -- some > >>what? I suppose you are about using vclock difference instead of > >>absolute value, but the message states something different. > > > >I guess it's enough for checking with hardcoded server id 1. > >If master==replica1 and master==replica2, it useless to checking > >replica1 and replica2. > > > >Previous test autoincrement replica id and also moves vclock. > > > >> > >>> reproduce: > >>> - [replication-py/cluster.test.py, null] > >>> - [replication-py/multi.test.py, null] > >>> > >>> Part of: #3232 > >>> --- > >>> BRANCH: https://github.com/tarantool/tarantool/tree/sergw/enable-parallel-test-py-long > >>> test/box-py/bootstrap.test.py | 4 +--- > >>> test/box-py/iproto.result | 4 ++++ > >>> test/box-py/iproto.test.py | 1 + > >>> test/box-py/suite.ini | 2 +- > >>> test/engine_long/suite.ini | 2 +- > >>> test/long_run-py/suite.ini | 2 +- > >>> test/luajit-tap/suite.ini | 2 +- > >>> test/replication-py/multi.result | 19 +++++-------------- > >>> test/replication-py/multi.test.py | 28 +++++++++++++++++----------- > >>> test/replication-py/suite.ini | 2 +- > >>> test/xlog-py/suite.ini | 2 +- > >>> 11 files changed, 34 insertions(+), 34 deletions(-) > >>> > >>> diff --git a/test/box-py/bootstrap.test.py b/test/box-py/bootstrap.test.py > >>> index 9d690b03f..dba6f5ae9 100644 > >>> --- a/test/box-py/bootstrap.test.py > >>> +++ b/test/box-py/bootstrap.test.py > >>> @@ -9,8 +9,6 @@ cluster_uuid = yaml.load(server.admin('box.space._schema:get("cluster")', > >>> sys.stdout.push_filter(cluster_uuid, '') > >>> > >>> server.admin('box.internal.bootstrap()') > >>> -server.restart() > >>> - > >> > >>I suppose now the test case will not test what it was intended. Please, > >>implement waiting for the server startup. > >> > >>You should have strong opinion why this will not broke the test case > >>before propose such kind of things in a patch. When you have this > >>opinion it is easy to write it in the commit message. > >> > >>I don't sure whether it really brokes the test case, but I don't see any > >>justification in the commit message. So I need to investigate the > >>problem from scratch to prove it is good or bad. And so what gain we get > >>from your work? > >> > >>The code cost is zero w/o investigation. The investigation result was > >>not shared. I don't understand why do you behave in this way. Again and > >>again. > >> > > > >I have checked upgrade.lua for bootstrap method for investigation. > >But I did not found any clue why we should do restart here. > >To fix it without delete `restart` we can do: > > > >iproto.py_con.ping() > >server.restart() > >iproto.reconnect() > >iproto.authenticate('test', 'test') > > > >But I have exception TarantoolStartError on OSX. > >After while you can see @locker already removed it at > >https://github.com/tarantool/tarantool/commit/3fe8c2835406c02acb59b6f86721d63cec69fac7 > > > >>> server.admin('box.space._schema:select{}') > >>> server.admin('box.space._cluster:select{}') > >>> server.admin('box.space._space:select{}') > >>> @@ -20,4 +18,4 @@ server.admin('box.space._func:select{}') > >>> server.admin('box.space._priv:select{}') > >>> > >>> # Cleanup > >>> -sys.stdout.pop_filter() > >>> +sys.stdout.clear_all_filters() > >> > >>Two filters was pushed, two should be popped, I think. > >> > >>> diff --git a/test/replication-py/multi.test.py b/test/replication-py/multi.test.py > >>> index 224332266..3394d47be 100644 > >>> --- a/test/replication-py/multi.test.py > >>> +++ b/test/replication-py/multi.test.py > >>> @@ -10,10 +10,14 @@ ROW_N = REPLICA_N * 20 > >>> > >>> # master server > >>> master = server > >>> + > >>> master.admin("fiber = require('fiber')") > >>> master.admin("box.schema.user.grant('guest', 'replication')") > >>> master.admin("box.schema.user.grant('guest', 'execute', 'universe')") > >>> > >>> +# Get vclock on master > >>> +vclock_cnt = yaml.load(master.admin("box.info.vclock[1]", silent = True))[0] > >>> + > >>> print '----------------------------------------------------------------------' > >>> print 'Bootstrap replicas' > >>> print '----------------------------------------------------------------------' > >>> @@ -48,14 +52,16 @@ for server in cluster: > >>> server.iproto.py_con.eval("box.cfg { replication = ... }", [sources]) > >>> > >>> # Wait connections to establish > >>> -for server in cluster: > >>> +for sid, server in enumerate(cluster, 1): > >>> for server2 in cluster: > >>> server.iproto.py_con.eval(""" > >>> while #box.info.vclock[...] ~= nil do > >>> fiber.sleep(0.01) > >>> end;""", server2.id) > >>> - print 'server', server.id, "connected" > >>> - server.admin("box.info.vclock") > >>> + print 'server', sid, "connected" > >>> + vclock_new = yaml.load(server.admin("box.info.vclock[1]", silent = True))[0] > >>> + print "vclock_diff: {}".format(vclock_new - vclock_cnt) > >>> + > >>> > >> > >>Re server.id -> sid: Please, cleanup _cluster in > >>replication-py/cluster.test.py instead. The same for all such changes > >>below. > >Added > > > >diff --git a/test/replication-py/cluster.result b/test/replication-py/cluster.result > >index 3b5566829..04f06f74e 100644 > >--- a/test/replication-py/cluster.result > >+++ b/test/replication-py/cluster.result > >@@ -196,6 +196,10 @@ box.info.vclock[2] == 2 > > --- > > - true > > ... > >+box.space._cluster:delete{2} ~= nil > >+--- > >+- true > >+... > > ------------------------------------------------------------- > > JOIN replica to read-only master > > ------------------------------------------------------------- > >@@ -216,3 +220,7 @@ Cleanup > > box.schema.user.revoke('guest', 'replication') > > --- > > ... > >+box.space._cluster:delete{2} ~= nil > >+--- > >+- true > >+... > >diff --git a/test/replication-py/cluster.test.py b/test/replication-py/cluster.test.py > >index b5f59f40f..dcb05a517 100644 > >--- a/test/replication-py/cluster.test.py > >+++ b/test/replication-py/cluster.test.py > >@@ -239,6 +239,7 @@ replica.admin('box.info.vclock[%d] == 2' % replica_id) > > > > replica.stop() > > replica.cleanup() > >+master.admin('box.space._cluster:delete{%d} ~= nil' % replica_id) > > > > print '-------------------------------------------------------------' > > print 'JOIN replica to read-only master' > >@@ -290,3 +291,5 @@ print '-------------------------------------------------------------' > > sys.stdout.pop_filter() > > > > master.admin("box.schema.user.revoke('guest', 'replication')") > >+ > >+master.admin('box.space._cluster:delete{2} ~= nil') > > > >> > >>Re vclocks: Why this vector clock difference is a scalar value? Why it > >>is always for server id 1? Please, describe the problem first. It is > >>that cluster.test.py moves vclock? I think we can just don't print it, > >>because it was informational thing. > > > >Yes it because of cluster.test.py moves vclock. I've updated commit message > > > > > >Fixed cleanup for python tests: > >- replication-py/cluster.test.py > >- box-py/iproto.test.py > >  need to cleanup created cluster > >  reproduce: > >  - [box-py/iproto.test.py, null] > >  - [box-py/bootstrap.test.py, null] > >- replication-py/multi.test.py > >  1) should not rely on hardcoded server.id because previous test can > >     create replica and `id` will autoincremented, instead enumerate > >     through cluster list > >  2) because of cluster.test.py moves vclock need to check for vclock > >     difference instead of printing `box.info.vclock` > >  reproduce: > >  - [replication-py/cluster.test.py, null] > >  - [replication-py/multi.test.py, null] > > > >Part of: #3232 > > > >-- > >Sergei Voronezhskii > > > > > -- > Sergei Voronezhskii