fix: restore shadowed test_discovery_http_is_closed test - #2801
Open
skippdot wants to merge 1 commit into
Open
Conversation
- Remove the duplicate `class Discovery(unittest.TestCase)` definition that has shadowed the active class of the same name since 2020, leaving its only test permanently uncollected by the test runner - Re-add `test_discovery_http_is_closed` inside the surviving class, rewritten to patch `httplib2.Http`: the original asserted on `HttpMock.close`, a plain method that would raise AttributeError if the test ever ran - Pass `cache_discovery=False` so the discovery document is always fetched through the mocked client and the close assertion cannot be bypassed by a cache hit; the test fails if `build()` stops closing its temporary client Fixes googleapis#2757
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the test_discovery_http_is_closed test in tests/test_discovery.py. The test was moved to the Discovery test class and updated to use @mock.patch on httplib2.Http to verify that the temporary HTTP client created during service discovery is properly closed, preventing connection leaks. There are no review comments, so I have no feedback to provide.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2757
tests/test_discovery.pyhas contained twoclass Discovery(unittest.TestCase)definitions since September 2020 (#1038). The second definition silently shadows the first, so its only test,test_discovery_http_is_closed, has never been collected — pytest picks up 0 instances of it onmain. The dead test also could not have passed if it ever ran: it callsassert_called_once()onHttpMock.close, a plain method, which raisesAttributeError.Changes
test_discovery_http_is_closedinside the survivingDiscoveryclass, rewritten to patchhttplib2.Httpso the assertion targets a real mock. It verifies thatbuild()closes the temporary http client it creates to fetch the discovery document (discovery.py, thediscovery_http.close()call).cache_discovery=Falsein addition tostatic_discovery=False, so the discovery document is always fetched through the mocked client — a discovery-cache hit cannot bypass the code path under test.One file, +14/−7.
Verification
tests/test_discovery.py: 173 → 174.discovery_http.close()call removed fromgoogleapiclient/discovery.py, the restored test fails; with it in place, the test passes — so it genuinely guards the close behavior.tests/folder on Python 3.14: 339 passed, 1 skipped.black --checkclean;flake8 --select=E9,F63,F7,F82(the set CI enforces) clean.Note: running
tests/test_discovery.pyin isolation shows one pre-existing failure,DiscoveryErrors::test_tests_should_be_run_with_strict_positional_enforcement— that is #2755, reproducible on an unmodifiedmain, and untouched by this PR.