LCORE-1126: better docstrings for unit tests#922
LCORE-1126: better docstrings for unit tests#922tisnik merged 3 commits intolightspeed-core:mainfrom
Conversation
WalkthroughThis pull request expands and clarifies docstrings across many unit tests and test fixtures. Tests in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/cache/test_cache_factory.py (1)
158-176: Incorrect docstring: refers to "memory cache" but this is a SQLite cache test.The docstring says "Check if memory cache configuration is checked" but the function tests SQLite cache configuration. This appears to be a copy-paste error.
Apply this diff to fix the docstring:
def test_conversation_cache_sqlite_improper_config(tmpdir: Path) -> None: - """Check if memory cache configuration is checked in cache factory. + """Check if SQLite cache configuration is checked in cache factory. Verifies that a nil SQLite configuration causes CacheFactory.conversation_cache to raise a ValueError.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tests/unit/authorization/test_middleware.py(5 hunks)tests/unit/authorization/test_resolvers.py(9 hunks)tests/unit/cache/test_cache_factory.py(7 hunks)tests/unit/cache/test_noop_cache.py(5 hunks)tests/unit/cache/test_postgres_cache.py(7 hunks)tests/unit/cache/test_sqlite_cache.py(6 hunks)tests/unit/conftest.py(1 hunks)tests/unit/metrics/test_utis.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/cache/test_noop_cache.pytests/unit/cache/test_cache_factory.pytests/unit/metrics/test_utis.pytests/unit/cache/test_sqlite_cache.pytests/unit/conftest.pytests/unit/authorization/test_middleware.pytests/unit/authorization/test_resolvers.pytests/unit/cache/test_postgres_cache.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/cache/test_noop_cache.pytests/unit/cache/test_cache_factory.pytests/unit/metrics/test_utis.pytests/unit/cache/test_sqlite_cache.pytests/unit/conftest.pytests/unit/authorization/test_middleware.pytests/unit/authorization/test_resolvers.pytests/unit/cache/test_postgres_cache.py
tests/**/conftest.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
conftest.pyfor shared pytest fixtures
Files:
tests/unit/conftest.py
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to tests/**/*.py : Use `pytest-mock` with AsyncMock objects for mocking in tests
Applied to files:
tests/unit/conftest.py
🧬 Code graph analysis (4)
tests/unit/cache/test_cache_factory.py (2)
src/models/config.py (2)
ConversationHistoryConfiguration(1146-1217)InMemoryCacheConfig(166-173)tests/unit/cache/test_postgres_cache.py (1)
postgres_cache_config(91-109)
tests/unit/cache/test_sqlite_cache.py (1)
tests/unit/cache/test_postgres_cache.py (2)
cursor(78-87)CursorMock(46-68)
tests/unit/authorization/test_resolvers.py (1)
src/models/config.py (2)
AccessRule(811-824)Action(755-808)
tests/unit/cache/test_postgres_cache.py (3)
tests/unit/cache/test_sqlite_cache.py (1)
execute(49-57)tests/unit/cache/test_cache_factory.py (1)
postgres_cache_config(60-76)src/models/config.py (1)
PostgreSQLDatabaseConfiguration(176-260)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: library mode / azure
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (45)
tests/unit/metrics/test_utis.py (1)
80-91: LGTM!The expanded docstring accurately documents the test behavior, including the mock setup (3 output tokens, 2 input tokens) and the expected assertions for both
llm_token_received_totalandllm_token_sent_totalmetrics.tests/unit/conftest.py (1)
31-34: LGTM!The "Yields:" docstring section correctly documents the tuple of AsyncMock objects returned by this generator fixture.
tests/unit/cache/test_noop_cache.py (4)
58-71: LGTM!The expanded docstring clearly explains the test scenario for
skip_user_id_check=Truewith a non-UUID user identifier.
112-121: LGTM!The docstring properly documents the expected
ValueErrorfor invalid conversation IDs.
178-184: LGTM!The expanded docstring accurately describes the readiness check behavior of the NoopCache.
222-230: LGTM!The docstring properly documents the expected
ValueErrorbehavior for improper conversation IDs.tests/unit/cache/test_cache_factory.py (8)
30-40: LGTM!The expanded docstring clearly describes the NOOP cache configuration fixture.
43-56: LGTM!The docstring accurately describes the in-memory cache configuration with
max_entries=10.
59-76: LGTM!The PostgreSQL cache configuration fixture docstring properly documents the placeholder values.
79-98: LGTM!The SQLite fixture docstring clearly documents the
tmpdirparameter and the database path creation.
101-114: LGTM!The docstring properly explains the purpose of this invalid configuration fixture for testing factory validation.
190-211: LGTM!The PostgreSQL improper config test docstring accurately describes the expected behavior.
214-227: LGTM!The docstring clearly explains the test for detecting missing cache type configuration.
230-242: LGTM!The docstring properly documents the test for unsupported cache type validation.
tests/unit/authorization/test_resolvers.py (10)
17-38: LGTM!The expanded docstring clearly documents the JWT token creation with base64url encoding and the placeholder header/signature format.
41-54: LGTM!The docstring accurately describes the AuthTuple structure returned by this helper function.
60-77: LGTM!The expanded docstring clearly documents the JwtRoleRule configuration for matching RedHat employees via
realm_access.roles.
79-94: LGTM!The docstring properly documents the JwtRolesResolver fixture and its dependency on the employee_role_rule.
114-129: LGTM!The non-employee claims fixture docstring accurately describes the JWT claims structure without the RedHat employee role.
165-183: LGTM!The email rule resolver fixture docstring clearly documents the MATCH operator configuration for email domain validation.
185-203: LGTM!The equals_rule_resolver fixture docstring accurately describes the EQUALS operator configuration.
213-230: LGTM!The in_rule_resolver fixture docstring properly documents the IN operator configuration.
349-357: LGTM!The admin_access_rules fixture docstring accurately describes the AccessRule with ADMIN action.
359-371: LGTM!The multi_role_access_rules fixture docstring clearly documents the two AccessRule instances with their respective actions.
tests/unit/cache/test_sqlite_cache.py (7)
49-57: LGTM!The expanded docstring accurately documents the
sqlite3.Errorraised by the mock cursor's execute method.
64-82: LGTM!The ConnectionMock docstrings clearly explain how the mock simulates a faulty database connection by returning a CursorMock that raises errors.
85-100: LGTM!The create_cache helper function docstring properly documents the database path creation and return type.
110-120: LGTM!The docstring clearly describes the expected exception for an invalid database path.
155-167: LGTM!The docstring properly documents the expected CacheError when initializing a disconnected cache.
170-185: LGTM!The expanded docstring accurately describes the test scenario for retrieving entries from a disconnected cache.
249-262: LGTM!The docstring clearly documents the expected behavior of listing conversations on a newly created cache.
tests/unit/authorization/test_middleware.py (6)
27-37: LGTM!The expanded docstring clearly documents the AuthTuple structure used in tests.
43-61: LGTM!The docstring accurately describes the mock configuration with empty access_rules and role_rules lists.
63-72: LGTM!The sample_access_rule fixture docstring properly documents the role and action configuration.
74-88: LGTM!The expanded docstring correctly documents the JwtRoleRule configuration including the explicit
value="test"parameter, which aligns with the updated JwtRoleRule signature.
200-219: LGTM!The mock_resolvers fixture docstring clearly describes the expected behavior of both the role_resolver (AsyncMock returning
{"employee"}) and access_resolver (MagicMock withcheck_access=Trueandget_actions={Action.QUERY}).
228-262: LGTM!The expanded docstring thoroughly documents the test scenario for access denial, including the expected HTTP 403 response and the detail payload structure.
tests/unit/cache/test_postgres_cache.py (8)
49-68: LGTM!The CursorMock docstrings accurately document the mock's behavior, including the
psycopg2.DatabaseErrorraised byexecute()with message "can not INSERT".
78-87: LGTM!The ConnectionMock.cursor docstring properly documents the simulated
psycopg2.OperationalErrorfor failed cursor acquisition.
90-109: LGTM!The postgres_cache_config fixture docstring clearly documents the placeholder configuration values used for testing without real database connections.
112-129: LGTM!The expanded docstring accurately describes the test for successful PostgresCache construction.
145-168: LGTM!The docstring properly documents the test for exception propagation during cache initialization.
254-272: LGTM!The docstring clearly describes the expected CacheError behavior when calling
get()on a disconnected cache.
380-403: LGTM!The expanded docstring thoroughly documents the test scenario for delete operation errors, including the injected ConnectionMock and expected CacheError.
470-494: LGTM!The docstring accurately describes the test for cascading deletion of topic summaries when conversations are deleted.
2923401 to
48c3a87
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/cache/test_sqlite_cache.py (2)
111-118: Remove inappropriate "Raises:" section from test docstring.Test functions should document what they verify, not what they raise. The "Raises:" section on lines 160-161 (in the similar test below) suggests an incorrect pattern. The first part of this docstring correctly describes what the test verifies: "attempting to create a SQLiteCache with a non-existent or inaccessible path raises an exception containing the text 'unable to open database file'."
Consider this pattern for test docstrings:
-"""Test the get operation when DB can not be connected. - -Verify that creating a cache with an invalid database path fails to open the database. - -Asserts that attempting to create a SQLiteCache with a non-existent or -inaccessible path raises an exception containing the text "unable to open -database file". -""" +"""Test cache initialization with invalid database path. + +Verify that creating a SQLiteCache with a non-existent or inaccessible path +raises an exception containing the text "unable to open database file". +"""
156-162: Remove "Raises:" section from test docstring.Test functions should document what they verify, not include a "Raises:" section. The "Raises:" section (lines 160-161) is appropriate for documenting the API contract of regular functions, but test functions verify that other code raises exceptions.
Apply this diff:
-"""Test the initialize_cache(). - -Verify that initialize_cache raises a CacheError when the cache is disconnected. - -Raises: - CacheError: If the cache connection is None with message "cache is disconnected". -""" +"""Test initialize_cache() with disconnected cache. + +Verify that initialize_cache raises CacheError with message "cache is disconnected" +when the cache connection is None. +"""
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/unit/cache/test_cache_factory.py(7 hunks)tests/unit/cache/test_noop_cache.py(5 hunks)tests/unit/cache/test_postgres_cache.py(7 hunks)tests/unit/cache/test_sqlite_cache.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/cache/test_cache_factory.py
- tests/unit/cache/test_noop_cache.py
- tests/unit/cache/test_postgres_cache.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/cache/test_sqlite_cache.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/cache/test_sqlite_cache.py
🧬 Code graph analysis (1)
tests/unit/cache/test_sqlite_cache.py (2)
tests/unit/cache/test_postgres_cache.py (2)
cursor(78-87)CursorMock(46-68)src/cache/sqlite_cache.py (1)
SQLiteCache(19-414)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: library mode / azure
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: library mode / ci
🔇 Additional comments (6)
tests/unit/cache/test_sqlite_cache.py (6)
50-56: LGTM - Clear documentation of mock behavior.The expanded docstring accurately documents that this method always raises
sqlite3.Errorwith the message "can not SELECT", which matches the implementation on line 57.
65-71: LGTM - Comprehensive mock documentation.The docstring clearly explains the mock's purpose and behavior. While quite detailed for a simple
__init__, it helps test readers understand the mock's role in simulating connection errors.
74-81: LGTM - Well-documented mock method.The docstring accurately describes the return type and the mock cursor's error-raising behavior, making the test fixture's purpose clear.
86-97: LGTM - Clear helper function documentation.The expanded docstring properly documents the parameters, return type, and behavior of this test helper function.
171-178: LGTM - Excellent test documentation.This docstring correctly documents what the test verifies without using an inappropriate "Raises:" section. It clearly describes the scenario (disconnected cache) and the expected outcome (CacheError with specific message).
250-256: LGTM - Clear test documentation.The docstring appropriately describes what the test verifies: that listing conversations on a newly created, connected cache returns an empty list. Good documentation style for test functions.
Description
LCORE-1126: better docstrings for unit tests
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.