Conversation
WalkthroughThis pull request introduces a comprehensive integration testing framework for Olla. Changes include a new Makefile target that invokes a Python-based test suite, documentation describing the test structure and usage, and a substantial integration test implementation with multi-phase validation covering health checks, endpoints, models, proxy routes, translation features, and error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Integration Test Runner
participant Olla as Olla Server Instance
participant Discovery as Endpoint Discovery
participant Phases as Test Phases Executor
CLI->>Olla: Health Check (Phase 1)
Olla-->>CLI: Health Status
CLI->>Discovery: Discover Endpoints & Models
Discovery->>Olla: List Available Backends & Models
Olla-->>Discovery: Endpoint & Model Configuration
Discovery-->>CLI: Dynamic Test Matrix
CLI->>Phases: Execute Test Phases (2-9)
Phases->>Olla: Internal/Monitoring Endpoints
Phases->>Olla: Model Endpoints (Unified)
Phases->>Olla: Proxy Routes (OpenAI Format)
Phases->>Olla: Anthropic Translator Validation
Phases->>Olla: Passthrough/Translation Modes
Phases->>Olla: Provider-Specific Routes
Phases->>Olla: Response Header Validation
Phases->>Olla: Error Handling Cases
Olla-->>Phases: Test Results
Phases-->>CLI: Phase-by-Phase Results Summary
CLI->>CLI: Generate Report & Exit Code
CLI-->>CLI: Output: Per-Phase Pass/Fail Counts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
test/scripts/integration/test-integration.py (7)
221-221: Moveimport randomto the module level.Inline imports inside methods are discouraged by PEP 8 unless there's a specific reason (e.g., avoiding circular imports). Here it's unconditional, so it should sit with the other imports at the top of the file.
Proposed fix
At the top of the file (e.g., after line 18):
+import randomThen remove line 221:
- import random🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/scripts/integration/test-integration.py` at line 221, Move the inline "import random" out of the function and into the module-level imports with the other imports at the top of the file (add a single "import random" alongside existing imports) and delete the inline "import random" statement inside the test body so there are no duplicate imports.
175-183: Silentexcept: passin health check swallows all errors.Per Ruff S110, catching
Exceptionwithpassmakes debugging connectivity problems harder. Logging the exception type (even at verbose level) would help operators diagnose failures such as DNS resolution errors vs. connection refused vs. TLS issues.Proposed fix
except Exception as e: - pass + if self.verbose: + self.pcolor(GREY, f" [{type(e).__name__}] {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/scripts/integration/test-integration.py` around lines 175 - 183, The health-check silently swallows exceptions; modify the try/except around requests.get in the health check so it catches Exception as e (not bare except) and surface the error details via the existing self.pcolor logging (or another test logger) — include e.__class__.__name__ and str(e) in the failure output and keep the existing failure return path; also ensure non-200 responses fall through to the same logged failure so operators can see whether it was a connection/TLS/DNS error or an HTTP status problem (references: requests.get call, self.base_url, self.pcolor).
113-145:_last_erroris never initialised in__init__, relying on dynamic attribute creation.
_getand_postsetself._last_erroron failure, and_error_detailreads it viagetattr(self, "_last_error", None)— so it won't crash. However, it's cleaner to initialise the attribute in__init__to make the class contract explicit and avoid surprises for future maintainers.Proposed fix
def __init__(self, base_url: str, timeout: int, verbose: bool, skip_streaming: bool, skip_anthropic: bool, skip_providers: bool): ... self.endpoint_types: Dict[str, str] = {} + self._last_error: Optional[Exception] = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/scripts/integration/test-integration.py` around lines 113 - 145, The class never initializes the _last_error attribute in __init__, relying on dynamic creation in _get/_post and getattr in _error_detail; initialize self._last_error = None in the class __init__ to make the contract explicit and ensure future reads (e.g., in _error_detail) are well-defined, keeping the existing behavior of _get, _post, and _error_detail unchanged.
24-29: Windows encoding fix replacessys.stdout/sys.stderrunconditionally on Win32.Wrapping the standard streams in a new
TextIOWrappercan break callers that depend on the original stream objects (e.g.,pytestcapture, some logging handlers). ThePYTHONIOENCODINGenv-var change only affects child processes started after this point, not the current interpreter.This is likely fine for a standalone script, but worth noting if this script is ever imported or invoked by a test runner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/scripts/integration/test-integration.py` around lines 24 - 29, The Windows-specific encoding change currently replaces sys.stdout/sys.stderr which can break test runners and logging; instead, detect and prefer using sys.stdout.reconfigure and sys.stderr.reconfigure (if available) to set encoding to 'utf-8', only wrap streams as a last resort when they expose a .buffer attribute and when running as a standalone script (guarded by if __name__ == '__main__'), and keep setting os.environ['PYTHONIOENCODING']='utf-8' only for subprocesses; update the logic around sys.stdout/sys.stderr and PYTHONIOENCODING in test/scripts/integration/test-integration.py to follow this approach.
600-609: Phase 5 bypassesself._postand duplicates error-handling logic inline.
phase_anthropic_translatormakes three rawrequests.postcalls (lines 601, 647, 686) with manualtry/exceptblocks that duplicate the pattern already encapsulated inself._post. This means any future changes to timeout, error logging, or session handling need to be replicated in every call site.Consider extending
self._postto accept customheaders(it already accepts**kwargs), then use it consistently:r = self._post("/olla/anthropic/v1/messages", body, headers=headers)This would unify error handling and reduce ~30 lines of boilerplate.
Also applies to: 646-656, 685-694
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/scripts/integration/test-integration.py` around lines 600 - 609, phase_anthropic_translator currently duplicates requests.post + try/except logic in three places (raw POSTs at the three blocks) instead of using the existing self._post wrapper; update the code to call self._post("/olla/anthropic/v1/messages", body, headers=headers) (or equivalent path/body call) in each location and remove the inline try/except so error handling, timeout and logging are centralized in self._post; if needed, ensure self._post accepts and forwards headers via its **kwargs signature so all existing behavior (setting self._last_error, verbose pcolor logging, timeout/session handling) is preserved.
429-456: Token-usage validation is copy-pasted between non-streaming and streaming paths.Lines 429–456 and 501–526 contain nearly identical logic for checking
prompt_tokens,completion_tokens, andtotal_tokens. Extracting a small helper would reduce duplication and make the validation easier to update:Sketch
def _validate_token_usage(self, usage: dict) -> Tuple[bool, str]: checks = { "prompt_tokens": isinstance(usage.get("prompt_tokens"), int) and usage["prompt_tokens"] > 0, "completion_tokens": isinstance(usage.get("completion_tokens"), int) and usage["completion_tokens"] >= 0, "total_tokens": isinstance(usage.get("total_tokens"), int) and usage["total_tokens"] > 0, } missing = [k for k, v in checks.items() if not v] if not missing: detail = ", ".join(f"{k}={usage[k]}" for k in checks) return True, detail return False, f"missing or invalid: {missing}, usage={usage}"Also applies to: 501-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/scripts/integration/test-integration.py` around lines 429 - 456, Extract the duplicated token-usage validation into a helper method (e.g. _validate_token_usage(self, usage) -> Tuple[bool, str]) and replace the repeated logic in both the non-streaming and streaming branches with a call to this helper; the helper should perform the three checks for "prompt_tokens", "completion_tokens", and "total_tokens" (using usage.get and the same int/threshold logic from the sketch), return (True, detail) with formatted values when all pass, or (False, "missing or invalid: [...], usage=...") when any fail, and keep the existing try/except around r.json() in each branch to set ok/detail on JSON parse failure before calling self._print_result and self.record as currently done.
1220-1230: Confusing acceptance logic for missing-model test.ok = r.status_code < 500 or r.status_code in (502, 503)This accepts all 1xx–4xx codes plus 502 and 503, but rejects 500, 501, and 504–599. The intent (accept either graceful handling or a reasonable error) is understandable, but the blanket
< 500combined with cherry-picked 5xx codes is hard to reason about. For example, a 504 Gateway Timeout would fail the test even though it's arguably a valid backend-unavailable response.Consider being explicit about accepted outcomes:
Proposed fix
- ok = r.status_code < 500 or r.status_code in (502, 503) + # Accept any client error (4xx), successful response (2xx), + # or common gateway errors indicating backend rejection + ok = (200 <= r.status_code < 500) or r.status_code in (502, 503, 504)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/scripts/integration/test-integration.py` around lines 1220 - 1230, Replace the confusing acceptance expression for the missing-model check with an explicit set of allowed status codes and a clear comment: instead of ok = r.status_code < 500 or r.status_code in (502, 503), define allowed_graceful = {200} (and add other 2xx you want to accept) and allowed_errors = {502, 503, 504} (or other 5xx you consider acceptable), then compute ok = r.status_code in allowed_graceful or r.status_code in allowed_errors; keep the surrounding use of r, ok, detail, self._print_result and self.record unchanged so the intent is explicit and easy to reason about.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@makefile`:
- Around line 328-331: The Makefile target test-script-integration uses the
python command which can be Python 2 or missing on some systems; update the
command invocation in the test-script-integration recipe to call python3 instead
of python (i.e., change the cd test/scripts && python
integration/test-integration.py $(ARGS) line) so the integration test runs with
Python 3 as required.
In `@test/scripts/integration/README.md`:
- Around line 7-20: The README states "nine phases" but the phases table
includes a Phase 10 "Summary" row; fix by making the count and table consistent:
either remove the "Phase 10 | Summary" row from the table (since it's not a test
phase) or update the opening sentence to "ten phases"; edit the table entry
"Phase 10 | Summary" or the sentence "The script runs nine phases" accordingly
so the number and table entries match.
- Line 66: The fenced code block in test/scripts/integration/README.md is
missing a language specifier (MD040); edit the closing/opening backticks for
that example terminal output and add a language such as text or console (e.g.,
replace ``` with ```text) so the fenced code block includes a language
specifier.
In `@test/scripts/integration/test-integration.py`:
- Line 581: The string passed to self.pcolor has an unnecessary f-string prefix;
remove the leading "f" so the call uses a plain string literal (update the
invocation of self.pcolor where the message " [SKIP] Anthropic translator not
configured, skipping phase" is passed).
- Line 366: The call to self._print_result uses an unnecessary f-string — remove
the leading f so the literal "{{id}}" is not treated as an f-string; update the
argument in the self._print_result call (the "Model lookup
(/olla/models/{{id}})" string) to a plain string in the same location where the
current f-string is used so no interpolation is attempted.
- Line 1106: The file comment header "Phase 7: Response Header Validation
(cross-cutting)" is stale; update it to match the actual phase used by
phase_response_headers (which calls _phase_header(8,...)). Locate the comment
block above the phase_response_headers tests (reference phase_provider_routes
and phase_response_headers) and change the header text from "Phase 7" to "Phase
8: Response Header Validation (cross-cutting)" so the numbered phase matches the
_phase_header(8, ...) call.
---
Nitpick comments:
In `@test/scripts/integration/test-integration.py`:
- Line 221: Move the inline "import random" out of the function and into the
module-level imports with the other imports at the top of the file (add a single
"import random" alongside existing imports) and delete the inline "import
random" statement inside the test body so there are no duplicate imports.
- Around line 175-183: The health-check silently swallows exceptions; modify the
try/except around requests.get in the health check so it catches Exception as e
(not bare except) and surface the error details via the existing self.pcolor
logging (or another test logger) — include e.__class__.__name__ and str(e) in
the failure output and keep the existing failure return path; also ensure
non-200 responses fall through to the same logged failure so operators can see
whether it was a connection/TLS/DNS error or an HTTP status problem (references:
requests.get call, self.base_url, self.pcolor).
- Around line 113-145: The class never initializes the _last_error attribute in
__init__, relying on dynamic creation in _get/_post and getattr in
_error_detail; initialize self._last_error = None in the class __init__ to make
the contract explicit and ensure future reads (e.g., in _error_detail) are
well-defined, keeping the existing behavior of _get, _post, and _error_detail
unchanged.
- Around line 24-29: The Windows-specific encoding change currently replaces
sys.stdout/sys.stderr which can break test runners and logging; instead, detect
and prefer using sys.stdout.reconfigure and sys.stderr.reconfigure (if
available) to set encoding to 'utf-8', only wrap streams as a last resort when
they expose a .buffer attribute and when running as a standalone script (guarded
by if __name__ == '__main__'), and keep setting
os.environ['PYTHONIOENCODING']='utf-8' only for subprocesses; update the logic
around sys.stdout/sys.stderr and PYTHONIOENCODING in
test/scripts/integration/test-integration.py to follow this approach.
- Around line 600-609: phase_anthropic_translator currently duplicates
requests.post + try/except logic in three places (raw POSTs at the three blocks)
instead of using the existing self._post wrapper; update the code to call
self._post("/olla/anthropic/v1/messages", body, headers=headers) (or equivalent
path/body call) in each location and remove the inline try/except so error
handling, timeout and logging are centralized in self._post; if needed, ensure
self._post accepts and forwards headers via its **kwargs signature so all
existing behavior (setting self._last_error, verbose pcolor logging,
timeout/session handling) is preserved.
- Around line 429-456: Extract the duplicated token-usage validation into a
helper method (e.g. _validate_token_usage(self, usage) -> Tuple[bool, str]) and
replace the repeated logic in both the non-streaming and streaming branches with
a call to this helper; the helper should perform the three checks for
"prompt_tokens", "completion_tokens", and "total_tokens" (using usage.get and
the same int/threshold logic from the sketch), return (True, detail) with
formatted values when all pass, or (False, "missing or invalid: [...],
usage=...") when any fail, and keep the existing try/except around r.json() in
each branch to set ok/detail on JSON parse failure before calling
self._print_result and self.record as currently done.
- Around line 1220-1230: Replace the confusing acceptance expression for the
missing-model check with an explicit set of allowed status codes and a clear
comment: instead of ok = r.status_code < 500 or r.status_code in (502, 503),
define allowed_graceful = {200} (and add other 2xx you want to accept) and
allowed_errors = {502, 503, 504} (or other 5xx you consider acceptable), then
compute ok = r.status_code in allowed_graceful or r.status_code in
allowed_errors; keep the surrounding use of r, ok, detail, self._print_result
and self.record unchanged so the intent is explicit and easy to reason about.
| # Run integration test scripts (requires running Olla instance) | ||
| test-script-integration: | ||
| @echo "Running integration test scripts..." | ||
| @cd test/scripts && python integration/test-integration.py $(ARGS) |
There was a problem hiding this comment.
Use python3 instead of python for portability.
On many Linux distributions python is either absent or points to Python 2. Since the test script requires Python 3.8+ (per the README), using python3 explicitly is safer:
Proposed fix
test-script-integration:
`@echo` "Running integration test scripts..."
- `@cd` test/scripts && python integration/test-integration.py $(ARGS)
+ `@cd` test/scripts && python3 integration/test-integration.py $(ARGS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@makefile` around lines 328 - 331, The Makefile target test-script-integration
uses the python command which can be Python 2 or missing on some systems; update
the command invocation in the test-script-integration recipe to call python3
instead of python (i.e., change the cd test/scripts && python
integration/test-integration.py $(ARGS) line) so the integration test runs with
Python 3 as required.
| The script runs nine phases covering every endpoint category: | ||
|
|
||
| | Phase | Area | Tests | | ||
| |-------|------|-------| | ||
| | 1 | **Health Check** | Olla reachability gate | | ||
| | 2 | **Internal/Monitoring** | `/internal/health`, `/internal/status`, endpoints, models, stats, process, `/version` | | ||
| | 3 | **Unified Models** | `/olla/models` listing, `/olla/models/{id}` lookup | | ||
| | 4 | **Proxy (OpenAI)** | Model list, non-streaming chat, streaming SSE, response headers | | ||
| | 5 | **Anthropic Translator** | Model list, non-streaming/streaming messages, token counting, passthrough mode | | ||
| | 6 | **Passthrough/Translation** | Mode validation per backend type, SSE event types, response structure, translator stats | | ||
| | 7 | **Provider Routes** | Per-discovered-backend `/olla/{provider}/v1/models` | | ||
| | 8 | **Response Headers** | Version structure, `X-Olla-Request-ID`, `X-Olla-Response-Time` | | ||
| | 9 | **Error Handling** | Non-existent model, invalid body, missing model field | | ||
| | 10 | **Summary** | Pass/fail totals grouped by phase | |
There was a problem hiding this comment.
Minor inconsistency: "nine phases" but the table lists ten rows.
Line 7 says "nine phases" while the table includes Phase 10 (Summary). Either drop the Summary row from the table (since it's not a test phase) or update the text to "ten phases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/scripts/integration/README.md` around lines 7 - 20, The README states
"nine phases" but the phases table includes a Phase 10 "Summary" row; fix by
making the count and table consistent: either remove the "Phase 10 | Summary"
row from the table (since it's not a test phase) or update the opening sentence
to "ten phases"; edit the table entry "Phase 10 | Summary" or the sentence "The
script runs nine phases" accordingly so the number and table entries match.
|
|
||
| ## Example Output | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language specifier to the fenced code block.
Per markdownlint MD040, fenced code blocks should have a language specified. Since this is example terminal output, text or console would be appropriate:
Proposed fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/scripts/integration/README.md` at line 66, The fenced code block in
test/scripts/integration/README.md is missing a language specifier (MD040); edit
the closing/opening backticks for that example terminal output and add a
language such as text or console (e.g., replace ``` with ```text) so the fenced
code block includes a language specifier.
| else: | ||
| detail = self._error_detail(r) | ||
|
|
||
| self._print_result(f"Model lookup (/olla/models/{{id}})", ok, detail) |
There was a problem hiding this comment.
Remove extraneous f prefix — no placeholders in this f-string.
The {{id}} escapes to a literal {id}, so there's no interpolation happening. A plain string suffices.
Proposed fix
- self._print_result(f"Model lookup (/olla/models/{{id}})", ok, detail)
+ self._print_result("Model lookup (/olla/models/{id})", ok, detail)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self._print_result(f"Model lookup (/olla/models/{{id}})", ok, detail) | |
| self._print_result("Model lookup (/olla/models/{id})", ok, detail) |
🧰 Tools
🪛 Ruff (0.15.1)
[error] 366-366: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/scripts/integration/test-integration.py` at line 366, The call to
self._print_result uses an unnecessary f-string — remove the leading f so the
literal "{{id}}" is not treated as an f-string; update the argument in the
self._print_result call (the "Model lookup (/olla/models/{{id}})" string) to a
plain string in the same location where the current f-string is used so no
interpolation is attempted.
| detail = "invalid JSON" | ||
| elif r and r.status_code == 404: | ||
| detail = "endpoint not configured (404)" | ||
| self.pcolor(YELLOW, f" [SKIP] Anthropic translator not configured, skipping phase") |
There was a problem hiding this comment.
Remove extraneous f prefix — no placeholders in this f-string.
Proposed fix
- self.pcolor(YELLOW, f" [SKIP] Anthropic translator not configured, skipping phase")
+ self.pcolor(YELLOW, " [SKIP] Anthropic translator not configured, skipping phase")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.pcolor(YELLOW, f" [SKIP] Anthropic translator not configured, skipping phase") | |
| self.pcolor(YELLOW, " [SKIP] Anthropic translator not configured, skipping phase") |
🧰 Tools
🪛 Ruff (0.15.1)
[error] 581-581: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/scripts/integration/test-integration.py` at line 581, The string passed
to self.pcolor has an unnecessary f-string prefix; remove the leading "f" so the
call uses a plain string literal (update the invocation of self.pcolor where the
message " [SKIP] Anthropic translator not configured, skipping phase" is
passed).
| self._print_result(f"{provider} models (/olla/{provider_slug}/v1/models)", ok, detail) | ||
| self.record(f"{phase}/{provider}", ok, detail, phase) | ||
|
|
||
| # -- Phase 7: Response Header Validation (cross-cutting) ------------------ |
There was a problem hiding this comment.
Comment says "Phase 7" but this is Phase 8.
The comment block header is stale — phase_provider_routes is Phase 7 and phase_response_headers calls _phase_header(8, …). Keeping the comment accurate avoids confusion when navigating the file.
Proposed fix
- # -- Phase 7: Response Header Validation (cross-cutting) ------------------
+ # -- Phase 8: Response Header Validation (cross-cutting) ------------------🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/scripts/integration/test-integration.py` at line 1106, The file comment
header "Phase 7: Response Header Validation (cross-cutting)" is stale; update it
to match the actual phase used by phase_response_headers (which calls
_phase_header(8,...)). Locate the comment block above the phase_response_headers
tests (reference phase_provider_routes and phase_response_headers) and change
the header text from "Phase 7" to "Phase 8: Response Header Validation
(cross-cutting)" so the numbered phase matches the _phase_header(8, ...) call.
1 participant
adds integration tests (tests generated & refined with Claude - Opus).
Summary by CodeRabbit
Release Notes
New Features
Chores