Skip to content

test: restore shadowed discovery http close test - #2848

Open
reginaldalfret wants to merge 1 commit into
googleapis:mainfrom
reginaldalfret:fix/discovery-close-test
Open

reginaldalfret wants to merge 1 commit into
googleapis:mainfrom
reginaldalfret:fix/discovery-close-test

Conversation

@reginaldalfret

Copy link
Copy Markdown

Fixes #2757

Description

In \ ests/test_discovery.py, \class Discovery(unittest.TestCase)\ was defined twice:

  • Line 498: containing an incomplete/shadowed test \ est_discovery_http_is_closed\
  • Line 1551: defining the active \class Discovery(unittest.TestCase)\ suite

Because the second class definition shadowed the first at import time, \ est_discovery_http_is_closed\ was dead code and never executed by test runners. Additionally, the shadowed test attempted to assert \http.close\ without patching \googleapiclient.discovery.build_http().

This PR:

  1. Removes the duplicate \class Discovery\ definition at line 498.
  2. In the active \Discovery\ class, restores \ est_discovery_http_is_closed\ with proper mock patching for \�uild_http(), asserting \mock_http.close.assert_called_once().
  3. Adds \ est_provided_http_is_not_closed\ to assert that caller-provided \http\ instances are not closed by \�uild().

Verification

Verified with pytest:
\\�ash
pytest tests/test_discovery.py -k 'test_discovery_http_is_closed or test_provided_http_is_not_closed' -v
\
Result: 2 passed in 0.42s.

Resolve googleapis#2757 by removing the duplicate class Discovery(unittest.TestCase) definition at line 498 which was shadowed by the active definition at line 1551. In the active Discovery test suite, reinstate test_discovery_http_is_closed with proper mock patching for build_http(), and add test_provided_http_is_not_closed to verify caller-provided HTTP instances are not closed.

Signed-off-by: reginaldalfret <reginaldalfret@gmail.com>
@reginaldalfret
reginaldalfret requested a review from a team as a code owner October 29, 2026 01:31
@product-auto-label product-auto-label Bot added the size: s Pull request size is small. label Oct 29, 2026

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

Copy link
Copy Markdown

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 refactors and expands the HTTP discovery tests in tests/test_discovery.py. It moves and updates test_discovery_http_is_closed to mock the HTTP builder and assert that the HTTP client is closed, and adds a new test test_provided_http_is_not_closed to ensure a user-provided HTTP client is not closed. The review feedback points out that using a plain mock.Mock() for credentials can cause test failures in environments where HAS_UNIVERSE is false, and suggests using a mock with a specific credentials spec and resetting the universe domain to prevent this issue.

Comment thread tests/test_discovery.py
Comment on lines +1549 to +1550
creds = mock.Mock()
service = build("youtube", "v3", credentials=creds)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using a plain mock.Mock() for credentials can cause test failures in environments where HAS_UNIVERSE is False. In those environments, _check_api_core_compatible_with_credentials_universe is executed and accesses credentials.universe_domain. Since creds is a plain mock, getattr(creds, "universe_domain", None) returns a new Mock object (which is truthy and not equal to "googleapis.com"), triggering an unexpected APICoreVersionError.

To make the test robust and consistent with other tests in this suite, use mock.Mock(spec=google.auth.credentials.Credentials) and call _reset_universe_domain(creds).

Suggested change
creds = mock.Mock()
service = build("youtube", "v3", credentials=creds)
creds = mock.Mock(spec=google.auth.credentials.Credentials)
_reset_universe_domain(creds)
service = build("youtube", "v3", credentials=creds)

This branch has not been deployed

No deployments
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