-
Notifications
You must be signed in to change notification settings - Fork 17
Various Error path handling tests and fixes. #66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The build failed the first time due to a formatting rule from ruff not being respected. Should be ok now if you can retrigger the ci... |
behrmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow turnaround, I'll make some time to review this properly soon, two nits: typo in the last commit message of the series and the one below
|
Sorry for the slow update, life got busy. |
|
Should i rebase ? is anything missing ? |
Not necessary
On your end, no, thank you for your patience. On my end, so far, the time to give this the attention it deserves. I'm on vacation at the moment and will look at this the (in Germany) long weekend after ASG next week. |
|
@Mocramis so the first two look good to me, the last one, too, but I'm not sure what the third one is about. Could you explain that? |
|
I'm not entirely sure the scanner is the right place for the error path tests. I tested your changes with a separate test file import os
import threading
import time
import typing
import unittest
import varlink
import varlink.error
service = varlink.Service(
vendor="Varlink",
product="Varlink Error Example",
version="1",
url="http://varlink.org",
interface_dir=os.path.dirname(__file__),
)
class ServiceRequestHandler(varlink.RequestHandler):
service = service
@service.interface("org.example.testerrors")
class ErrorExample:
def Foo(self, param):
pass
def Bar(self, param):
pass
class TestService(unittest.TestCase):
address = "tcp:127.0.0.1:23451"
@classmethod
def setUpClass(cls):
cls._server = varlink.ThreadingServer(cls.address, ServiceRequestHandler)
cls._server_thread = threading.Thread(target=cls._server.serve_forever)
cls._server_thread.start()
time.sleep(0.1)
@classmethod
def tearDownClass(cls):
cls._server.shutdown()
cls._server.server_close()
cls._server_thread.join()
def test_foo_correct(self):
client = varlink.Client.new_with_address(self.address)
with client.open("org.example.testerrors") as conn:
self.assertEqual(conn.Foo("a"), {})
def test_foo_errexpect(self):
client = varlink.Client.new_with_address(self.address)
with client.open("org.example.testerrors") as conn:
with self.assertRaises(varlink.InvalidParameter):
conn.Foo("d")
def test_bar_correct(self):
client = varlink.Client.new_with_address(self.address)
with client.open("org.example.testerrors") as conn:
self.assertEqual(conn.Bar({"dict": {"foo": "bar"}}), {})
def test_bar_errexpect(self):
client = varlink.Client.new_with_address(self.address)
with client.open("org.example.testerrors") as conn:
with self.assertRaises(varlink.InvalidParameter):
conn.Bar({"dict": {"foo": None}})and an interface file with the interface you defined in the test. |
behrmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to cherry-pick the first two commits first and wait for feedback on the rest.
…ced message Or if this assertion is valid, the way the Client handles error is broken then. Adds a new test file checking errors serialization/deserialization.
|
@Mocramis I finally found the time to investigate this and this really is a subtle error you found there, it took me a moment to wrap my head around this. Thank you. Having looked at this, I agree that the assumption of namespaced being carried through doesn't make sense in this case, but I think I might have found a smaller way to address this, which you can find in #82. Could you have a look at this? Generally, I think the whole usage of simple namespace makes all of this much more complicated and when @jelly and I looked for users, we didn't really find any that themselves still seemed to be used. |
|
Closing in favor of #82 |
Here is a PR following #65
The 3rd commit might be entirely wrong if my interpretation of the namespaced argument is erroneous, but in that case, the client implementation is broken instead.