jiridanek commented on a change in pull request #1516:
URL: https://github.com/apache/qpid-dispatch/pull/1516#discussion_r805142665
##
File path: tests/system_tests_http1_adaptor.py
##
@@ -800,15 +799,37 @@ def _read_until_empty(self, sock, timeout):
break # timeout
return data
+def check_logs(self, prefix, log_file):
+# check router log for proper block/unblock activity
+block_ct = 0
+unblock_ct = 0
+block_line = 0
+unblock_line = 0
+line_no = 0
+with io.open(log_file) as f:
Review comment:
```suggestion
with open(log_file) as f:
```
That is just alias for the built in `open`, using it through the module name
is IMO unusual. And `io.open == open` evaluates to True
##
File path: tests/system_tests_http1_adaptor.py
##
@@ -800,15 +799,37 @@ def _read_until_empty(self, sock, timeout):
break # timeout
return data
+def check_logs(self, prefix, log_file):
+# check router log for proper block/unblock activity
Review comment:
```suggestion
"""Check router log for proper block/unblock activity"""
```
Makes it render in a pleasing green, instead of indifferent gray for me (in
PyCharm).
##
File path: tests/system_tests_http1_adaptor.py
##
@@ -905,7 +918,7 @@ def test_02_backpressure_server(self):
client_sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
Review comment:
The leaking sockets probably don't matter too much in this test. A
leaking router would be a bigger problem.
##
File path: tests/system_tests_http1_adaptor.py
##
@@ -951,18 +964,8 @@ def test_02_backpressure_server(self):
server_listener.shutdown(socket.SHUT_RDWR)
server_listener.close()
-# search the router log file to verify Q2 was hit
-
-block_ct = 0
-unblock_ct = 0
-with io.open(self.INT_A.logfile_path) as f:
-for line in f:
-if 'server link blocked on Q2 limit' in line:
-block_ct += 1
-if 'server link unblocked from Q2 limit' in line:
-unblock_ct += 1
-self.assertTrue(block_ct > 0)
-self.assertEqual(block_ct, unblock_ct)
+router.teardown()
+self.check_logs("server", router.logfile_path)
Review comment:
Since there is one server started and stopped in every test method, I'd
put the creation into `def setUp` and teardown into `tearDown` methods.
e.g.
```python
def tearDown(self)
router.teardown()
self.check_logs("server", router.logfile_path)
super().tearDown()
```
The way it is now, if test fails, router is not stopped.
If we ever decided to switch to exclusively to pytest, then qdrouterd could
be managed as pytest fixture
(https://docs.pytest.org/en/7.0.x/explanation/fixtures.html).
##
File path: tests/system_tests_http1_adaptor.py
##
@@ -800,15 +799,37 @@ def _read_until_empty(self, sock, timeout):
break # timeout
return data
+def check_logs(self, prefix, log_file):
+# check router log for proper block/unblock activity
+block_ct = 0
+unblock_ct = 0
+block_line = 0
+unblock_line = 0
+line_no = 0
+with io.open(log_file) as f:
+for line in f:
Review comment:
```suggestion
for line_no, line in enumerate(f, start=1):
```
##
File path: tests/system_tests_http1_adaptor.py
##
@@ -905,7 +918,7 @@ def test_02_backpressure_server(self):
client_sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
Review comment:
```suggestion
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as client_sock
```
or do the `try/finally` around it, to ensure that socket does not leak if
the test fails before socket is closed.
Context manager (`with`) is IMO preferrable, when it can be used. Here you
have three sockets, so it does not work all that well (requires three levels of
nesting the `with`s). I'd go with `with` here, after all.
(I realize this is old code, so I understand if you don't want to change it
more than necessary.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
