[Tarantool-patches] [PATCH 1/4] test: convert print to function and make quotes use consistent
Leonid Vasiliev
lvasiliev at tarantool.org
Tue Dec 15 13:05:34 MSK 2020
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 at 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
>
>
More information about the Tarantool-patches
mailing list