test(sdk): add Anthropic Messages integration tests against llm-katan - #601
test(sdk): add Anthropic Messages integration tests against llm-katan#601yossiovadia wants to merge 2 commits into
Conversation
|
Missing Signed-off-by: ec519c1. All commits require sign-off (via |
Exercise the full Anthropic filter chain (anthropic_messages_format, anthropic_validate, anthropic_messages_protocol, token_count) through Praxis against llm-katan's Anthropic Messages endpoint. Covers non-streaming, streaming SSE, multi-turn, system messages, and token usage extraction — none of which had SDK-level integration coverage against a live Anthropic-format backend. Skips gracefully when llm-katan is unreachable. Relates to praxis-proxy#600 Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
ec519c1 to
c77ed28
Compare
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
Review: test(sdk): add Anthropic Messages integration tests against llm-katan
Good addition — SDK-level integration tests against a live Anthropic-format backend fill a real gap that the fixture-replay Rust tests cannot cover. The test structure follows the established pattern from the OpenAI SDK tests.
Findings below are actionable items to address before merge.
| Severity | Count | Summary |
|---|---|---|
| Large | 2 | Hardcoded external IP default; no error-path coverage |
| Medium | 3 | Helper duplication; redundant URL parsing; inline config vs. example config |
Automated review by praxis-bot
| # --------------------------------------------------------------------------- | ||
|
|
||
| LLM_KATAN_BASE_URL = os.environ.get( | ||
| "LLM_KATAN_BASE_URL", "https://3-147-232-199.sslip.io" |
There was a problem hiding this comment.
[Large] The default https://3-147-232-199.sslip.io embeds a specific AWS IP that will go stale when the instance is recycled. The OpenAI SDK tests default to http://127.0.0.1:8000 (local).
Either default to a local address (e.g. http://127.0.0.1:8080) or require the env var to be set and skip when unset:
LLM_KATAN_BASE_URL = os.environ.get("LLM_KATAN_BASE_URL")
# then in the fixture:
if not LLM_KATAN_BASE_URL:
pytest.skip("LLM_KATAN_BASE_URL not set")Hardcoding an ephemeral cloud IP as the default means the test silently breaks when that IP changes, with no clear signal about what went wrong.
There was a problem hiding this comment.
Fixed in 158f8f5. LLM_KATAN_BASE_URL is now required — tests skip when unset. No hardcoded default.
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def _free_port() -> int: |
There was a problem hiding this comment.
[Medium] _free_port(), _find_binary(), _wait_for_proxy(), and the subprocess-based praxis_proxy fixture pattern are duplicated verbatim from test_openai_responses_vllm.py. Extract these to a shared module (e.g. tests/integration/sdk/conftest.py or a _helpers.py) so future changes to the proxy lifecycle only need to happen in one place.
There was a problem hiding this comment.
Acknowledged, but declining in this PR — extracting shared helpers would modify test_openai_responses_vllm.py which is out of scope. Happy to do it as a follow-up if a maintainer wants it.
| ) | ||
|
|
||
|
|
||
| def _llm_katan_endpoint() -> str: |
There was a problem hiding this comment.
[Medium] _llm_katan_endpoint(), _llm_katan_host(), and _llm_katan_reachable() each independently call urlparse(LLM_KATAN_BASE_URL) and repeat the same fallback hostname. Parse once at module level and reuse:
_PARSED_URL = urlparse(LLM_KATAN_BASE_URL)
_LLM_KATAN_HOST = _PARSED_URL.hostname or "3-147-232-199.sslip.io"
_LLM_KATAN_PORT = _PARSED_URL.port or (443 if _PARSED_URL.scheme == "https" else 80)This removes the duplicated fallback strings and makes the parsing consistent.
There was a problem hiding this comment.
Fixed in 158f8f5. URL is now parsed once via _parse_llm_katan_url() returning (host, port, tls). No duplicated fallback strings.
| return False | ||
|
|
||
|
|
||
| def _write_config(proxy_port: int) -> str: |
There was a problem hiding this comment.
[Medium] The OpenAI SDK test loads from examples/configs/openai/responses/full-flow.yaml and patches it, which means config schema changes are automatically caught. This test generates config inline, so a renamed filter or changed config field would silently produce a stale config.
Consider adding an example config under examples/configs/anthropic/ and loading+patching it here, consistent with the OpenAI pattern. If inline generation is intentional (e.g. no suitable example config exists yet), add a comment explaining why.
There was a problem hiding this comment.
Intentional — no existing example config matches this pipeline (format + validate + protocol + token_count). Added a comment in the code explaining why inline generation is used. Happy to extract to an example config if a maintainer prefers.
| ) | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
[Large] All 6 tests exercise happy paths. The existing Rust integration tests cover error paths (backend_owned_missing_model, backend_owned_empty_messages), but the SDK-level tests do not verify that the anthropic_validate filter rejects invalid requests end-to-end through the real SDK.
Add at least one error-path test, for example:
def test_missing_max_tokens_rejected(self, anthropic_client):
"""Verify the validate filter rejects requests missing max_tokens."""
with pytest.raises(anthropic.BadRequestError):
anthropic_client.messages.create(
model=LLM_KATAN_MODEL,
messages=[{"role": "user", "content": "test"}],
)This confirms the validate filter works correctly when exercised by the real SDK client, not just raw HTTP.
There was a problem hiding this comment.
Fixed in 158f8f5 with test_malformed_json_rejected. Note: the suggested test_missing_max_tokens_rejected would not work here — anthropic_validate only checks proxy-owned envelope requirements (valid JSON, is an object). max_tokens is backend-owned and explicitly deferred to the inference backend per the filter's design. Malformed JSON is the correct error path to test.
- Remove hardcoded llm-katan IP default; require LLM_KATAN_BASE_URL env var and skip when unset - Parse URL once at module level instead of redundantly in each helper - Add error-path test (malformed JSON rejected by anthropic_validate) - Add inline config comment explaining why no example config is loaded Relates to praxis-proxy#600 Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
Re-review: test(sdk): add Anthropic Messages integration tests against llm-katan
The five findings from the previous review have been addressed or acknowledged:
| Previous Finding | Severity | Status |
|---|---|---|
| Hardcoded external IP default | Large | Resolved -- LLM_KATAN_BASE_URL is now required; tests skip when unset |
| Helper duplication across SDK tests | Medium | Deferred (author: out of scope for this PR) |
| Redundant URL parsing | Medium | Resolved -- centralized in _parse_llm_katan_url() |
| Inline config vs. example config | Medium | Deferred with rationale (no matching example config exists) |
| No error-path coverage | Large | Resolved -- test_malformed_json_rejected added |
No new findings at medium severity or above.
Automated re-review by praxis-bot
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
Re-review (round 3): test(sdk): add Anthropic Messages integration tests against llm-katan
All five findings from the first two rounds are resolved or deferred. One new finding below.
| Severity | Count | Summary |
|---|---|---|
| Medium | 1 | Streaming test verifies event presence but not ordering |
Automated re-review by praxis-bot
| event_types = set() | ||
|
|
||
| with anthropic_client.messages.stream( | ||
| model=LLM_KATAN_MODEL, |
There was a problem hiding this comment.
[Medium] event_types = set() discards event ordering. The PR summary claims this test validates the "SSE event lifecycle (message_start → content_block_delta → message_stop)" but the set only confirms presence, not sequence. A proxy bug that emitted message_stop before message_start would still pass.
Collect into a list and add an ordering assertion:
event_types = []
with anthropic_client.messages.stream(
...
) as stream:
for event in stream:
event_types.append(event.type)
start = event_types.index("message_start")
stop = event_types.index("message_stop")
assert start < stop, (
f"message_start (pos {start}) should precede "
f"message_stop (pos {stop}); order: {event_types}"
)
assert any(
t == "content_block_delta" for t in event_types[start:stop]
), f"content_block_delta should appear between start and stop; got: {event_types}"2 participants
Summary
anthropicPython SDKanthropic_messages_format→anthropic_validate→anthropic_messages_protocol→token_count(provider: anthropic)What this covers
test_non_streaming_basictest_non_streaming_with_systemtest_streaming_basictest_streaming_collects_full_texttest_multi_turntest_usage_presentWhy
The existing Anthropic integration tests (
tests/integration/tests/suite/anthropic_messages.rs) useBackend::fixed()to replay pre-recorded JSON fixtures. They verify Praxis passes through static data but never test against a server that speaks the actual Anthropic Messages protocol.This test sends real Anthropic SDK requests through the full filter chain to a live Anthropic-format endpoint, validating protocol fidelity end-to-end — including streaming SSE and Anthropic-native token usage extraction.
6 tests, ~1.3 seconds total. Skips gracefully when llm-katan is unreachable.
Usage
Relates to #600
Checklist
Signed-off-bytrailer.