feat: set the X-Server-Timeout header when timeout is set

[jimfulton]

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:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #919 🦕

[jimfulton]

For reviewers:

The change here is very simple:
https://github.com/googleapis/python-bigquery/pull/921/files#diff-fc3199dcfb64ea8758d31d37f30dc25c4885b1c21b03812fbc0f1914f48763f3R752-R762

If timeout is set, also set the X-Server-Timeout header.

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:

• Add a new api_call API to replace mock.call. This takes expected calls and adds X-Server-Timeout headers to the expected calls if the expected calls include timeouts.
• All but one test use make_connection (including job tests that had their own version and didn't need to). make_connection sets up the mock used to make assertions about HTTP calls. I modified assert_called_with and assert_called_once_with on this mock to normalize expectations in the same way as api_call above.

[tswast]

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?

[tswast]

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?

[tswast]

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.

[tswast]

Not only specific header assertions, but a separate assertion for path, a separate assertion for body, etc.

[jimfulton]

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)

[jimfulton]

@tswast shall I? :)

[tswast]

That does sound preferable if it means we can remove the logic that checks for timeout in our tests.

[jimfulton]

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?

Sure.

[tswast]

@jimfulton Did you mean to close this PR?

[jimfulton]

@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. :)

[jimfulton]

See #927