LCORE-574: database configuration unit tests #495
Conversation
WalkthroughAdds a DatabaseConfiguration wrapper supporting either PostgreSQL or SQLite with mutual-exclusivity validation and a db_type property; exposes SQLiteDatabaseConfiguration and DatabaseConfiguration publicly; extends LlamaStackConfiguration with an optional api_key and updates serialization; tests updated/added for these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor T as Tests
participant C as DatabaseConfiguration
participant P as PostgreSQLDatabaseConfiguration
participant S as SQLiteDatabaseConfiguration
T->>C: construct(db_postgres=P?, db_sqlite=S?)
alt both provided
C-->>T: raise ValidationError
else neither provided
T->>C: access db_type
C-->>T: raise ValueError
else exactly one provided
T->>C: access db_type
C-->>T: "postgres" or "sqlite"
end
sequenceDiagram
autonumber
actor T as Tests
participant L as LlamaStackConfiguration
T->>L: construct(..., api_key="whatever")
T->>L: dump()
L-->>T: {... "llama_stack": {"api_key": "whatever"}, ...}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (4)
tests/unit/models/test_config.py (4)
508-511: Docstring claims redaction but test asserts cleartext valuesThe test does not verify redaction and actually expects api_key to be present. Align the docstring with behavior.
- Test that the Configuration object can be serialized to a JSON file and - that the resulting file contains all expected sections and values. It also - checks if all keys and passwords were redacted. + Test that the Configuration object can be serialized to a JSON file and + that the resulting file contains all expected sections and values.
530-531: Confirm intent to serialize secrets (api_key) to diskSerializing api_key in plaintext may be undesirable. If redaction is intended, update dump() to mask secrets and adjust the assertions; otherwise, keep as-is but document the security rationale.
Also applies to: 598-602
880-905: Typo and small coverage boost for DatabaseConfiguration
- Typo: “PostgresSQL” → “PostgreSQL”.
- Optional: also assert the config property returns the active config.
- with subtests.test(msg="PostgresSQL"): + with subtests.test(msg="PostgreSQL"): @@ d = DatabaseConfiguration(postgres=d1) assert d is not None assert d.sqlite is None assert d.postgres is not None assert d.db_type == "postgres" + assert d.config is d1 @@ d = DatabaseConfiguration(sqlite=d1) assert d is not None assert d.sqlite is not None assert d.postgres is None assert d.db_type == "sqlite" + assert d.config is d1
920-928: Exception type consistency: ValueError vs ValidationErrorWhen both DB configs are provided, the model validator raises a ValueError in the implementation; depending on Pydantic behavior, this may surface as ValidationError. Please confirm the actual exception and make tests consistent with other validators in this file.
If the implementation raises ValueError directly (consistent with other checks like the PostgreSQL port guard), adjust the test:
- with pytest.raises( - ValidationError, match="Only one database configuration can be provided" - ): + with pytest.raises( + ValueError, match="Only one database configuration can be provided" + ): DatabaseConfiguration(postgres=d1, sqlite=d2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/unit/models/test_config.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/models/test_config.py (1)
src/models/config.py (4)
SQLiteDatabaseConfiguration(70-73)DatabaseConfiguration(97-135)PostgreSQLDatabaseConfiguration(76-94)db_type(120-126)
🪛 GitHub Actions: Python linter
tests/unit/models/test_config.py
[warning] 917-917: pylint: W0104: Statement seems to have no effect (pointless-statement). Lint step 'uv run pylint src tests' exited with code 4.
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (1)
tests/unit/models/test_config.py (1)
30-32: LGTM: new public DB config imports are correctThe added imports align with the new types used below.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/unit/models/test_config.py (1)
908-923: W0104 linter issue resolved; fix comment typosNice update using _ = d.db_type to trigger the property. Tidy the comment typos.
- # access propery to call it's getter + # access property to call its getter
🧹 Nitpick comments (3)
tests/unit/models/test_config.py (3)
508-512: Polish the docstring grammarMinor grammar edits for clarity.
- Test that the Configuration object can be serialized to a JSON file and - that the resulting file contains all expected sections and values. - - Please note that redaction process is not in place. + Test that the Configuration object can be serialized to JSON and that + the resulting file contains all expected sections and values. + + Note: redaction is not applied.
528-533: API key round‑trip covered; consider redaction policyIncluding api_key in both construction and the dump assertion is correct per current behavior. Consider a follow-up to optionally redact or exclude secrets in persisted config dumps (env-var overrides, redact-on-dump flag), then adjust this test accordingly.
Also applies to: 598-603
881-906: Solid coverage; make SQLite path OS‑agnosticGood mutual‑exclusion and db_type assertions. To avoid hardcoded POSIX paths, prefer tmp_path for SQLite.
-def test_database_configuration(subtests) -> None: +def test_database_configuration(subtests, tmp_path) -> None: @@ - with subtests.test(msg="SQLite"): - d1 = SQLiteDatabaseConfiguration( - db_path="/tmp/foo/bar/baz", - ) + with subtests.test(msg="SQLite"): + sqlite_path = tmp_path / "foo" / "bar" / "baz.db" + d1 = SQLiteDatabaseConfiguration(db_path=str(sqlite_path))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/unit/models/test_config.py(6 hunks)
⏰ 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). (2)
- GitHub Check: e2e_tests
- GitHub Check: build-pr
🔇 Additional comments (4)
tests/unit/models/test_config.py (4)
18-18: Good swap to a domain-specific exceptionImporting and asserting on InvalidConfigurationError improves intent over a generic ValueError.
32-33: Public DB configs import looks rightExposing SQLiteDatabaseConfiguration and DatabaseConfiguration via models.config aligns with test usage.
925-933: Both-DBs path validatedAsserting ValidationError with the specific message keeps contract strict; looks good.
881-906: Verified: test expectations align with implementationAll error messages (“Only one database configuration can be provided” and “No database configuration found”), the db_type property, the default SQLite path (“/tmp/lightspeed-stack.db”), and the api_key field are present as tested. No changes required.
1 participant
Description
LCORE-574: database configuration unit tests
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Tests