test: add transport-wiring regression tests for #459

[nabinchha]

📋 Summary

Adds the missing regression tests for #459 / #460 that verify the actual transport wiring, not just the limits property computation. Follow-up from nabinchha's review on #460.

🔄 Changes

🧪 Tests

• test_sync_pool_limits_forwarded_to_transport — patches httpx.HTTPTransport, triggers lazy init via completion(), asserts the constructor received limits.max_connections == 600 and limits.max_keepalive_connections == 300. Fails on pre-fix code because HTTPTransport was never explicitly constructed.
• test_async_pool_limits_forwarded_to_transport — same pattern for async: patches AsyncHTTPTransport, triggers via acompletion().
• Existing test_client_limits_respect_max_parallel_requests kept as a unit check for the limits property computation.

Context

The existing test (test_client_limits_respect_max_parallel_requests) only asserts client.limits.max_connections == 600, which reads self._limits — a value that was computed correctly even before the #460 fix. It passes on both the old and new code, providing no regression protection. The new tests use a mock-capture approach (per nabinchha's review suggestion) that patches the transport constructors and verifies the wiring without touching any private API.

---

🤖 Generated with AI

Made with Cursor

[greptile-apps]

Greptile Summary

This PR adds two targeted regression tests (test_sync_pool_limits_forwarded_to_transport and test_async_pool_limits_forwarded_to_transport) that verify the connection-pool limits computed from max_parallel_requests are actually forwarded to the httpx.HTTPTransport / httpx.AsyncHTTPTransport constructors, rather than only being stored in self._limits. It also parametrizes the existing test_client_limits_respect_max_parallel_requests unit test to cover both OpenAICompatibleClient and AnthropicClient, and extends the _make_openai_client / _make_anthropic_client factory helpers to accept arbitrary **kwargs.

The mock wiring is correct: the innermost @patch (_SYNC_CLIENT_PATCH / _ASYNC_CLIENT_PATCH) maps to mock_client_cls and the outer @patch (transport) maps to mock_transport_cls, which matches the actual call sequence in _get_sync_client / _get_async_client. Accessing mock_transport_cls.call_args.kwargs["limits"] is safe because the implementation always calls HTTPTransport(limits=self._limits) with a keyword argument.

• _SYNC_TRANSPORT_WIRING_CASES and _ASYNC_TRANSPORT_WIRING_CASES are identical; they can be merged into one constant.
• @pytest.mark.asyncio is placed inside the @patch stack on the async test, which is inconsistent with every other async test in the file (though functionally equivalent).

Confidence Score: 5/5

• Safe to merge; all findings are minor style/cleanup suggestions with no correctness impact.
• No P0 or P1 issues found. The test logic, mock ordering, and assertion targets are all correct. The only findings are two P2 style points (duplicate case list and decorator ordering inconsistency) that do not affect test correctness or reliability.
• No files require special attention.

Important Files Changed

Filename	Overview
packages/data-designer-engine/tests/engine/models/clients/test_native_http_clients.py	Adds two new transport-wiring regression tests and parametrizes the existing limits unit test; logic and mock ordering are correct, with one minor style inconsistency in decorator ordering.

Sequence Diagram

sequenceDiagram
    participant Test
    participant patch_Transport as @patch(HTTPTransport)
    participant patch_Client as @patch(httpx.Client)
    participant HttpModelClient
    participant mock_Transport as mock_transport_cls (MagicMock)
    participant RetryTransport
    participant mock_Client as mock_client_cls.return_value

    Test->>HttpModelClient: client_factory(max_parallel_requests=300)
    Note over HttpModelClient: _limits = Limits(max_connections=600, max_keepalive_connections=300)
    Test->>HttpModelClient: client.completion(request)
    HttpModelClient->>HttpModelClient: _get_sync_client()
    HttpModelClient->>patch_Transport: HTTPTransport(limits=self._limits)
    patch_Transport-->>HttpModelClient: mock_Transport
    HttpModelClient->>RetryTransport: create_retry_transport(config, transport=mock_Transport)
    RetryTransport-->>HttpModelClient: retry_transport
    HttpModelClient->>patch_Client: httpx.Client(transport=retry_transport, timeout=...)
    patch_Client-->>HttpModelClient: mock_client_cls.return_value
    HttpModelClient->>mock_Client: client.post(url, ...)
    mock_Client-->>HttpModelClient: mock_httpx_response
    Test->>patch_Transport: assert_called_once()
    Test->>patch_Transport: call_args.kwargs["limits"].max_connections == 600
    Test->>patch_Transport: call_args.kwargs["limits"].max_keepalive_connections == 300

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: packages/data-designer-engine/tests/engine/models/clients/test_native_http_clients.py
Line: 311-319

Comment:
**Duplicate case list for sync/async**

