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

[Leonid Vasiliev <lvasiliev@tarantool.org>]

Hi! I looked at the commit 1f34abf6e059532ddfa097e4c1a86030d34341a9 on
the branch, because not all the diff has been attached here (or I can't
find them).
Not all multi-line prints were reverted. Let's finish the revert.
As for the rest of the changes:"I think this patch can be considered
complete." (LGTM)


On 14.12.2020 14:44, Sergey Bronnikov via Tarantool-patches wrote:
> 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 
> 
>