Repository navigation
Conversation
Two tests in add.collections.test.ts wrapped the assertion inside the catch block: if collection.add() didn't throw, the catch body (and its expect()) never ran and the test passed vacuously instead of failing. Rewritten as await expect(...).rejects.toThrow(...), matching the pattern the file already uses two tests above (should error on non existing collection) and the fix suggested in chroma-core#2135. Scoped to this one file rather than the full sweep in chroma-core#2135, which was closed for being too large to review. Addresses chroma-core#2801
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
ErenAta16
left a comment
ErenAta16
left a comment
There was a problem hiding this comment.
Correct, and worth doing. The diagnosis holds exactly as written: with the assertion inside catch, a non-throwing add() skips the block entirely and the test reports green having asserted nothing. Both of these were in that state, so the "should error on duplicate ids" case would not have caught a regression that stopped rejecting duplicates, which is the one thing it exists to catch.
rejects.toThrow(...) is the right replacement rather than adding expect.assertions(1) to the existing shape. expect.assertions would have made the vacuous pass fail, but it fails with "expected 1 assertion, received 0" rather than saying the call didn't reject, so the new form is both stricter and clearer at failure time.
Semantics are preserved: expect(e.message).toMatch("duplicates") with a string argument is a substring check, and rejects.toThrow("duplicates") is also a substring check against the message, so neither test got looser or tighter. Passing an async arrow to expect() and unwrapping with .rejects is supported in Jest 29 (this package is on jest ^29.5.0), so the function form is fine.
Only cosmetic thing: #7509 passes the promise directly (expect(promise).rejects...) while this one wraps in async () => {}. Both work, and lining them up would be nice if you touch these again, but not worth a round trip on its own.
Pass the promise directly to expect() instead of wrapping in an async arrow, matching the style used in chroma-core#7509. Purely cosmetic - same assertions, same behavior - per review feedback on this PR.
New library + binary crate at rust/foundation-api/. Hosts the future foundation HTTP route surface as a sibling to chroma-frontend, per the "Foundation API: Long-Term Home" ADR. Depends on frontend-core (chroma-core#7070) for Axum scaffolding, middleware, the AuthenticateAndAuthorize trait, config primitives, error types, and OTEL bootstrap. Does NOT depend on chroma-frontend and does NOT register any Chroma CRUD routes. The /api/foundation/* handlers will land via chroma-core#7442, chroma-core#7507, chroma-core#7508, chroma-core#7509 by registering on the empty routes module. ## Description of changes _Summarize the changes made by this PR._ - Improvements & Bug fixes - ... - New functionality - ... ## Test plan _How are these changes tested?_ - [ ] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Migration plan _Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?_ ## Observability plan _What is the plan to instrument and monitor this change?_ ## Documentation Changes _Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the [docs section](https://github.com/chroma-core/chroma/tree/main/docs/docs.trychroma.com)?_
Nimra3261
changed the title
Two tests in
add.collections.test.tswrapped the assertion inside the catch block:If
collection.add()didn't throw, the catch body (and itsexpect()) never ran and the test passed vacuously instead of failing - not just a style issue, a real gap in coverage.Rewritten as
await expect(...).rejects.toThrow(...), matching the pattern the file already uses two tests above ("should error on non existing collection") and the fix #2135 proposed for the oldclients/jsclient before it was replaced byclients/new-js.Scoped to this one file rather than the full sweep in #2135, which was closed for being too large to review - happy to follow up with more files in separate PRs if this pattern is welcome.
Addresses #2801