SDK-shape conformance test for SearchableIndex - #222
Conversation
SearchableIndex is a hand-written interface and indexes.ts force-casts the SDK index to it with as unknown as, which erases type checking; existing tests only mock our own interface. So an SDK bump that renames or removes a method the code calls would slip past the compiler and fail at runtime. Assert the members the code relies on (describeIndexStats, namespace, searchRecords, query) exist on a real SDK index handle, constructed offline with no network. A compile-time assertion was tried but the SDK types the index too loosely to catch drift without passing vacuously, so this is a runtime check; verified a renamed member fails it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Vitest conformance suite that constructs a Pinecone index handle and verifies required index and namespace methods are available. ChangesSearchableIndex conformance
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/pinecone/searchable-index-conformance.test.ts`:
- Around line 19-28: Update the conformance test to validate the nested
namespace query path used by indexes.ts: obtain the result of
index.namespace('conformance-test') and assert its query member is a function.
Remove query from REQUIRED_MEMBERS unless the top-level SDK index is
independently required to expose it, while preserving checks for the remaining
members.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf3a135b-8a43-426f-a684-d0df4eeb3601
📒 Files selected for processing (1)
src/core/pinecone/searchable-index-conformance.test.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #222 +/- ##
=======================================
Coverage ? 85.07%
=======================================
Files ? 46
Lines ? 2399
Branches ? 826
=======================================
Hits ? 2041
Misses ? 357
Partials ? 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…el (cppalliance#220 review) The code calls query and searchRecords through index.namespace(ns) (indexes.ts namespace(ns).query, search.ts namespace(ns).searchRecords), so probe them on the namespace handle rather than the top-level index. describeIndexStats/namespace and the searchRecords index-fallback stay on the top-level index. Confirmed a bogus namespace-handle member fails the test.
Closes #220.
SearchableIndex(src/types.ts) is a hand-written interface, andsrc/core/pinecone/indexes.tsforce-casts the real SDK index onto it withas unknown as SearchableIndex, which erases type checking. The existing tests only mock our own interface, so nothing verifies the SDK index actually provides the members the code calls. A Pinecone SDK bump that renames or removes one would slip past the compiler and fail at runtime.What this adds
A conformance test that constructs a real SDK index handle (offline —
pc.index()returns a handle with no network call) and asserts the members the code relies on exist:describeIndexStats,namespace,searchRecords,query. A rename/removal in a future SDK fails this test loudly instead of at runtime. Verified against SDK 8.0.0, and confirmed a renamed member fails the test.searchis intentionally not asserted — the SDKIndexhas no baresearch(); that call site insearch.tsis a guarded fallback.Note
I tried a compile-time assertion so signature drift would also fail at build, but the SDK types the index too loosely for it to catch drift without passing vacuously (a negative check caught two formulations doing exactly that), so this is a runtime existence check rather than ship a guard that looks like it verifies something but does not.
Summary by CodeRabbit