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

[Sergey Bronnikov <sergeyb@tarantool.org>]

Hello,

thanks for review!

On 13.12.2020 20:58, Vladislav Shpilevoy wrote:
> 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.

Yep, it's a right branch. Cover letter is here [1], first time it was 
send without ML address.

> 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?

On first iteration I have converted print statements to functions as it 
is required by Python 3.

Leonid asked me to fix code style of strings too and I did it on second 
iteration, see conversation here [2].

Nothing wrong with multi-line prints, but for my taste it looks ugly.

I don't want to have yet another conversation about code style, if you 
are happy with multi-line strings in print - it's fine.

So I reverted changes related to multi-line strings because the main 
purpose of the series is Python 3 support.

>
>> - 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.
You are right. Removed import and force-pushed.
>
>> +
>>   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.
Added newlines.
>
>>   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.

Python2 print automatically adds a whitespace before an argument:

Python 2.7.18 (default, Aug  4 2020, 11:16:42)
[GCC 9.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
 >>> print "X", 3
X 3
 >>> from __future__ import print_function
 >>> print("X{}".format(3))
X3
 >>>

To produce same output as in a .result file additional whitespace has 
been added in a code.

>
>>   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?

I just made using quotes consistent in a Python code. Outer quotes are 
double quote and internal quote is a single one.

--- a/test/box-py/bootstrap.test.py
+++ b/test/box-py/bootstrap.test.py
@@ -1,11 +1,11 @@
-server.admin('box.internal.bootstrap()')
-server.admin('box.space._schema:select{}')
-server.admin('box.space._cluster:select{}')
-server.admin('box.space._space:select{}')
-server.admin('box.space._index:select{}')
-server.admin('box.space._user:select{}')
-server.admin('for _, v in box.space._func:pairs{} do r = {} 
table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end')
-server.admin('box.space._priv:select{}')
+server.admin("box.internal.bootstrap()")
+server.admin("box.space._schema:select{}")
+server.admin("box.space._cluster:select{}")
+server.admin("box.space._space:select{}")
+server.admin("box.space._index:select{}")
+server.admin("box.space._user:select{}")
+server.admin("for _, v in box.space._func:pairs{} do r = {} 
table.insert(r, v:update({{'=', 18, ''}, {'=', 19, ''}})) return r end")
+server.admin("box.space._priv:select{}")

  # Cleanup
  server.stop()

>
>>   ---
>>   - - [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?

Same as in answer to the previous comment. Using single quote inside 
double quotes.

Relevant part of code:

  # Transactions
-test('space:auto_increment({"transaction"})')
-test('space:select{}')
-test('box.begin(), space:auto_increment({"failed"}), box.rollback()')
-test('space:select{}')
-test('require("fiber").sleep(0)')
+test("space:auto_increment({'transaction'})")
+test("space:select{}")
+test("box.begin(), space:auto_increment({'failed'}), box.rollback()")
+test("space:select{}")
+test("require('fiber').sleep(0)")

>>   ---
>>   ...
>>   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.

To fix exceptions when Python 3 used:

[006]   File "box-py/iproto.test.py", line 40, in <module>
[006]     for (k,v) in globals().items():
[006] RuntimeError: dictionary changed size during iteration

>
> I suspect this has something to do with the second commit,
> and therefore belongs to it.
Right, moved to a separate commit.
>
>> +    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 	clge.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?

reverted indentation here and in two lines below:

--- a/test/replication-py/cluster.test.py
+++ b/test/replication-py/cluster.test.py
@@ -30,18 +30,18 @@ replica_uuid = str(uuid.uuid4())
  ## Universal read permission is required to perform JOIN/SUBSCRIBE
  rows = list(server.iproto.py_con.join(replica_uuid))
  status = len(rows) == 1 and rows[0].return_message.find("Read access") 
 >= 0 and \
-        "ok" or "not ok"
+    "ok" or "not ok"
  print("{} - join without read permissions on universe".format(status))
  rows = list(server.iproto.py_con.subscribe(cluster_uuid, replica_uuid))
  status = len(rows) == 1 and rows[0].return_message.find("Read access") 
 >= 0 and \
-        "ok" or "not ok"
+    "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))
  status = len(rows) == 1 and rows[0].return_message.find("Write 
access") >= 0 and \
-        "ok" or "not ok"
+    "ok" or "not ok"
  print("{} - join without write permissions to _cluster".format(status))

  def check_join(msg):

>
>> +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 ".
--- a/test/replication-py/cluster.test.py
+++ b/test/replication-py/cluster.test.py
@@ -240,7 +240,7 @@ try:
  except Exception as e:
      line = "ER_READONLY"
      if failed.logfile_pos.seek_once(line) >= 0:
-        print("\'{}\' exists in server log".format(line))
+        print("'{}' exists in server log".format(line))

  master.admin("box.cfg { read_only = false }")
>
>> -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 ".

Right.

@@ -259,7 +259,7 @@ try:
  except Exception as e:
      line = "ER_REPLICASET_UUID_MISMATCH"
      if failed.logfile_pos.seek_once(line) >= 0:
-        print("\'{}\' exists in server log".format(line))
+        print("'{}' exists in server log".format(line))

  failed.cleanup()

>
> 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.

Done, it was xlog-py/dup_key.test.py.


1. 
https://lists.tarantool.org/pipermail/tarantool-patches/2020-December/021323.html

2. 
https://lists.tarantool.org/pipermail/tarantool-patches/2020-December/021044.html