feat: set the X-Server-Timeout header when timeout is set#921
feat: set the X-Server-Timeout header when timeout is set#921jimfulton wants to merge 5 commits intogoogleapis:mainfrom jimfulton:x-server-timeout-919
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:
|
tswast
left a comment
There was a problem hiding this comment.
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?
| import pytest | ||
|
|
||
|
|
||
| def add_header_assertion_to_kwargs(kwargs): |
There was a problem hiding this comment.
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.
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.
Not only specific header assertions, but a separate assertion for path, a separate assertion for body, etc.
There was a problem hiding this comment.
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.
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 🦕