Skip to content

fix(test): mock runCaptureEx so the probe-times-out test is deterministic - #3624

Merged
cv merged 2 commits into
mainfrom
fix/local-test-probe-message-drift
May 15, 2026
Merged

fix(test): mock runCaptureEx so the probe-times-out test is deterministic#3624
cv merged 2 commits into
mainfrom
fix/local-test-probe-message-drift

Conversation

@cjagwani

@cjagwani cjagwani commented May 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

The test fails ollama model validation when the probe times out or returns nothing was environment-dependent — passed on CI (no ollama installed) but failed on hosts with ollama installed.

Root cause

validateOllamaModel takes 4 args: (model, runCapture, isSpark, runCaptureEx). The probe inside uses runCaptureEx (4th), not runCapture (2nd). The test was only mocking runCapture, so the real runCaptureEx ran against the host:

  • CI (no ollama installed) → real probe returns empty stdout → empty-output branch → did not answer the local probe in time message → regex matched, test passed.
  • Host with ollama (my local, anyone who installed it) → real probe returns JSON error like {"error":"model 'X' not found"} → JSON branch → failed the local probe: … message → regex didn't match, test failed.

Other tests in the same file (lines 623, 631, 637 etc.) correctly mock runCaptureEx. This one missed it.

Fix

Mock runCaptureEx with an empty-stdout result so the empty-output branch is exercised intentionally regardless of host state. Original assertion stays — that branch still emits did not answer the local probe in time.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Updated tests to mock probe behavior and adjusted the expected failure message for local probe timeouts/empty responses.

Note: This update contains no user-facing changes.

Review Change Stack

…r output

The unit test at src/lib/inference/local.test.ts:618 expected the
legacy 'did not answer the local probe in time' error message, but
the validator now returns 'failed the local probe: model X not
found' for empty probe output.

Test/source drift, broken on main, blocking pre-commit Test (CLI)
locally for anyone whose local hits this assertion. One-line fix:
update the regex to match the current message shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
@coderabbitai

coderabbitai Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8062e4f3-a547-40dd-af98-8e8fbb2cbfc4

📥 Commits

Reviewing files that changed from the base of the PR and between 91c7839 and 5028718.

📒 Files selected for processing (1)
  • src/lib/inference/local.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/inference/local.test.ts

📝 Walkthrough

Walkthrough

The test for validateOllamaModel now injects a mocked captureEx that returns empty stdout and updates the assertion to expect an error matching /failed the local probe/.

Changes

Ollama model validation test assertion

Layer / File(s) Summary
Probe empty-output test change
src/lib/inference/local.test.ts
The "probe times out or returns nothing" test now passes a mocked captureEx (empty stdout) into validateOllamaModel and asserts the thrown error matches /failed the local probe/.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#3389: Updates around validateOllamaModel probe handling and test adjustments for empty/timeout probe failures.

Suggested labels

fix, Local Models, Provider: Ollama

Suggested reviewers

  • ericksoa
  • cv

Poem

🐰 A little mock hopped into the test,
Silent stdout, no probe to suggest.
The assertion now listens for a clearer note,
"Failed the local probe" — that's what it wrote.
The rabbit applauds this tidy jest. 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: mocking runCaptureEx to make the probe-timeout test deterministic, which aligns with the core objective of removing non-determinism from the test.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/local-test-probe-message-drift

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: None
Optional E2E: None

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • None. No E2E is recommended because this PR changes only an existing unit test and cannot affect runtime behavior or real assistant user flows. The appropriate validation is the normal unit test suite covering src/lib/inference/local.test.ts.

Optional E2E

  • None.

New E2E recommendations

  • None.

…stic

The probe inside validateOllamaModel uses runCaptureEx (4th positional
arg), not runCapture (2nd). The test was only mocking runCapture, so
the real probe ran against the host: on CI (no ollama installed) it
returned empty stdout and the test passed; on hosts with ollama it
returned a JSON error and the test failed.

Mock runCaptureEx with an empty-stdout result so the empty-output
branch is exercised intentionally regardless of host state. The
assertion stays at the original 'did not answer the local probe in
time' message that branch emits.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
@cjagwani cjagwani changed the title fix(test): align ollama probe-timeout assertion with current validator output fix(test): mock runCaptureEx so the probe-times-out test is deterministic May 15, 2026
@cjagwani cjagwani self-assigned this May 15, 2026
@cv
cv enabled auto-merge (squash) May 15, 2026 19:58
@cv
cv merged commit 0968e59 into main May 15, 2026
25 of 26 checks passed
@wscurran wscurran added bug-fix PR fixes a bug or regression and removed priority: low labels Jun 3, 2026
@cv
cv deleted the fix/local-test-probe-message-drift branch June 28, 2026 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants