LCORE-741: Rest of unit tests #708
Conversation
WalkthroughAdds unit tests covering PostgreSQL and SQLite connection helpers and extensive tests for ClusterQuotaLimiter and UserQuotaLimiter behaviors; adjusts quota-limiter-factory tests for emptiness checks and test signatures; adds a quota test package initializer. Tests exercise success and failure paths using mocks and in-memory SQLite. (50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 0
🧹 Nitpick comments (4)
tests/unit/quota/test_connect_pg.py (3)
20-20: Patch the import location, not the global module.The patch target should be
quota.connect_pg.psycopg2.connectinstead ofpsycopg2.connectto patch where the module is imported and used, preventing potential test interference.Apply this diff:
- mocker.patch("psycopg2.connect") + mock_connection = mocker.MagicMock() + mocker.patch("quota.connect_pg.psycopg2.connect", return_value=mock_connection)
28-28: Make the docstring more specific.The docstring is identical to the success case test. Clarify that this tests error handling, e.g., "Test that OperationalError is propagated when PostgreSQL connection fails."
35-35: Patch the import location, not the global module.Similar to the first test, the patch target should be
quota.connect_pg.psycopg2.connectfor consistency and to avoid test interference.Apply this diff:
- mocker.patch("psycopg2.connect", side_effect=OperationalError("ERROR")) + mocker.patch("quota.connect_pg.psycopg2.connect", side_effect=OperationalError("ERROR"))tests/unit/quota/test_connect_sqlite.py (1)
21-21: Make the docstring more specific.The docstring should clarify this tests error handling, e.g., "Test that OperationalError is raised when SQLite database path is invalid."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/quota/test_connect_pg.py(1 hunks)tests/unit/quota/test_connect_sqlite.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_connect_pg.pytests/unit/quota/test_connect_sqlite.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_connect_pg.pytests/unit/quota/test_connect_sqlite.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_connect_pg.pytests/unit/quota/test_connect_sqlite.py
🧬 Code graph analysis (2)
tests/unit/quota/test_connect_pg.py (2)
src/quota/connect_pg.py (1)
connect_pg(12-31)src/models/config.py (2)
config(140-146)PostgreSQLDatabaseConfiguration(87-105)
tests/unit/quota/test_connect_sqlite.py (2)
src/quota/connect_sqlite.py (1)
connect_sqlite(12-25)src/models/config.py (2)
config(140-146)SQLiteDatabaseConfiguration(75-78)
⏰ 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 (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (1)
tests/unit/quota/test_connect_sqlite.py (1)
11-17: LGTM!The test correctly uses an in-memory SQLite database (
:memory:) to verify successful connection establishment without requiring external resources.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/quota/test_cluster_quota_limiter.py (1)
40-175: Consider removing unusedquota_limitvariable declarations.Once the unused parameter is removed from the helper function, the
quota_limitvariable declarations in each test function (lines 43, 52, 67, 81, 110, 132, 145, 159) become unnecessary and can be removed for cleaner code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/quota/test_cluster_quota_limiter.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_cluster_quota_limiter.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_cluster_quota_limiter.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_cluster_quota_limiter.py
🧬 Code graph analysis (1)
tests/unit/quota/test_cluster_quota_limiter.py (4)
src/models/config.py (5)
config(140-146)QuotaLimiterConfiguration(568-575)PostgreSQLDatabaseConfiguration(87-105)SQLiteDatabaseConfiguration(75-78)QuotaHandlersConfiguration(584-593)src/quota/cluster_quota_limiter.py (1)
ClusterQuotaLimiter(10-30)src/quota/quota_exceed_error.py (1)
QuotaExceedError(6-38)src/quota/revokable_quota_limiter.py (1)
_init_quota(192-223)
🪛 GitHub Actions: Ruff
tests/unit/quota/test_cluster_quota_limiter.py
[error] 3-3: F401: 'datetime' imported but unused
[error] 6-6: F401: 'pytest_mock.MockerFixture' imported but unused
[error] 10-10: F401: 'models.config.PostgreSQLDatabaseConfiguration' imported but unused
[error] 1-1: ruff: 3 lint errors found in file (fixable with --fix)
[error] 1-1: Command failed: uv tool run ruff check . --per-file-ignores=tests/:S101 --per-file-ignores=scripts/:S101
⏰ 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 (ci)
- GitHub Check: e2e_tests (azure)
| def create_quota_limiter( | ||
| name: str, initial_quota: int, quota_limit: int | ||
| ) -> ClusterQuotaLimiter: | ||
| """Create new quota limiter instance.""" | ||
| configuration = QuotaHandlersConfiguration() | ||
| configuration.sqlite = SQLiteDatabaseConfiguration( | ||
| db_path=":memory:", | ||
| ) | ||
| configuration.limiters = [ | ||
| QuotaLimiterConfiguration( | ||
| type="cluster_limiter", | ||
| name="foo", | ||
| initial_quota=initial_quota, | ||
| quota_increase=1, | ||
| period="5 days", | ||
| ), | ||
| ] | ||
| quota_limiter = ClusterQuotaLimiter(configuration, initial_quota, 1) | ||
| assert quota_limiter is not None | ||
| return quota_limiter |
There was a problem hiding this comment.
Remove unused parameters from helper function.
The name and quota_limit parameters are defined but never used in the function body. The name parameter is always hardcoded to "foo" on line 29, and quota_limit is not referenced at all.
Apply this diff to remove the unused parameters:
def create_quota_limiter(
- name: str, initial_quota: int, quota_limit: int
+ initial_quota: int
) -> ClusterQuotaLimiter:
"""Create new quota limiter instance."""
configuration = QuotaHandlersConfiguration()Then update all test function calls to remove the unused arguments:
- quota_limiter = create_quota_limiter("foo", initial_quota, quota_limit)
+ quota_limiter = create_quota_limiter(initial_quota)This change should be applied to lines 45, 54, 69, 83, 112, 134, 147, and 161.
🤖 Prompt for AI Agents
In tests/unit/quota/test_cluster_quota_limiter.py around lines 18 to 37, the
helper create_quota_limiter defines unused parameters name and quota_limit (name
is hardcoded to "foo" and quota_limit is never referenced); remove these
parameters from the function signature (e.g., def
create_quota_limiter(initial_quota: int) -> ClusterQuotaLimiter:), keep the
internal hardcoded name or replace it with a constant if desired, and update the
body accordingly; then update all test calls that currently pass the extra
arguments on lines 45, 54, 69, 83, 112, 134, 147, and 161 to call
create_quota_limiter with only the remaining initial_quota argument.
tisnik
force-pushed
the
lcore-741-unit-tests-for-quota-limiters
branch
2 times, most recently
from
986462d to
767f167
Compare
tisnik
force-pushed
the
lcore-741-unit-tests-for-quota-limiters
branch
from
767f167 to
3a3f665
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/quota/test_cluster_quota_limiter.py (1)
16-35: Simplify helper by removing the unused variation innameparameter.The
nameparameter is always passed as"foo"in all test calls (lines 43, 52, 67, 81, 110, 132, 145, 159). Since there's no variation, you can simplify the helper by removing this parameter and using a constant instead.Additionally, the parameter naming is confusing:
quota_limitis used asinitial_quotain theQuotaLimiterConfiguration(line 28), while theinitial_quotaparameter is passed to theClusterQuotaLimiterconstructor (line 33). Consider renaming for clarity (e.g.,config_initial_quotaandruntime_initial_quota, or similar).Apply this diff to simplify the helper:
+CLUSTER_NAME = "foo" + + def create_quota_limiter( - name: str, initial_quota: int, quota_limit: int + initial_quota: int, config_initial_quota: int ) -> ClusterQuotaLimiter: """Create new quota limiter instance.""" configuration = QuotaHandlersConfiguration() configuration.sqlite = SQLiteDatabaseConfiguration( db_path=":memory:", ) configuration.limiters = [ QuotaLimiterConfiguration( type="cluster_limiter", - name=name, - initial_quota=quota_limit, + name=CLUSTER_NAME, + initial_quota=config_initial_quota, quota_increase=1, period="5 days", ), ] quota_limiter = ClusterQuotaLimiter(configuration, initial_quota, 1) assert quota_limiter is not None return quota_limiterThen update all test calls to remove the
"foo"argument and renamequota_limittoconfig_initial_quota:- quota_limiter = create_quota_limiter("foo", initial_quota, quota_limit) + quota_limiter = create_quota_limiter(initial_quota, config_initial_quota)And rename the variable in test functions:
- quota_limit = 100 + config_initial_quota = 100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/unit/quota/__init__.py(1 hunks)tests/unit/quota/test_cluster_quota_limiter.py(1 hunks)tests/unit/quota/test_connect_sqlite.py(1 hunks)tests/unit/quota/test_quota_limiter_factory.py(7 hunks)tests/unit/quota/test_user_quota_limiter.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/quota/init.py
- tests/unit/quota/test_connect_sqlite.py
- tests/unit/quota/test_user_quota_limiter.py
- tests/unit/quota/test_quota_limiter_factory.py
🧰 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_cluster_quota_limiter.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_cluster_quota_limiter.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_cluster_quota_limiter.py
🧬 Code graph analysis (1)
tests/unit/quota/test_cluster_quota_limiter.py (5)
src/models/config.py (4)
config(140-146)QuotaLimiterConfiguration(568-575)SQLiteDatabaseConfiguration(75-78)QuotaHandlersConfiguration(584-593)src/quota/cluster_quota_limiter.py (1)
ClusterQuotaLimiter(10-30)src/quota/quota_exceed_error.py (1)
QuotaExceedError(6-38)tests/unit/quota/test_user_quota_limiter.py (1)
create_quota_limiter(16-35)src/quota/revokable_quota_limiter.py (1)
_init_quota(192-223)
⏰ 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: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
- GitHub Check: build-pr
🔇 Additional comments (1)
tests/unit/quota/test_cluster_quota_limiter.py (1)
38-173: LGTM! Comprehensive test coverage for ClusterQuotaLimiter.The test suite provides solid coverage of the
ClusterQuotaLimiterfunctionality:
- Connection verification
- Quota initialization and availability checks
- Token consumption with correct arithmetic (total of 4 tokens: 1+1+2)
- Quota increase and revoke operations
- Error handling for zero quota with appropriate exception matching
All tests follow pytest conventions and include proper docstrings and type hints as per coding guidelines.
1 participant
Description
LCORE-741: Rest of unit tests
Type of change
Related Tickets & Documents
Summary by CodeRabbit