Skip to content

fix(test): restore port-resolution assertions in nimStatusByName tests#3183

Merged
jyaunches merged 1 commit into
mainfrom
fix/issue-1280-nim-port-resolution-assertions
May 7, 2026
Merged

fix(test): restore port-resolution assertions in nimStatusByName tests#3183
jyaunches merged 1 commit into
mainfrom
fix/issue-1280-nim-port-resolution-assertions

Conversation

@laitingsheng
Copy link
Copy Markdown
Contributor

@laitingsheng laitingsheng commented May 7, 2026

Summary

Restore the curl-URL assertions in two nimStatusByName tests so they actually verify the port-resolution logic instead of only the boolean health result.

Related Issue

Closes #1280

Changes

  • src/lib/nim.test.ts:
    • "uses published docker port when no port is provided" — assert the curl call used http://127.0.0.1:9000/v1/models.
    • "falls back to 8000 when docker port lookup fails" — assert the curl call used http://127.0.0.1:8000/v1/models.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • make docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

  • Tests
    • Enhanced test coverage for health-probe verification scenarios to ensure proper command execution validation.

Assert that the curl health-check URL actually used the resolved port
in the published-docker-port and 8000-fallback branches, so the tests
verify the port-resolution logic and not just the boolean health
result.

Closes #1280

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 7, 2026

Review Change Stack
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: b38e4e3e-65a5-4a2f-ba57-317cb2f19d3a

📥 Commits

Reviewing files that changed from the base of the PR and between 2c3a392 and c968e89.

📒 Files selected for processing (1)
  • src/lib/nim.test.ts

📝 Walkthrough

Walkthrough

This PR restores health-probe URL assertions in the nimStatusByName test suite. Two test scenarios now verify that curl commands target the correct port-resolved localhost URLs using substring matching via commands.some(...), confirming the port-resolution branching logic is exercised correctly.

Changes

Health Probe URL Assertions

Layer / File(s) Summary
Health Probe URL Assertions
src/lib/nim.test.ts
"uses published docker port when no port is provided" test asserts that at least one recorded command contains http://127.0.0.1:9000/v1/models using commands.some(...). "falls back to 8000 when docker port lookup fails" test similarly asserts presence of http://127.0.0.1:8000/v1/models among recorded commands while preserving docker port invocation verification.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A rabbit hops through test assertions bright,
Where curl commands now target ports just right—
Nine-thousand here, eight-thousand there,
Port resolution answers to our care! 🐰✓

🚥 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 clearly and concisely describes the main change: restoring port-resolution assertions in specific tests.
Linked Issues check ✅ Passed The PR addresses the requirements from issue #1280 by restoring curl URL assertions for both port-resolution test scenarios.
Out of Scope Changes check ✅ Passed All changes are directly related to restoring assertions in the nimStatusByName tests as specified in issue #1280.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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/issue-1280-nim-port-resolution-assertions

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.

🔧 Microsoft Presidio Analyzer (2.2.362)
src/lib/nim.test.ts

Microsoft Presidio Analyzer failed to scan this file


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

@laitingsheng laitingsheng added the v0.0.37 Release target label May 7, 2026
@jyaunches jyaunches merged commit 54ea287 into main May 7, 2026
20 checks passed
jyaunches pushed a commit that referenced this pull request May 8, 2026
## Summary
- Bump the docs release metadata to `0.0.37`.
- Document release-prep updates for messaging policy presets, sandbox
runtime utilities, and the GPU CDI troubleshooting path.
- Refresh generated `nemoclaw-user-*` skills from the updated docs.

## Source summary
- #3159 -> `docs/reference/troubleshooting.md`: Documents the GPU CDI
preflight warning and remediation for `nvidia.com/gpu=all` gateway start
failures.
- #2415 -> `docs/reference/network-policies.md`,
`docs/manage-sandboxes/messaging-channels.md`,
`docs/network-policy/customize-network-policy.md`: Clarifies that
Telegram, Discord, and Slack egress comes from opt-in messaging presets,
not the baseline policy.
- #3091 -> `docs/deployment/sandbox-hardening.md`,
`docs/network-policy/customize-network-policy.md`: Documents the
retained sandbox utilities `vi`, `jq`, and `dos2unix` while keeping
host-side policy files as the durable source of truth.

## Test plan
- `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix
nemoclaw-user`
- `make docs`
- `npm run build:cli`
- `npm run typecheck:cli`
- Commit and pre-push hooks: markdownlint, docs-to-skills verification,
gitleaks, commitlint, CLI typecheck

## Skipped
- #3193 and #3191 matched `docs/.docs-skip` entries for experimental
shields/config paths.
- #3200 and #3183 were test-only fixes.
- #3189 and #3163 were internal documentation/refactor changes with no
public docs impact.

Made with [Cursor](https://cursor.com)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Documentation**
* Clarified which utilities remain in the sandbox runtime for
lightweight inspection and cleanup
* Noted that messaging endpoints (Discord, Slack, Telegram) are not in
the baseline policy and that channel presets are applied during
onboarding
  * Added GPU passthrough troubleshooting for gateway startup
* Updated release/version bump and release-prep workflow guidance,
including Discord preset description updates
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v0.0.37 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(test): restore port-resolution assertions in nimStatusByName tests

3 participants