Skip to content

Conversation

@tisnik
Copy link
Contributor

@tisnik tisnik commented Jan 28, 2026

Description

LCORE-890: fixed all pyright issues found in quota limiters tests

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement

Tools used to create PR

  • Assisted-by: N/A
  • Generated by: N/A

Related Tickets & Documents

  • Related Issue #LCORE-890

Summary by CodeRabbit

  • Tests
    • Improved password handling in tests by switching to a secure secret representation.
    • Expanded database connection test payloads with additional connection parameters (host, port, SSL and GSS-related settings).
    • Added minor type-checking annotations and suppressions across quota and database configuration tests to aid static analysis.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 28, 2026

Walkthrough

Tests in the quota module were updated to match an expanded PostgreSQL configuration: password fields now use SecretStr, additional connection parameters were added (host, port, ssl_mode, gss_encmode, ca_cert_path), and a few pyright type-check suppressions were introduced. No runtime behavior changed.

Changes

Cohort / File(s) Summary
Quota tests — typing & config updates
tests/unit/quota/test_cluster_quota_limiter.py, tests/unit/quota/test_user_quota_limiter.py
Added pyright ignore comments on QuotaHandlersConfiguration instantiation; no functional changes.
PostgreSQL config updates in quota tests
tests/unit/quota/test_connect_pg.py, tests/unit/quota/test_quota_limiter_factory.py
Replaced plain-string passwords with SecretStr("password"); expanded PostgreSQL test configs to include host, port, ssl_mode, gss_encmode, and ca_cert_path=None; updated imports for SecretStr and constants; preserved existing test logic and error paths.

Sequence Diagram(s)

(Skipped — changes are test/type updates without new or altered multi-component control flow.)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing pyright issues in quota limiter tests, which is reflected across all four modified test files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tisnik tisnik force-pushed the lcore-890-fixed-all-pyright-issues-found-in-quota-limiters-tests branch from cc7b70a to 1ceab70 Compare January 28, 2026 18:05
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@tests/unit/quota/test_quota_limiter_factory.py`:
- Around line 6-9: The import order is wrong: third‑party imports must come
before first‑party ones. Move the pydantic import (SecretStr from pydantic)
above the project import (constants) so that "from pydantic import SecretStr" is
grouped with other third‑party imports and "import constants" remains with
first‑party imports to satisfy pylint import ordering.
🧹 Nitpick comments (2)
tests/unit/quota/test_user_quota_limiter.py (1)

16-32: Prefer constructor args to avoid the pyright ignore.
Constructing QuotaHandlersConfiguration with the fields removes the suppression and keeps the test typed.

♻️ Proposed refactor
-    configuration = QuotaHandlersConfiguration()  # pyright: ignore[reportCallIssue]
-    configuration.sqlite = SQLiteDatabaseConfiguration(
-        db_path=":memory:",
-    )
-    configuration.limiters = [
-        QuotaLimiterConfiguration(
-            type="user_limiter",
-            name=name,
-            initial_quota=quota_limit,
-            quota_increase=1,
-            period="5 days",
-        ),
-    ]
+    configuration = QuotaHandlersConfiguration(
+        sqlite=SQLiteDatabaseConfiguration(db_path=":memory:"),
+        limiters=[
+            QuotaLimiterConfiguration(
+                type="user_limiter",
+                name=name,
+                initial_quota=quota_limit,
+                quota_increase=1,
+                period="5 days",
+            ),
+        ],
+    )
tests/unit/quota/test_cluster_quota_limiter.py (1)

16-32: Prefer constructor args to avoid the pyright ignore.
Same rationale as the user limiter test: build the config with fields to keep typing intact.

♻️ Proposed refactor
-    configuration = QuotaHandlersConfiguration()  # pyright: ignore[reportCallIssue]
-    configuration.sqlite = SQLiteDatabaseConfiguration(
-        db_path=":memory:",
-    )
-    configuration.limiters = [
-        QuotaLimiterConfiguration(
-            type="cluster_limiter",
-            name=name,
-            initial_quota=quota_limit,
-            quota_increase=1,
-            period="5 days",
-        ),
-    ]
+    configuration = QuotaHandlersConfiguration(
+        sqlite=SQLiteDatabaseConfiguration(db_path=":memory:"),
+        limiters=[
+            QuotaLimiterConfiguration(
+                type="cluster_limiter",
+                name=name,
+                initial_quota=quota_limit,
+                quota_increase=1,
+                period="5 days",
+            ),
+        ],
+    )

@tisnik tisnik merged commit d58d9bf into lightspeed-core:main Jan 28, 2026
21 of 22 checks passed
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.

1 participant