Skip to content

fix(test): register fastapi fixtures and correct sqlite fixture name - #7520

Open
rekha0suthar wants to merge 1 commit into
chroma-core:mainfrom
rekha0suthar:fix/conftest-fixture-registration
Open

fix(test): register fastapi fixtures and correct sqlite fixture name#7520
rekha0suthar wants to merge 1 commit into
chroma-core:mainfrom
rekha0suthar:fix/conftest-fixture-registration

Conversation

@rekha0suthar

Copy link
Copy Markdown

Summary

Three related bugs in chromadb/test/conftest.py that prevent FastAPI fixtures from being discovered and cleaned up by pytest:

  • fastapi, async_fastapi, and fastapi_persistent were plain functions with no @pytest.fixture() decorator, so request.getfixturevalue() could not find them at runtime.
  • All three used return _fastapi_fixture(...) instead of yield from _fastapi_fixture(...), meaning the generator's teardown code never executed after each test.
  • filtered_fixture_names() listed "sqlite_fixture" which does not exist; the correct name is "sqlite".

Changes

  • Added @pytest.fixture() to fastapi, async_fastapi, and fastapi_persistent.
  • Changed return to yield from in all three so teardown (server shutdown) runs after each test.
  • Corrected "sqlite_fixture""sqlite" in filtered_fixture_names().

Test plan

  • Run existing FastAPI integration tests — fixtures are now discoverable via request.getfixturevalue("fastapi") etc.
  • Confirm server teardown executes after each test (no dangling processes).
  • Confirm "sqlite" fixture resolves correctly in the filtered fixture list.

🤖 Generated with Claude Code

Three issues in chromadb/test/conftest.py broke test collection:

fastapi, async_fastapi, and fastapi_persistent were plain functions
without @pytest.fixture decorators. The system fixture uses
request.getfixturevalue() with their names as params, which requires
them to be registered fixtures. Without the decorator pytest raises
'fixture not found' at collection time.

The same three functions used return instead of yield from, so the
server teardown (system.stop() and proc.kill()) inside _fastapi_fixture
never ran after tests completed.

filtered_fixture_names() listed 'sqlite_fixture' which does not exist;
the registered parameterized fixture is named 'sqlite'. Tests
parameterized via the system fixture would skip the sqlite variant
silently.

Fixes: add @pytest.fixture() to the three functions, switch return to
yield from, and rename 'sqlite_fixture' to 'sqlite' in the default
fixture list.

Fixes chroma-core#7395

@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)

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