Re: [Tarantool-patches] [PATCH 1/4] test: convert print to function and make quotes use consistent

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

Finally, http_client.test.lua works on my machine.

I don't see the cover letter, with issue and branch links.
I assume the branch is ligurio/gh-5538-support-python3.

See 12 minor comments below.

On 11.12.2020 09:42, Sergey Bronnikov via Tarantool-patches wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> - convert print statement to function. In a Python 3 'print' becomes a
> function, see [1]. Patch makes 'print' in a regression tests compatible with
> Python 3.
> - according to PEP8, mixing using double quotes and quotes in a project looks
> inconsistent. Patch makes using quotes with strings consistent.> - print multiline strings with multiple print()

1. Why? Is it according to PEP too? The same for '%'.
What is wrong with using multiline strings?

> - use "format()" instead of "%" everywhere
> 1. https://docs.python.org/3/whatsnew/3.0.html#print-is-a-function
> 
> Part of #5538
> ---
> diff --git a/test/box-py/args.test.py b/test/box-py/args.test.py
> index c0fac9038..f1b840a85 100644
> --- a/test/box-py/args.test.py
> +++ b/test/box-py/args.test.py
> @@ -1,3 +1,5 @@
> +from __future__ import print_function

2. In this file I don't see you changing 'print' to 'print()'
anywhere. So why is this line needed? In this file I only see
"print" as a part of strings. As a word. Not as a function.

> +
>  import sys
>  import os
>  import re
> diff --git a/test/box-py/bad_trigger.result b/test/box-py/bad_trigger.result
> index 5d064b764..42cd9a7e8 100644
> --- a/test/box-py/bad_trigger.result
> +++ b/test/box-py/bad_trigger.result
> @@ -1,8 +1,6 @@
> -
>   #
>   # if on_connect() trigger raises an exception, the connection is dropped
>   #
> - 

3. Why is this file changed? It is .result file. Your changes are just
refactoring, so no .result files should be changed.

>  nosuchfunction = nil
>  ---
>  ...
> diff --git a/test/box-py/bad_trigger.test.py b/test/box-py/bad_trigger.test.py
> index 7d200b921..789fe8045 100644
> --- a/test/box-py/bad_trigger.test.py
> +++ b/test/box-py/bad_trigger.test.py
> @@ -24,12 +24,12 @@ conn.connect()
>  s = conn.socket
>  
>  # Read greeting
> -print 'greeting: ', len(s.recv(IPROTO_GREETING_SIZE)) == IPROTO_GREETING_SIZE
> +print("greeting:  {}".format(len(s.recv(IPROTO_GREETING_SIZE)) == IPROTO_GREETING_SIZE))
>  
>  # Read error packet
>  IPROTO_FIXHEADER_SIZE = 5
>  fixheader = s.recv(IPROTO_FIXHEADER_SIZE)
> -print 'fixheader: ', len(fixheader) == IPROTO_FIXHEADER_SIZE
> +print("fixheader:  {}".format(len(fixheader) == IPROTO_FIXHEADER_SIZE))

4. There was a single whitespace. You made it double. Why? The
same for the change above. And in one place below.

>  unpacker.feed(fixheader)
>  packet_len = unpacker.unpack()
>  packet = s.recv(packet_len)
> @@ -38,9 +38,9 @@ unpacker.feed(packet)
>  # Parse packet
>  header = unpacker.unpack()
>  body = unpacker.unpack()
> -print 'error code', (header[IPROTO_CODE] & (REQUEST_TYPE_ERROR - 1))
> -print 'error message: ', body[IPROTO_ERROR]
> -print 'eof:', len(s.recv(1024)) == 0
> +print("error code {}".format((header[IPROTO_CODE] & (REQUEST_TYPE_ERROR - 1))))
> +print("error message:  {}".format(body[IPROTO_ERROR]))
> +print("eof: {}".format(len(s.recv(1024)) == 0))
>  s.close()
>  
>  server.admin("box.session.on_connect(nil, f1)")
> diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
> index 0876e77a6..865302698 100644
> --- a/test/box-py/bootstrap.result
> +++ b/test/box-py/bootstrap.result
> @@ -165,7 +165,7 @@ box.space._user:select{}
>    - [3, 1, 'replication', 'role', {}]
>    - [31, 1, 'super', 'role', {}]
>  ...
> -for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
> +for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{'=', 18, ''}, {'=', 19, ''}})) return r end

5. This is Lua. Not python. Why did you change it?

>  ---
>  - - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
>      false, false, true, ['LUA'], {}, '', '', '']
> diff --git a/test/box-py/call.result b/test/box-py/call.result
> index d340ed6fa..e6b7b8dc9 100644
> --- a/test/box-py/call.result
> +++ b/test/box-py/call.result
> @@ -536,10 +536,10 @@ function f(...) return ... end
>  call f({'k2': 'v2', 'k1': 'v1'})
>  ---
>  - {'k2': 'v2', 'k1': 'v1'}
> -eval (return space:auto_increment({"transaction"}))()
> +eval (return space:auto_increment({'transaction'}))()
>  ---
>  - [1, 'transaction']
> -function f(...) return space:auto_increment({"transaction"}) end
> +function f(...) return space:auto_increment({'transaction'}) end

6. This whole file is Lua output. Why is it changed?