`_SYNC_TRANSPORT_WIRING_CASES` and `_ASYNC_TRANSPORT_WIRING_CASES` are byte-for-byte identical. They can be unified into a single constant (e.g. `_TRANSPORT_WIRING_CASES`) and reused by both tests, keeping the file easier to maintain.

```suggestion
_TRANSPORT_WIRING_CASES = [
    pytest.param(_make_openai_client, _OPENAI_MODEL, _make_openai_chat_response(), id="openai"),
    pytest.param(_make_anthropic_client, _ANTHROPIC_MODEL, _make_anthropic_chat_response(), id="anthropic"),
]
```

Then reference `_TRANSPORT_WIRING_CASES` in both `@pytest.mark.parametrize` calls.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/data-designer-engine/tests/engine/models/clients/test_native_http_clients.py
Line: 366-371

Comment:
**`@pytest.mark.asyncio` placement inconsistent with rest of file**

All other async tests in this file place `@pytest.mark.asyncio` as the outermost (or second-outermost after `@pytest.mark.parametrize`) decorator. Here it is sandwiched inside the `@patch` decorators. While it works — `functools.wraps` propagates `pytestmark` through the `@patch` wrapper — the inconsistency makes the decorator stack harder to scan and could silently break if the project ever switches to `pytest-asyncio` strict mode without `functools.wraps` propagation.

The standard, consistent ordering (matching the rest of the file) would be:

```suggestion
@pytest.mark.parametrize(("client_factory", "model_name", "response_json"), _ASYNC_TRANSPORT_WIRING_CASES)
@pytest.mark.asyncio
@patch(_ASYNC_HTTP_TRANSPORT_PATCH)
@patch(_ASYNC_CLIENT_PATCH)
async def test_async_pool_limits_forwarded_to_transport(
```

The mock argument order (`mock_client_cls` first, `mock_transport_cls` second) is unaffected by moving `@pytest.mark.asyncio` outside the `@patch` decorators.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (3): Last reviewed commit: "Merge branch 'main' into nmulepati/fix/4..." | Re-trigger Greptile}

[johnnygreco]

@nabinchha Nice idea for these tests — verifying the transport wiring is exactly the gap in coverage for #459. A couple of issues with the current approach though:

The tests will fail on the current production code. Both new tests patch httpx.HTTPTransport / AsyncHTTPTransport and assert they're called with limits=. But nothing in the production code constructs these classes directly:

• create_retry_transport() returns RetryTransport(retry=retry) with no limits parameter (retry.py:75)
• _get_sync_client() / _get_async_client() pass limits= to httpx.Client / AsyncClient, not to HTTPTransport
• A grep for HTTPTransport across clients/ returns zero matches

So mock_transport_cls.assert_called_once() will fail, and the limit assertions after it never execute.

This means one of two things needs to happen:

1. The production code needs a fix first — explicitly construct HTTPTransport(limits=...) before wrapping it in RetryTransport, since httpx ignores limits= when a custom transport= is provided. Then these tests would correctly verify that fix.
2. Or, pivot the assertions to test what the code actually does today — assert on the httpx.Client mock kwargs instead of HTTPTransport. This locks down the limits computation reaching the client constructor:

mock_client_cls.assert_called_once()
limits = mock_client_cls.call_args.kwargs["limits"]
assert limits.max_connections == 600
assert limits.max_keepalive_connections == 300

This wouldn't prove the limits are effective (same underlying httpx issue), but it would be a passing test that catches computation regressions. Option 1 is the more complete fix.

Two smaller notes:

• Only OpenAICompatibleClient is tested. Since the wiring lives in HttpModelClient, consider parametrizing over AnthropicClient too, consistent with the rest of the file.
• The new tests construct OpenAICompatibleClient directly rather than using _make_openai_client. If you add **kwargs to the helper, you could pass max_parallel_requests through it and stay consistent.

[nabinchha]

@johnnygreco Thanks for the review! Addressed your feedback in 02ccff3.

Re: "The tests will fail on the current production code" — the production code does already construct HTTPTransport(limits=self._limits) and AsyncHTTPTransport(limits=self._limits) explicitly in _get_sync_client() (L105) and _get_async_client() (L123) — that was the #460 fix. The transport is then passed into create_retry_transport(..., transport=inner), so RetryTransport wraps it rather than creating its own default pool. The tests pass as-is and correctly fail on pre-fix code.

Re: parametrize over AnthropicClient — done. All three regression tests now parametrize over both OpenAICompatibleClient and AnthropicClient, consistent with the rest of the file.

Re: use _make_openai_client helper — done. Added **kwargs to both helpers so max_parallel_requests flows through, and replaced direct construction with the factory parameter.

Also moved the transport patch constants (_HTTP_TRANSPORT_PATCH, _ASYNC_HTTP_TRANSPORT_PATCH) up with the other module-level constants.