LCORE-889: fixed issues in config models #1059
Conversation
…ContextProtocolServer, and PostgresDatabaseConfiguration tests
…LimiterConfig, and QuotaSchedulerConfig tests
…nkConfiguration, TLSConfiguration, and UserDataCollection tests
WalkthroughThis PR updates test files across the configuration models directory to reflect API changes in multiple configuration classes. Changes include adding new fields to PostgreSQL and quota scheduler configurations, introducing a health-probe skip flag to authentication configuration, extending ServiceConfiguration constructor parameters, and adding type-check suppression annotations throughout tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In `@tests/unit/models/config/test_conversation_history.py`:
- Around line 74-80: The subtest message is incorrect: change the
subtests.test(msg="SQLite cache") to reflect PostgreSQL; in the test that
constructs ConversationHistoryConfiguration(type=constants.CACHE_TYPE_POSTGRES)
and expects a ValidationError with "PostgreSQL cache is selected, but not
configured", update the msg string to "PostgreSQL cache" so it matches the
assertion and the constant usage.
In `@tests/unit/models/config/test_tls_configuration.py`:
- Around line 31-38: The test constructs a ServiceConfiguration with base_url
set to an empty string which differs from the model's Optional[str] default of
None; update the test to pass None for base_url when creating the configuration
(e.g., in the ServiceConfiguration/constructor call in
tests/unit/models/config/test_tls_configuration.py) so behavior matches
downstream checks like if cfg.base_url: and aligns with ServiceConfiguration's
default semantics.
🧹 Nitpick comments (8)
tests/unit/models/config/test_quota_scheduler_config.py (1)
10-17: Test correctly validates default values for new fields.Good coverage for the new
database_reconnection_countanddatabase_reconnection_delayfields with their expected defaults.However, the
pyright: ignore[reportCallIssue]on line 12 is inconsistent withtest_quota_scheduler_custom_configuration(lines 28-32) which doesn't have the ignore comment despite both being valid constructor calls. If the ignore is truly needed due to ConfigurationBase or Pydantic typing quirks, consider adding it consistently; otherwise, remove it from here.tests/unit/models/config/test_quota_handlers_config.py (1)
12-24: Consider adding assertions for scheduler field values.The test passes explicit values to
QuotaSchedulerConfigurationbut only assertscfg.scheduler is not None. For completeness, consider verifying the scheduler's field values:🔧 Suggested enhancement
assert cfg.scheduler is not None + assert cfg.scheduler.period == 10 + assert cfg.scheduler.database_reconnection_count == 10 + assert cfg.scheduler.database_reconnection_delay == 60 assert not cfg.enable_token_historytests/unit/models/config/test_user_data_collection.py (3)
9-17: Test name does not reflect the scenario being tested.The function name
test_user_data_collection_feedback_enabledsuggests it tests behavior when feedback is enabled, but it actually testsfeedback_enabled=False. Similarly,test_user_data_collection_feedback_disabledtests the error case when feedback is enabled. Consider renaming for clarity:
test_user_data_collection_feedback_enabled→test_user_data_collection_feedback_disabled_validtest_user_data_collection_feedback_disabled→test_user_data_collection_feedback_enabled_missing_storageSame pattern applies to the transcripts tests.
75-89: Test assumes/rootis non-writable.This test relies on
/rootbeing non-writable, which holds in typical Unix environments but would fail if tests run as root (e.g., some CI containers). Consider usingpytest'stmp_pathfixture with explicit permission manipulation for a more robust test.
12-14: Consider addressing pyright/Pydantic compatibility at project level.Multiple
# pyright: ignore[reportCallIssue]comments suppress type errors caused by pyright not fully understanding Pydantic'sField()defaults. While this workaround is acceptable, consider:
- Using
pylanceorpyrightPydantic plugin for better type inference- Documenting this pattern in the project's contributing guidelines if it's expected across test files
tests/unit/models/config/test_conversation_history.py (1)
25-25: Consider using a centralized pyright configuration for Pydantic models.The widespread
# pyright: ignore[reportCallIssue]comments throughout the test files indicate that pyright doesn't fully understand Pydantic's optional field semantics. While the inline suppressions work, you might consider:
- Adding
pyrightconfig.jsonwith Pydantic support configuration- Using
pydantic.v1or ensuring the pyright Pydantic plugin is enabledThis would reduce the need for repetitive inline comments across test files. However, the current approach is functional and acceptable for test code.
tests/unit/models/config/test_postgresql_database_configuration.py (1)
18-23: PreferSecretStrinputs over pyright suppression in PostgreSQL config tests.Passing a plain string forces
# pyright: ignore. UsingSecretStrkeeps type checking clean and aligns with the model’s declared type. Apply this pattern to all constructor calls in this file.♻️ Proposed fix (apply similarly to other call sites)
-from pydantic import ValidationError +from pydantic import SecretStr, ValidationError - c = PostgreSQLDatabaseConfiguration( - db="db", user="user", password="password" - ) # pyright: ignore[reportCallIssue] + c = PostgreSQLDatabaseConfiguration( + db="db", user="user", password=SecretStr("password") + )Also applies to: 36-41, 64-81, 97-105, 107-115
tests/unit/models/config/test_splunk_configuration.py (1)
20-20: Consolidatepyrightsuppression into a helper function to reduce noise and centralize type-check handling.The repeated
# pyright: ignore[reportCallIssue]comments across seven test cases add visual clutter. Creating a smallmake_splunk_config()helper moves the suppression to one place while keeping all call sites clean and type-checked.♻️ Suggested refactor
Add this helper at the top of the test file:
def make_splunk_config(**overrides: Any) -> SplunkConfiguration: """Create a SplunkConfiguration instance for testing.""" return SplunkConfiguration(**overrides) # pyright: ignore[reportCallIssue]Then replace all calls:
-cfg = SplunkConfiguration() # pyright: ignore[reportCallIssue] +cfg = make_splunk_config()-cfg = SplunkConfiguration(enabled=False) # pyright: ignore[reportCallIssue] +cfg = make_splunk_config(enabled=False)-SplunkConfiguration( +make_splunk_config( enabled=True, url=url, token_path=token_file if has_token else None, index=index, -) # pyright: ignore[reportCallIssue] +)And similarly for lines 94–96, 102–104, and 110–112.
| with subtests.test(msg="SQLite cache"): | ||
| with pytest.raises( | ||
| ValidationError, match="PostgreSQL cache is selected, but not configured" | ||
| ): | ||
| _ = ConversationHistoryConfiguration(type=constants.CACHE_TYPE_POSTGRES) | ||
| _ = ConversationHistoryConfiguration( | ||
| type=constants.CACHE_TYPE_POSTGRES | ||
| ) # pyright: ignore[reportCallIssue] |
There was a problem hiding this comment.
Incorrect subtest message: should be "PostgreSQL cache" not "SQLite cache".
The subtest message on line 74 is a copy-paste error. It says "SQLite cache" but the test validates PostgreSQL cache behavior (CACHE_TYPE_POSTGRES and "PostgreSQL cache is selected").
Proposed fix
- with subtests.test(msg="SQLite cache"):
+ with subtests.test(msg="PostgreSQL cache"):
with pytest.raises(
ValidationError, match="PostgreSQL cache is selected, but not configured"
):🤖 Prompt for AI Agents
In `@tests/unit/models/config/test_conversation_history.py` around lines 74 - 80,
The subtest message is incorrect: change the subtests.test(msg="SQLite cache")
to reflect PostgreSQL; in the test that constructs
ConversationHistoryConfiguration(type=constants.CACHE_TYPE_POSTGRES) and expects
a ValidationError with "PostgreSQL cache is selected, but not configured",
update the msg string to "PostgreSQL cache" so it matches the assertion and the
constant usage.
| ), | ||
| host="localhost", | ||
| base_url="", | ||
| port=1234, | ||
| color_log=True, | ||
| access_log=True, | ||
| workers=10, | ||
| auth_enabled=True, |
There was a problem hiding this comment.
Consider using None instead of empty string for base_url.
The base_url field is defined as Optional[str] with a default of None in ServiceConfiguration. Passing an empty string "" differs semantically from None—downstream code checking if cfg.base_url: will behave differently for these values.
Unless explicitly testing empty-string behavior, use None to match the model's default semantics.
Suggested fix
cfg = ServiceConfiguration(
tls_config=TLSConfiguration(
tls_certificate_path=Path("tests/configuration/server.crt"),
tls_key_path=Path("tests/configuration/server.key"),
tls_key_password=Path("tests/configuration/password"),
),
host="localhost",
- base_url="",
+ base_url=None,
port=1234,
color_log=True,
access_log=True,
workers=10,
auth_enabled=True,
)📝 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.
| ), | |
| host="localhost", | |
| base_url="", | |
| port=1234, | |
| color_log=True, | |
| access_log=True, | |
| workers=10, | |
| auth_enabled=True, | |
| ), | |
| host="localhost", | |
| base_url=None, | |
| port=1234, | |
| color_log=True, | |
| access_log=True, | |
| workers=10, | |
| auth_enabled=True, |
🤖 Prompt for AI Agents
In `@tests/unit/models/config/test_tls_configuration.py` around lines 31 - 38, The
test constructs a ServiceConfiguration with base_url set to an empty string
which differs from the model's Optional[str] default of None; update the test to
pass None for base_url when creating the configuration (e.g., in the
ServiceConfiguration/constructor call in
tests/unit/models/config/test_tls_configuration.py) so behavior matches
downstream checks like if cfg.base_url: and aligns with ServiceConfiguration's
default semantics.
1 participant
Description
LCORE-889: fixed issues in config models
Type of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit
New Features
Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.