-
Notifications
You must be signed in to change notification settings - Fork 58
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
chore(tests): add conformance tests #872
Conversation
test_proxy/handlers/grpc_handler.py
Outdated
@@ -0,0 +1,148 @@ | |||
|
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.
missing header?
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.
good catch!
client_map = {} | ||
background_tasks = set() | ||
while True: | ||
if not request_q.empty(): |
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.
I'm still trying to understand the architecture of this PR. Why is the functionality of CreateClient, CloseClient and RemoveClient handled here instead of being delegated down to the next level like all the other functions sort of like how the node.js test proxy works? (ie. https://github.com/googleapis/nodejs-bigtable/blob/79035298bbf663c98d46c95021f7c944ef2c97f2/testproxy/services/index.js#L38-L47)
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.
Yes, it is annoyingly complicated because of a quirk of python protobuf. We can't load gapic proto objects and raw proto objects in the same Python instance, so we need to do a complex multi-processing system that passes messages through a queue to accomplish anything here.
The original test_proxy PR is here, which has more discussuion if you're interested, and I tried to describe some of the architecture in the docstrings. But I suggest you ignore all the test_proxy code that was already reviewed there, this is just moving the same code into the main
branch
We are currently focusing on the conformance tests for the v3 version, and will come back to fix this one in the future. I'm going to close this for now. |
This PR replicates #870 in the main branch
To accomplish this, we needed to pull in the test_proxy code from
experimental_v3
Note that the conformance tests are currently failing. We will need to address this in a future PR