This is a behavioural change. If you want to keep the behaviour the same, we could instead explicitly set a None timeout and defer using the new default of 120s until the next major release of firebase-admin-python. (Although defaulting to an indefinite timeout does seem a little broken, so I'd be happy to call this a bug fix instead.)

google-auth shipped the same change as minor version bump. Given our next version is going to be a major version bump this should be ok. And yes, I also prefer to consider this a bug fix than anything else.

The docs above now seem wrong: "If not set connections will never timeout, which is the default behavior of the underlying requests library."

I think you could probably just remove that sentence entirely since there is no default value for this parameter. Alternatively, you could use "If set to None, the connections will never timeout."

Done

Hm... It seems like all of our components will have an approximately identical test like this one. We could remove them all and just add this into the _http_client tests. Or we could add a parameterized test that verifies that each component sets the timeout appropriately. (No action required; I'm just thinking out loud.)

Yes, and I think the existing test cases in _http_client already provides us sufficient coverage. In each of the other modules it's probably enough to check that the Client.timeout is correctly set. We can do that clean up in a future iteration.

nit: the 'which is the default behaviour ...' part is no longer true.

If set to None connections will never
timeout, which is the default behavior of the underlying requests library.

That is still true. The requests library behavior hasn't yet changed. The default timeout=None behavior still comes from there.

Oops; yeah I missed the change from 'if unset' to 'if set to none' (despite suggesting exactly that earlier. 😳)

This function looks mostly identical to test_default_timeout. You could optionally refactor to something like this:

def _helper_test_timeout(**kwargs):
    client = _http_client.HttpClient()
    timeout = kwargs.get('timeout', _http_client.DEFAULT_TIMEOUT_SECONDS)
    assert client.timeout == timeout
    ...
    if timeout is None
        assert recorder[0]._extra_kwargs['timeout'] is None
    else:
        assert ... == pytest.approx(timeout, 0.001)

def test_default_timeout():
    _helper_test_timeout()

@parameterize(...)
def test_custom_timeout(timeout)
    _helper_test_timeout(timeout=timeout)

(Or you could parameterize the helper directly.)

Refactored with parameterization.

BaseProjectManagementTest seems to call testutils.cleanup_apps() in it's teardown() method. If you're willing to rely on that, you could drop the try, finally, and delete_app() lines. Presumably you'd nave to not hard-code the app name ("timeout-app") but instead use something based on the test parameter or something like that instead. Optional.

I have no objection to leaving it in either, but it's odd to see the app cleaned up in this test case, but not the one above.

Done