Skip to content

Fix vacuous-pass assertion in get.collection "wrong code" test - #7525

Open
Nimra3261 wants to merge 1 commit into
chroma-core:mainfrom
Nimra3261:idiomatic-jest-get-collection
Open

Fix vacuous-pass assertion in get.collection "wrong code" test#7525
Nimra3261 wants to merge 1 commit into
chroma-core:mainfrom
Nimra3261:idiomatic-jest-get-collection

Conversation

@Nimra3261

Copy link
Copy Markdown

"wrong code returns an error" wrapped its assertions inside the catch block:

try {
  await collection.get({ where: { test: { $invalid: "hello" } } });
} catch (error: any) {
  expect(error).toBeDefined();
  expect(error.message).toMatchInlineSnapshot(...);
}

If collection.get() didn't throw, the catch body (and its error.message check) never ran and the test passed vacuously instead of failing.

Rewritten as await expect(...).rejects.toThrow(message), matching the pattern already used elsewhere in this file ("should error on non existing collection") and applied in #7508/#7509.

Addresses #2801

"wrong code returns an error" wrapped its assertions inside the catch
block: if collection.get() didn't throw, the catch body (and its
error.message check) never ran and the test passed vacuously instead
of failing.

Rewritten as await expect(...).rejects.toThrow(message), matching the
pattern already used elsewhere in this file ("should error on non
existing collection") and applied in chroma-core#7508/chroma-core#7509.

Addresses chroma-core#2801

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@github-actions

Copy link
Copy Markdown

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@ErenAta16 ErenAta16 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three more tests in this suite have the same shape, so the pattern outlives this PR.

file line test
add.collections.test.ts 72 It should return an error when inserting duplicate IDs in the same batch
add.collections.test.ts 84 should error on empty embedding
collection.config.client.test.ts 614 it should error if both schema and embedding function are provided

All three are try { await ... } catch (e) { expect(...) } with nothing after the await inside the try, so if the call ever stops throwing the catch never runs and the test still reports green. Reduced to plain node:

const neverThrows = async () => "ok";

try { await neverThrows(); } catch (e) { /* assertions live here */ }
// -> falls through, test passes

await neverThrows().then(() => { throw new Error("resolved, expected rejection"); }, () => {});
// -> throws, test fails

collection.config.client.test.ts:614 is the weakest of the three: the only assertion is expect(error).toBeDefined(), so even when it does throw it only checks that something was thrown.

add.collections.test.ts:62 — ten lines above the first one — already uses the idiom this PR switches to:

await expect(async () => {
  await collection.add({ ids: IDS, embeddings: EMBEDDINGS });
}).rejects.toThrow();

One difference worth deciding on rather than inheriting: toMatchInlineSnapshot was an exact match on the whole message, rejects.toThrow(string) is a substring match. Same message either way here, so nothing breaks — it just means a message that grows a suffix later won't be caught.

Read at 78e0d21.

@Nimra3261

Copy link
Copy Markdown
Author

Good catch, thanks. add.collections.test.ts:72/84 are the same tests already covered by #7508 (just not merged into main yet, so they still show up from this PR's diff). collection.config.client.test.ts:614 wasn't covered by anything open yet, so I opened #7526 for that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants