[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1512: DISPATCH-2327: clean up link routes at end of tests
jiridanek commented on a change in pull request #1512: URL: https://github.com/apache/qpid-dispatch/pull/1512#discussion_r804047423 ## File path: tests/system_tests_multi_tenancy.py ## @@ -229,6 +231,17 @@ def test_16_two_router_anonymous_sender_tenant_on_both(self): test.run() self.assertIsNone(test.error) +def _cleanup_link_routes(func): Review comment: > Yeah in the end I'm punting on the decorator approach: too many issues with the python linters. If the only problem is the linter, that should not be hard to put right. Put the decorator as a standalone top level method in the module, and the warning you got should be gone! You were not relying on it being an instance method anyway. ```python def _cleanup_link_routes(func): @functools.wraps(func) def wrapper(self, *args, **kwargs): result = func(self, *args, **kwargs) # TODO something here return result return wrapper class TestClass(unittest.TestCae): def __init__(self, a=1, b=2): @decorate def test_something(self): pass ``` I'm not sure what other problems there might be with it. I'd have to actually try it, and I don't want to poke into tests that I don't understand. -- 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
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1512: DISPATCH-2327: clean up link routes at end of tests
jiridanek commented on a change in pull request #1512:
URL: https://github.com/apache/qpid-dispatch/pull/1512#discussion_r803833609
##
File path: tests/system_tests_multi_tenancy.py
##
@@ -683,12 +704,8 @@ def __init__(self, first_host, second_host, first_address,
second_address, dynam
self.n_settled = 0
def timeout(self):
-self.error = "Timeout Expired: n_sent=%d n_rcvd=%d n_settled=%d" %
(self.n_sent, self.n_rcvd, self.n_settled)
-self.first_conn.close()
-self.second_conn.close()
-self.lookup_conn.close()
-if self.poll_timer:
-self.poll_timer.cancel()
+self.fail("Timeout Expired: n_sent=%d n_rcvd=%d n_settled=%d" %
+ (self.n_sent, self.n_rcvd, self.n_settled))
Review comment:
@kgiusti Actually, you don't even have `self.fail` available here! This
is instance of `proton.handlers.MessagingHandler`, not `unittest.TestCase`. (At
least, If I look right).
--
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
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1512: DISPATCH-2327: clean up link routes at end of tests
jiridanek commented on a change in pull request #1512: URL: https://github.com/apache/qpid-dispatch/pull/1512#discussion_r803827248 ## File path: tests/system_tests_multi_tenancy.py ## @@ -229,6 +231,17 @@ def test_16_two_router_anonymous_sender_tenant_on_both(self): test.run() self.assertIsNone(test.error) +def _cleanup_link_routes(func): Review comment: The 'traditional' way of using `unittest` would be to do your `setUp()` and `tearDown()` in these two methods. (With classes being understood as a way to group together tests that share setup and teardown.) I'd probably try to put the teardown into `LinkRouteTest` itself... but that does not have access to `self.routers`, so it is not such a good idea. Maybe add method `def tearDown(routers)` to it and call it in every test... Decorators are too much magic for my simple tastes, I guess. It's a matter of preference. The decorator is not too hard to understand... And if I had unlimited time, enthusiasm, and money to redesign this test "the best way", I would not actually know what to do with it right away... So, lgtm, just go ahead, I guess? -- 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
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1512: DISPATCH-2327: clean up link routes at end of tests
jiridanek commented on a change in pull request #1512:
URL: https://github.com/apache/qpid-dispatch/pull/1512#discussion_r803820024
##
File path: tests/system_tests_multi_tenancy.py
##
@@ -683,12 +704,8 @@ def __init__(self, first_host, second_host, first_address,
second_address, dynam
self.n_settled = 0
def timeout(self):
-self.error = "Timeout Expired: n_sent=%d n_rcvd=%d n_settled=%d" %
(self.n_sent, self.n_rcvd, self.n_settled)
-self.first_conn.close()
-self.second_conn.close()
-self.lookup_conn.close()
-if self.poll_timer:
-self.poll_timer.cancel()
+self.fail("Timeout Expired: n_sent=%d n_rcvd=%d n_settled=%d" %
+ (self.n_sent, self.n_rcvd, self.n_settled))
Review comment:
All the other tests scrupulously avoid throwing in the Proton handlers.
I presume there was/is a good reason for it. (Other tests have the `error`
field, and always do `self.assertIsNone(test.error)` at the end). So, is
throwing actually alright?
--
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
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1512: DISPATCH-2327: clean up link routes at end of tests
jiridanek commented on a change in pull request #1512:
URL: https://github.com/apache/qpid-dispatch/pull/1512#discussion_r803813675
##
File path: tests/system_tests_multi_tenancy.py
##
@@ -229,6 +231,17 @@ def
test_16_two_router_anonymous_sender_tenant_on_both(self):
test.run()
self.assertIsNone(test.error)
+def _cleanup_link_routes(func):
+"""Wait for all link routes to clean up before exiting the test
+"""
+@wraps(func)
+def wrapper(self, *args, **kwargs):
+func(self, *args, **kwargs)
+self.routers[0].wait_address_unsubscribed("D0.0.0.0/link")
+self.routers[1].wait_address_unsubscribed("D0.0.0.0/link")
+return wrapper
+
+@_cleanup_link_routes # type: ignore
Review comment:
```
77:
/home/jdanek/repos/qpid/qpid-dispatch/tests/system_tests_multi_tenancy.py:255:
error: Argument 1 to "_cleanup_link_routes" has incompatible type
"Callable[[RouterTest], Any]"; expected "RouterTest" [arg-type]
```
That is actually legit. Your `def _cleanup_link_routes(func):` is an
instance method in a class, so the first argument should be `self`. So the
types should be fixed.
--
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
