-
Notifications
You must be signed in to change notification settings - Fork 10.8k
xds-k8s: Fix incorrect type hint in the baseline test #29577
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
Did a manual test run of this single test, all passed. |
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!
with self.subTest('7_start_test_client'): | ||
test_client = self.startTestClient(test_servers[0]) | ||
test_client: _XdsTestClient = self.startTestClient(test_server) |
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.
Let's lift the declaration two lines up? Variable "test_client" is expected to be used later in other sub tests.
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.
IRRC with
doesn't create separate variable scope (that's python's quirk) - so declarations inside with
are equivalent to the declarations outside.
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.
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.
No strong opinion here. Asking this because #29476 (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.
Oh - that comment was referring to breaking it for readability - otherwise the line break made it less readable.
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.
Then I completely misunderstood that 🤦 I thought it's about variable type annotation.
Minor fix: noticed test_servers should've been a
List[_XdsTestServer]
, but instead of fixing the type I renamed the variable fromtest_servers
totest_server
instead, and grabbed the first element.