-
Notifications
You must be signed in to change notification settings - Fork 62
tests: System/compatibility tests for testing shim compatiblity #1218
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
base: v3_staging
Are you sure you want to change the base?
Conversation
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.
A couple comments for now, but I'll look at the tests in more detail soon
env_vars: { | ||
key: "NOX_SESSION" | ||
value: "system-3.12" | ||
} No newline at end of file |
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 pulled in main
, so this shouldn't be necessary anymore
self._row_merger = _RowMerger(self._row_merger.last_seen_row_key) | ||
self.response_iterator = self.read_method(retry_request) | ||
self.response_iterator = self.read_method(retry_request, retry=self.retry) | ||
|
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.
why was this change needed?
class SelectiveMethodsErrorInjector(UnaryStreamClientInterceptor): | ||
def __init__(self): | ||
# As long as there are more items on this list, the items on the list | ||
# are as follows: |
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 assume after the list is exhausted, it always behaves as normal?
data_table, rows_to_delete, row_keys, columns, CELL_VAL_READ_ROWS_RETRY | ||
) | ||
|
||
yield data_table |
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.
Isn't this re-populating the table before each test function? Is that necessary?
|
||
|
||
def _populate_table(data_table, rows_to_delete, row_keys): | ||
def _add_test_error_handler(retry): |
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.
a docstring could be helpful here
It looks like it overrides the on_error function to assert that backoff values are within expected bounds?
Additional system tests for testing shim compatibility with the classic client interface.
Also fixes a bug where encountering a retriable error after another retriable error mid-stream raised an exception instead of retrying.