LCORE-741: unit tests for quota limiter factory #706
Conversation
WalkthroughAdded unit test modules for quota functionality: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes are test-only additions with consistent patterns across both modules. Each test follows straightforward assertion logic against mock configurations or constructed error states. While there is variety in test scenarios (multiple subject types, storage backends), the logic density is low and repetition within each test module is high, reducing per-scenario review overhead. Possibly related PRs
Poem
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/unit/quota/test_quota_exceed_error.py (2)
8-8: Add return type annotation.Per coding guidelines, all functions require complete type annotations including return types.
Apply this diff:
-def test_quota_exceed_error_constructor(): +def test_quota_exceed_error_constructor() -> None:As per coding guidelines.
8-32: Consider verifying custom attributes on the exception.The test comprehensively validates error messages but doesn't verify the custom attributes (
subject_id,available,needed) that are set on the exception object according to the implementation insrc/quota/quota_exceed_error.py(lines 29-32).For more thorough coverage, you could capture the exception and assert these attributes:
def test_quota_exceed_error_constructor() -> None: """Test the QuotaExceedError constructor.""" expected = "User 1234 has 100 tokens, but 1000 tokens are needed" with pytest.raises(QuotaExceedError, match=expected) as exc_info: raise QuotaExceedError("1234", "u", 100, 1000) assert exc_info.value.subject_id == "1234" assert exc_info.value.available == 100 assert exc_info.value.needed == 1000 # ... (similar for other test cases)tests/unit/quota/test_quota_limiter_factory.py (3)
17-17: Add return type annotations to all test functions.Per coding guidelines, all functions require complete type annotations including return types.
Apply this pattern to all test functions:
-def test_quota_limiters_no_storage(): +def test_quota_limiters_no_storage() -> None:As per coding guidelines.
Also applies to: 27-27, 38-38, 49-49, 60-60, 71-71, 93-93, 113-113, 135-135, 155-155, 185-185
28-28: Improve docstring specificity.The docstring "Test the quota limiters creating when no limiters are specified." is used for multiple tests that actually test different scenarios (no limiters vs empty limiters list).
Consider making the docstrings more specific:
- Line 28: "Test quota limiters when PostgreSQL storage is configured but limiters is None."
- Line 39: "Test quota limiters when SQLite storage is configured but limiters is None."
- Line 50: "Test quota limiters when PostgreSQL storage is configured with empty limiters list."
- Line 61: "Test quota limiters when SQLite storage is configured with empty limiters list."
Also applies to: 39-39, 50-50, 61-61
72-72: Improve docstring specificity.The docstring "Test the quota limiters creating when one limiter is specified." is repeated for multiple tests with different limiter types and storage backends.
Consider making the docstrings more specific:
- Line 72: "Test user quota limiter creation with PostgreSQL storage."
- Line 94: "Test user quota limiter creation with SQLite storage."
- Line 114: "Test cluster quota limiter creation with PostgreSQL storage."
- Line 136: "Test cluster quota limiter creation with SQLite storage."
Also applies to: 94-94, 114-114, 136-136
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/quota/test_quota_exceed_error.py(1 hunks)tests/unit/quota/test_quota_limiter_factory.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/unit/quota/test_quota_limiter_factory.pytests/unit/quota/test_quota_exceed_error.py
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 in tests; pytest is the standard
Files:
tests/unit/quota/test_quota_limiter_factory.pytests/unit/quota/test_quota_exceed_error.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/quota/test_quota_limiter_factory.pytests/unit/quota/test_quota_exceed_error.py
🧬 Code graph analysis (2)
tests/unit/quota/test_quota_limiter_factory.py (2)
src/models/config.py (5)
config(140-146)QuotaLimiterConfiguration(568-575)PostgreSQLDatabaseConfiguration(87-105)SQLiteDatabaseConfiguration(75-78)QuotaHandlersConfiguration(584-593)src/quota/quota_limiter_factory.py (2)
QuotaLimiterFactory(17-66)quota_limiters(21-50)
tests/unit/quota/test_quota_exceed_error.py (1)
src/quota/quota_exceed_error.py (1)
QuotaExceedError(6-38)
⏰ 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). (3)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
| configuration.limiters[0].type = "foo" | ||
| # do not use connection to real PostgreSQL instance | ||
| mocker.patch("psycopg2.connect") | ||
| with pytest.raises(ValueError, match="Invalid limiter type: foo"): | ||
| _ = QuotaLimiterFactory.quota_limiters(configuration) |
There was a problem hiding this comment.
Fix error message match - missing period.
The error message match is incorrect. According to the factory implementation in src/quota/quota_limiter_factory.py (line 62), the error message includes a period at the end: f"Invalid limiter type: {limiter_type}." but the test matches without the period.
Apply this diff:
- with pytest.raises(ValueError, match="Invalid limiter type: foo"):
+ with pytest.raises(ValueError, match="Invalid limiter type: foo."):Alternatively, use a regex pattern that makes the period optional:
- with pytest.raises(ValueError, match="Invalid limiter type: foo"):
+ with pytest.raises(ValueError, match=r"Invalid limiter type: foo\.?"):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| configuration.limiters[0].type = "foo" | |
| # do not use connection to real PostgreSQL instance | |
| mocker.patch("psycopg2.connect") | |
| with pytest.raises(ValueError, match="Invalid limiter type: foo"): | |
| _ = QuotaLimiterFactory.quota_limiters(configuration) | |
| configuration.limiters[0].type = "foo" | |
| # do not use connection to real PostgreSQL instance | |
| mocker.patch("psycopg2.connect") | |
| with pytest.raises(ValueError, match="Invalid limiter type: foo."): | |
| _ = QuotaLimiterFactory.quota_limiters(configuration) |
🤖 Prompt for AI Agents
In tests/unit/quota/test_quota_limiter_factory.py around lines 200 to 204, the
pytest.raises match string is missing the trailing period present in the factory
error message; update the test to expect the exact message with the period (e.g.
"Invalid limiter type: foo.") or change the match argument to a regex that
allows an optional trailing period (e.g. make the period optional with \.?), so
the test matches the actual error text emitted by QuotaLimiterFactory.
1 participant
Description
LCORE-741: unit tests for quota limiter factory
Type of change
Related Tickets & Documents
Summary by CodeRabbit