[Tarantool-patches] [PATCH 1/4] test: convert print to function and make quotes use consistent
Sergey Bronnikov
sergeyb at tarantool.org
Mon Dec 14 14:44:20 MSK 2020
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