test: add unit tests for Bedrock provider - #1204
Conversation
- inference-config: verify bedrock returns correct config and default model - onboard: verify getSandboxInferenceConfig maps bedrock with supportsStore:false - onboard-selection: verify curated model list, region check, and endpoint probe
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughUpdated and added tests to validate Amazon Bedrock model selection (model id changed to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/onboard-selection.test.js (1)
2375-2465: Test title overstatesBEDROCK_REGIONvalidation coverage.Line 2375 says this validates the region check, but the case only tests the success path with
BEDROCK_REGIONalready set (Line 2430). Add a companion negative-path test (missingBEDROCK_REGION) to ensure the guard is actually enforced.✅ Suggested follow-up test
+ it("requires BEDROCK_REGION before accepting Bedrock selection", () => { + // same setup as the Bedrock success test, but omit BEDROCK_REGION + // assert that setup does not complete successfully and prompts/logs region guidance + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard-selection.test.js` around lines 2375 - 2465, The test title is misleading because it only covers the success path with BEDROCK_REGION set; add a negative-path test that mirrors the existing "shows curated Bedrock models..." test but does not set process.env.BEDROCK_REGION (remove or unset BEDROCK_REGION in the inline script before calling setupNim) and assert that setupNim fails (non-zero process status) and emits an error or message indicating a missing/invalid BEDROCK_REGION; reference the same symbols used in the existing test (setupNim, BEDROCK_API_KEY, BEDROCK_REGION, and the spawnSync-based test harness) so the new test reuses the fake curl and prompt runner scaffolding but expects failure when BEDROCK_REGION is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/onboard-selection.test.js`:
- Around line 2375-2465: The test title is misleading because it only covers the
success path with BEDROCK_REGION set; add a negative-path test that mirrors the
existing "shows curated Bedrock models..." test but does not set
process.env.BEDROCK_REGION (remove or unset BEDROCK_REGION in the inline script
before calling setupNim) and assert that setupNim fails (non-zero process
status) and emits an error or message indicating a missing/invalid
BEDROCK_REGION; reference the same symbols used in the existing test (setupNim,
BEDROCK_API_KEY, BEDROCK_REGION, and the spawnSync-based test harness) so the
new test reuses the fake curl and prompt runner scaffolding but expects failure
when BEDROCK_REGION is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78a413ad-4ebe-41aa-9a1d-6dbe6f59f834
📒 Files selected for processing (3)
test/inference-config.test.jstest/onboard-selection.test.jstest/onboard.test.js
|
@mvincentbb can you make sure your commits are signed and DCO tag has been added to the PR description, please? |
Signed-off-by: BAKPATINA Vincent de Paul <47268786+mvincentbb@users.noreply.github.com>
0fe0d30 to
9f2ac6d
Compare
Bedrock's OpenAI-compatible endpoint works through the existing "compatible-endpoint" option — the same path every other CSP wrapper (Vertex, Azure OpenAI, etc.) should use. Adding a dedicated provider key for each CSP does not scale and creates maintenance overhead for config, tests, UX, and docs that is better avoided. Also adds a guard test that enumerates the approved provider set and fails if a new provider key is introduced, steering contributors toward the compatible-endpoint / compatible-anthropic-endpoint options instead. Reverts: #963 Closes: #1204
|
@mvincentbb Thanks for the work on the Bedrock tests and the original provider PR (#963) — the testing was thorough and the implementation was clean. After reviewing the direction this takes us, we've decided to revert Bedrock as a first-class provider and close this PR. The core issue: adding a dedicated provider key for each CSP (Bedrock, Vertex, Azure OpenAI, etc.) doesn't scale. Each one requires config in We've added a guard test in #1212 that enforces this — it'll fail if anyone tries to add a new provider key, pointing them to the compatible-endpoint options instead. No issue with the quality of the contribution; this is a design direction decision. If you run into friction using |
## Summary Reverts #963 and adds a guard test that prevents new CSP-specific provider keys from being added to the installer. ## Rationale Bedrock's OpenAI-compatible endpoint already works through the existing `compatible-endpoint` option — the same path every other CSP wrapper (Vertex, Azure OpenAI, etc.) should use. Adding a dedicated provider key for each CSP does not scale: each one requires config in `onboard.js`, a case in `inference-config.js`, UX menu entries, model curation, validation logic, tests, and docs. The `compatible-endpoint` and `compatible-anthropic-endpoint` options already cover these use cases generically. ## Changes - **`bin/lib/onboard.js`**: Remove `BEDROCK_ENDPOINT_URL`, `bedrock` from `REMOTE_PROVIDER_CONFIG`, `REMOTE_MODEL_OPTIONS`, `validProviders`, interactive menu, region check, validation, `setupInference`, and dashboard label. - **`bin/lib/inference-config.js`**: Remove `bedrock` case from `getProviderSelectionConfig()`. - **`test/inference-config.test.js`**: Remove Bedrock-specific assertions; add `does not grow beyond the approved provider set` guard test that fails if any new provider key (bedrock, vertex, azure, etc.) is introduced. ## Related - Reverts #963 - Closes #1204 ## Type of Change - [x] Code change for a new feature, bug fix, or refactor. ## Checklist - [x] Formatters applied - [x] Tests added (guard test) - [x] No secrets committed - [x] `npm test` passes (725 passed) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Breaking Changes** * Removed support for Amazon Bedrock as an inference provider. * **Documentation** * Updated command documentation to recommend `nemoclaw onboard` for new installations. * Marked legacy `nemoclaw setup` command as deprecated. * Updated deployment guides to reference the `nemoclaw onboard` setup wizard. * **Tests** * Updated test suite to validate approved inference providers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Deepak Jain <deepujain@users.noreply.github.com>
## Summary Reverts #963 and adds a guard test that prevents new CSP-specific provider keys from being added to the installer. ## Rationale Bedrock's OpenAI-compatible endpoint already works through the existing `compatible-endpoint` option — the same path every other CSP wrapper (Vertex, Azure OpenAI, etc.) should use. Adding a dedicated provider key for each CSP does not scale: each one requires config in `onboard.js`, a case in `inference-config.js`, UX menu entries, model curation, validation logic, tests, and docs. The `compatible-endpoint` and `compatible-anthropic-endpoint` options already cover these use cases generically. ## Changes - **`bin/lib/onboard.js`**: Remove `BEDROCK_ENDPOINT_URL`, `bedrock` from `REMOTE_PROVIDER_CONFIG`, `REMOTE_MODEL_OPTIONS`, `validProviders`, interactive menu, region check, validation, `setupInference`, and dashboard label. - **`bin/lib/inference-config.js`**: Remove `bedrock` case from `getProviderSelectionConfig()`. - **`test/inference-config.test.js`**: Remove Bedrock-specific assertions; add `does not grow beyond the approved provider set` guard test that fails if any new provider key (bedrock, vertex, azure, etc.) is introduced. ## Related - Reverts #963 - Closes #1204 ## Type of Change - [x] Code change for a new feature, bug fix, or refactor. ## Checklist - [x] Formatters applied - [x] Tests added (guard test) - [x] No secrets committed - [x] `npm test` passes (725 passed) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Breaking Changes** * Removed support for Amazon Bedrock as an inference provider. * **Documentation** * Updated command documentation to recommend `nemoclaw onboard` for new installations. * Marked legacy `nemoclaw setup` command as deprecated. * Updated deployment guides to reference the `nemoclaw onboard` setup wizard. * **Tests** * Updated test suite to validate approved inference providers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Deepak Jain <deepujain@users.noreply.github.com>
## Summary Reverts NVIDIA#963 and adds a guard test that prevents new CSP-specific provider keys from being added to the installer. ## Rationale Bedrock's OpenAI-compatible endpoint already works through the existing `compatible-endpoint` option — the same path every other CSP wrapper (Vertex, Azure OpenAI, etc.) should use. Adding a dedicated provider key for each CSP does not scale: each one requires config in `onboard.js`, a case in `inference-config.js`, UX menu entries, model curation, validation logic, tests, and docs. The `compatible-endpoint` and `compatible-anthropic-endpoint` options already cover these use cases generically. ## Changes - **`bin/lib/onboard.js`**: Remove `BEDROCK_ENDPOINT_URL`, `bedrock` from `REMOTE_PROVIDER_CONFIG`, `REMOTE_MODEL_OPTIONS`, `validProviders`, interactive menu, region check, validation, `setupInference`, and dashboard label. - **`bin/lib/inference-config.js`**: Remove `bedrock` case from `getProviderSelectionConfig()`. - **`test/inference-config.test.js`**: Remove Bedrock-specific assertions; add `does not grow beyond the approved provider set` guard test that fails if any new provider key (bedrock, vertex, azure, etc.) is introduced. ## Related - Reverts NVIDIA#963 - Closes NVIDIA#1204 ## Type of Change - [x] Code change for a new feature, bug fix, or refactor. ## Checklist - [x] Formatters applied - [x] Tests added (guard test) - [x] No secrets committed - [x] `npm test` passes (725 passed) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Breaking Changes** * Removed support for Amazon Bedrock as an inference provider. * **Documentation** * Updated command documentation to recommend `nemoclaw onboard` for new installations. * Marked legacy `nemoclaw setup` command as deprecated. * Updated deployment guides to reference the `nemoclaw onboard` setup wizard. * **Tests** * Updated test suite to validate approved inference providers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Deepak Jain <deepujain@users.noreply.github.com>
## Summary Reverts NVIDIA#963 and adds a guard test that prevents new CSP-specific provider keys from being added to the installer. ## Rationale Bedrock's OpenAI-compatible endpoint already works through the existing `compatible-endpoint` option — the same path every other CSP wrapper (Vertex, Azure OpenAI, etc.) should use. Adding a dedicated provider key for each CSP does not scale: each one requires config in `onboard.js`, a case in `inference-config.js`, UX menu entries, model curation, validation logic, tests, and docs. The `compatible-endpoint` and `compatible-anthropic-endpoint` options already cover these use cases generically. ## Changes - **`bin/lib/onboard.js`**: Remove `BEDROCK_ENDPOINT_URL`, `bedrock` from `REMOTE_PROVIDER_CONFIG`, `REMOTE_MODEL_OPTIONS`, `validProviders`, interactive menu, region check, validation, `setupInference`, and dashboard label. - **`bin/lib/inference-config.js`**: Remove `bedrock` case from `getProviderSelectionConfig()`. - **`test/inference-config.test.js`**: Remove Bedrock-specific assertions; add `does not grow beyond the approved provider set` guard test that fails if any new provider key (bedrock, vertex, azure, etc.) is introduced. ## Related - Reverts NVIDIA#963 - Closes NVIDIA#1204 ## Type of Change - [x] Code change for a new feature, bug fix, or refactor. ## Checklist - [x] Formatters applied - [x] Tests added (guard test) - [x] No secrets committed - [x] `npm test` passes (725 passed) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Breaking Changes** * Removed support for Amazon Bedrock as an inference provider. * **Documentation** * Updated command documentation to recommend `nemoclaw onboard` for new installations. * Marked legacy `nemoclaw setup` command as deprecated. * Updated deployment guides to reference the `nemoclaw onboard` setup wizard. * **Tests** * Updated test suite to validate approved inference providers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Deepak Jain <deepujain@users.noreply.github.com>
Summary
Add unit tests for the Amazon Bedrock inference provider introduced in #963. Covers provider config, sandbox inference mapping,
and interactive selection UX.
Related Issue
Relates to #963
Changes
test/inference-config.test.js: verifygetProviderSelectionConfig("bedrock")returns correct OpenAI-compatible config,BEDROCK_API_KEYcredential env, and default model (nvidia.nemotron-super-3-120b)test/onboard.test.js: verifygetSandboxInferenceConfigmaps bedrock to routed inference provider withsupportsStore: falsetest/onboard-selection.test.js: verify interactive setup shows curated Bedrock models, enforcesBEDROCK_REGIONcheck, andprobes endpoint via Chat Completions API
Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).contradict existing docs).
Doc Changes
N/A — no doc changes in this PR.
Signed-off-by: BAKPATINA Vincent de Paul 47268786+mvincentbb@users.noreply.github.com
Summary by CodeRabbit