Re: [tarantool-patches] Re[3]: [PATCH] test: enable parallel for python tests and long

[Alexander Turenko <alexander.turenko@tarantool.org>]

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 <sergw@tarantool.org>:
> >
> >
> >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, '<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