-
Notifications
You must be signed in to change notification settings - Fork 12
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
TDL-16355 Implement request timeouts #43
Conversation
self.assertEqual(kwargs.get('timeout'), REQUEST_TIMEOUT_FLOAT) # Verify timeout argument | ||
|
||
# Verify that requests.Session.request is called 5 times | ||
self.assertEqual(mocked_request.call_count, 5) |
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.
@prijendev Please modularize this code to check the backoff mechanism in a separate 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.
Updated.
pass | ||
|
||
# Verify that requests.Session.request is called 5 times | ||
self.assertEqual(mocked_request.call_count, 5) |
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.
@prijendev the test case name and the assertion that you are making is different. Can you please verify it?
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, updated test case name.
|
||
# Verify requests.Session.request is called with expected timeout | ||
args, kwargs = mocked_request.call_args | ||
self.assertEqual(kwargs.get('timeout'), REQUEST_TIMEOUT_FLOAT) # Verify timeout argument |
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.
@prijendev Can you please add comment in the code regarding why you are expecting float value when you pass integer value in the config
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.
If none zero positive integer or string value passed in the config then it converted to float value. So, here it verifies the same. Added comment in the code also.
|
||
# Verify requests.Session.request is called with expected timeout | ||
args, kwargs = mocked_request.call_args | ||
self.assertEqual(kwargs.get('timeout'), REQUEST_TIMEOUT_INT) # Verify timeout argument |
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.
@prijendev Why int value is expected in this test case?
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.
If the request_timeout value is not passed in the config then the default value that is int value(300) is used.
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.
Approving, but with one comment. This workflow is using an old image for tap-tester. Please fix before moving forward.
@@ -12,6 +12,7 @@ jobs: | |||
source /usr/local/share/virtualenvs/tap-mailchimp/bin/activate | |||
pip install -U 'pip<19.2' 'setuptools<51.0.0' | |||
pip install .[dev] | |||
pip install coverage |
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.
Please use latest tap-tester image.
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.
Updated tap-tester image
Description of change
Manual QA steps
Risks
Rollback steps