>  ---
>  ...
>  call f()
> diff --git a/test/box-py/iproto.test.py b/test/box-py/iproto.test.py
> index cdd1a71c5..5eccd40d3 100644
> --- a/test/box-py/iproto.test.py
> +++ b/test/box-py/iproto.test.py
> @@ -11,46 +13,42 @@ from lib.tarantool_connection import TarantoolConnection
>  
>  admin("box.schema.user.grant('guest', 'read,write,execute', 'universe')")
>  
> -print """
> -#
> -# iproto packages test
> -#
> -"""
> +print("#")
> +print("# iproto packages test")
> +print("#")
>  
> -# opeing new connection to tarantool/box
> +# opening new connection to tarantool/box
>  conn = TarantoolConnection(server.iproto.host, server.iproto.port)
>  conn.connect()
>  s = conn.socket
>  
> -print """
> -# Test bug #899343 (server assertion failure on incorrect packet)
> -"""
> -print "# send the package with invalid length"
> -invalid_request = struct.pack('<LLL', 1, 4294967290, 1)
> -print s.send(invalid_request)
> -print "# check that is server alive"
> -print iproto.py_con.ping() > 0
> +print("# Test bug #899343 (server assertion failure on incorrect packet)")
> +print("# send the package with invalid length")
> +invalid_request = struct.pack("<LLL", 1, 4294967290, 1)
> +print(s.send(invalid_request))
> +print("# check that is server alive")
> +print(iproto.py_con.ping() > 0)
>  
>  # closing connection
>  s.close()
>  
>  key_names = {}
> -for (k,v) in globals().items():
> -    if type(k) == str and k.startswith('IPROTO_') and type(v) == int:
> +for (k,v) in list(globals().items()):

7. Why did you add list() call? I don't see in the commit
message any mentioning of why is it bad to use items() without
list(). The same below.

I suspect this has something to do with the second commit,
and therefore belongs to it.

> +    if type(k) == str and k.startswith("IPROTO_") and type(v) == int:
>          key_names[v] = k
> @@ -59,36 +57,34 @@ def test(header, body):
>      try:
>          s.send(query)
>      except OSError as e:
> -        print '   => ', 'Failed to send request'
> +        print("   => ", "Failed to send request")
>      c.close()
> -    print iproto.py_con.ping() > 0
> +    print(iproto.py_con.ping() > 0)
>  
> -print """
> -#  Test gh-206 "Segfault if sending IPROTO package without `KEY` field"
> -"""
> +print("#  Test gh-206 'Segfault if sending IPROTO package without `KEY` field'")

8. You changed the output. Previously it had one empty line before and
after the text. The same in some other places in this commit. No .result
files should be changed.

> diff --git a/test/replication-py/cluster.test.py b/test/replication-py/cluster.test.py
> index 088ca9c34..c770a9bc9 100644
> --- a/test/replication-py/cluster.test.py
> +++ b/test/replication-py/cluster.test.py
> @@ -27,56 +29,59 @@ replica_uuid = str(uuid.uuid4())
>  
>  ## Universal read permission is required to perform JOIN/SUBSCRIBE
>  rows = list(server.iproto.py_con.join(replica_uuid))
> -print len(rows) == 1 and rows[0].return_message.find('Read access') >= 0 and \
> -    'ok' or 'not ok', '-', 'join without read permissions on universe'
> +status = len(rows) == 1 and rows[0].return_message.find("Read access") >= 0 and \
> +        "ok" or "not ok"

9. I see, that at least in this file the indentation step is 4
symbols. Why did you change it to 8?

> +print("{} - join without read permissions on universe".format(status))
>  rows = list(server.iproto.py_con.subscribe(cluster_uuid, replica_uuid))
> -print len(rows) == 1 and rows[0].return_message.find('Read access') >= 0 and \
> -    'ok' or 'not ok', '-', 'subscribe without read permissions on universe'
> +status = len(rows) == 1 and rows[0].return_message.find("Read access") >= 0 and \
> +        "ok" or "not ok"
> +print("{} - subscribe without read permissions on universe".format(status))
>  ## Write permission to space `_cluster` is required to perform JOIN
>  server.admin("box.schema.user.grant('guest', 'read', 'universe')")
>  server.iproto.reconnect() # re-connect with new permissions
>  rows = list(server.iproto.py_con.join(replica_uuid))
> -print len(rows) == 1 and rows[0].return_message.find('Write access') >= 0 and \
> -    'ok' or 'not ok', '-', 'join without write permissions to _cluster'
> +status = len(rows) == 1 and rows[0].return_message.find("Write access") >= 0 and \
> +        "ok" or "not ok"> @@ -235,16 +240,16 @@ try:
>  except Exception as e:
>      line = "ER_READONLY"
>      if failed.logfile_pos.seek_once(line) >= 0:
> -        print "'%s' exists in server log" % line
> +        print("\'{}\' exists in server log".format(line))

10. You don't need to escape ' when they are inside of ".

> -master.admin('box.cfg { read_only = false }')
> +master.admin("box.cfg { read_only = false }")
>  
> -print '-------------------------------------------------------------'
> -print 'JOIN replica with different replica set UUID'
> -print '-------------------------------------------------------------'
> +print("-------------------------------------------------------------")
> +print("JOIN replica with different replica set UUID")
> +print("-------------------------------------------------------------")
>  
>  failed = TarantoolServer(server.ini)
> -failed.script = 'replication-py/uuid_mismatch.lua'
> +failed.script = "replication-py/uuid_mismatch.lua"
>  failed.vardir = server.vardir
>  failed.rpl_master = master
>  failed.name = "uuid_mismatch"
> @@ -254,15 +259,15 @@ try:
>  except Exception as e:
>      line = "ER_REPLICASET_UUID_MISMATCH"
>      if failed.logfile_pos.seek_once(line) >= 0:
> -        print "'%s' exists in server log" % line
> +        print("\'{}\' exists in server log".format(line))

11. You don't need to escape ' when they are inside of ".

12. There is also one place, where you removed an empty line in the
end of file. I can't find it again, but it belongs to the last
commit. Please, move it.