Skip to content

test: restore and fix shadowed test_discovery_http_is_closed#2758

Open
Capstan wants to merge 1 commit into
googleapis:mainfrom
Capstan:fix-shadowed-http-close-test
Open

test: restore and fix shadowed test_discovery_http_is_closed#2758
Capstan wants to merge 1 commit into
googleapis:mainfrom
Capstan:fix-shadowed-http-close-test

Conversation

@Capstan
Copy link
Copy Markdown

@Capstan Capstan commented May 31, 2026

This Pull Request deletes a duplicate test class definition in test_discovery.py and restores/fixes a shadowed, previously inactive unit test for HTTP connection lifecycle checks.

The Bug (Problem)

As reported in #2757, there were two separate definitions for class Discovery(unittest.TestCase) in tests/test_discovery.py (one at line 498 and one at line 1566).

In Python, defining a class twice in the same module causes the second definition to silently overwrite the first. Because of this, the first Discovery class—which contained the connection lifecycle test test_discovery_http_is_closed—was completely dead code and was never executed by the test runner.

Furthermore, the original test case was syntactically invalid: it instantiated a regular HttpMock object (which does not inherit from mock) but tried to assert http.close.assert_called_once(). If it had ever been executed, it would have crashed instantly with an AttributeError.

Historical Context

This shadowing bug was introduced in September 2020 in commit 98888da ("fix: add method to close httplib2 connections (#1038)"). That commit added test_discovery_http_is_closed near the top of the file, without realizing that the module already contained an active class Discovery(unittest.TestCase) block near the bottom of the file (which immediately shadowed the new test).

As a result, this important connection-lifecycle test has been silently bypassed and inactive in the test suite for over 5 years.

The Fix (Solution)

  1. Deleted the duplicate class Discovery definition near the top of test_discovery.py.
  2. Restored the test_discovery_http_is_closed test inside the active Discovery class near the bottom of the file.
  3. Corrected the test logic to use standard unittest.mock.patch on httplib2.Http to verify that the temporary HTTP client created by build() to fetch the discovery document is safely closed after initialization completes, preventing socket/connection leaks.

Verification

Ran the restored test directly under pytest:

pytest tests/test_discovery.py::Discovery::test_discovery_http_is_closed

Result: Passed successfully.

Fixes #2757

@Capstan Capstan requested a review from a team as a code owner May 31, 2026 01:48
@product-auto-label product-auto-label Bot added the size: s Pull request size is small. label May 31, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request relocates and updates the test_discovery_http_is_closed test in tests/test_discovery.py to mock httplib2.Http using unittest.mock. However, the current test implementation uses a single mock instance for multiple instantiations of httplib2.Http during build(), which can lead to false positives. It is recommended to use side_effect to return distinct mock instances for each instantiation to ensure the temporary discovery client is closed and the service client is not closed prematurely.

Comment thread tests/test_discovery.py Outdated
During a cleanup of duplicate 'class Discovery(unittest.TestCase)' definitions in tests/test_discovery.py, we discovered that one of the classes was completely dead code because Python silently overwrites classes with identical names in the same module.

This shadowed class contained a connection lifecycle test: 'test_discovery_http_is_closed'.

This commit deletes the duplicate class and restores the 'test_discovery_http_is_closed' test inside the active Discovery class. The test case has been corrected to use standard 'mock.patch' on 'httplib2.Http' to successfully assert that build() closes the temporary HTTP client used during dynamic discovery initialization, preventing socket leakage.

#jetski

AI_USAGE=true
@Capstan Capstan force-pushed the fix-shadowed-http-close-test branch from ff803e6 to 2575000 Compare May 31, 2026 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: s Pull request size is small.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: connection leak test test_discovery_http_is_closed is shadowed and syntactically invalid

1 participant