-
Notifications
You must be signed in to change notification settings - Fork 298
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
feat: set the X-Server-Timeout header when timeout is set #921
Conversation
For reviewers: The change here is very simple: If timeout is set, also set the What's not simple is the impact on testing. Many tests make assertions about HTTP requests and this change invalidated most of those assertions. I would argue that the assertions were too broad, exposing them to cross-cutting concerns, like this one. I would also argue that the assertions were pragmatic. :) To mitigate the impact on existing test assertions, I did 2 things:
|
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.
Could you add a few unit tests specifically for _call_api
so we have tests documenting the header behavior without relying on your api_call
and add_header_assertion_to_kwargs
helpers?
@@ -18,6 +18,38 @@ | |||
import pytest | |||
|
|||
|
|||
def add_header_assertion_to_kwargs(kwargs): |
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.
My initial reaction to this is duplicating too much logic from the actual implementation and encouraging more "change detector tests". The need for it probably indicates we've been too strict on our header assertions to begin with.
Note to self: any additional thoughts after seeing the rest of the test updates?
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.
This is a clever solution. I think my initial reaction is still correct, but it's not worth it to rip out our (probably too low level) api_request
call assertions for something less specific.
If we were writing these tests for the first time, I think separate assertions for specific headers would be desired.
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.
Not only specific header assertions, but a separate assertion for path, a separate assertion for body, etc.
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.
Here's another idea, perhaps benefiting from a weekend's distance. :)
Abstract the addition of the X-Server-Timeout
header into a function. Add an autouse test fixture that replaces that function with a noop except on the tests that test addition of the header, by adding a special marker to those tests and and checking for the marker in the autouse fixture.
(https://stackoverflow.com/questions/38748257/disable-autouse-fixtures-on-specific-pytest-marks)
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.
@tswast shall I? :)
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.
That does sound preferable if it means we can remove the logic that checks for timeout in our tests.
Sure. |
@jimfulton Did you mean to close this PR? |
Yes. I'm going to make a new one. We can reopen if we change our minds. :) |
See #927 |
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #919 🦕