LCORE-574: more config property unit tests #500
Conversation
|
Warning Rate limit exceeded@tisnik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds public JwtRoleRule and JsonPathOperator for JSONPath-based JWT role rules with regex MATCH support; exposes DatabaseConfiguration.config and AuthenticationConfiguration.jwk_config / .jwk_configuration accessors with explicit validation and error flows; expands unit tests covering these behaviors and validations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant JwtRule as JwtRoleRule
note right of JwtRule #D6F5D6: Validation & regex compilation for MATCH
Caller->>JwtRule: instantiate(jsonpath, operator, value, roles, negate)
JwtRule->>JwtRule: validate required fields and JSONPath syntax
alt operator == MATCH
JwtRule->>JwtRule: compile regex from value
alt compile succeeds
JwtRule-->>Caller: instance (compiled_regex set)
else compile fails
JwtRule-->>Caller: raise ValidationError
end
else operator == EQUALS
JwtRule-->>Caller: instance (compiled_regex = None)
end
JwtRule->>JwtRule: validate roles (non-empty, no '*', unique)
JwtRule-->>Caller: final instance or ValidationError
sequenceDiagram
autonumber
actor Caller
participant DC as DatabaseConfiguration
participant PG as PostgreSQLConfig
participant SQ as SQLiteConfig
Caller->>DC: access .config
alt only PostgreSQL configured
DC-->>Caller: return PG
else only SQLite configured
DC-->>Caller: return SQ
else none configured
DC-->>Caller: raise ValueError("No database configuration found")
else both configured
DC-->>Caller: raise ValidationError("only one database configuration allowed")
end
sequenceDiagram
autonumber
actor Caller
participant AC as AuthenticationConfiguration
participant JWK as JwkConfiguration
Caller->>AC: access .jwk_configuration
alt auth module supports JWK and jwk_config provided
AC-->>Caller: return JWK
else auth module supports JWK but jwk_config missing/invalid
AC-->>Caller: raise ValidationError / ValueError
else auth module not JWK-capable
AC-->>Caller: raise ValueError("JWK configuration is only available for JWK token authentication module")
end
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: 0
🧹 Nitpick comments (3)
tests/unit/models/test_config.py (3)
896-896: Prefer identity assertion for the active config.Use
isto ensure the property returns the same instance, not just an equal model.- assert d.config == d1 + assert d.config is d1
907-907: Prefer identity assertion for the active config.Same rationale as above; assert instance identity.
- assert d.config == d1 + assert d.config is d1
926-929: Good negative test; fix comment typos.Minor: “propery” → “property”, “it's” → “its”.
- with pytest.raises(ValueError, match="No database configuration found"): - # access propery to call it's getter - _ = d.config + with pytest.raises(ValueError, match="No database configuration found"): + # access property to call its getter + _ = d.config
📜 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(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/models/test_config.py (1)
src/models/config.py (1)
config(129-135)
⏰ 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
tisnik
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/models/test_config.py (1)
1-5: Split tests/unit/models/test_config.py into smaller modules to resolve pylint C0302 (1028 > 1000).Recommended split:
- tests/unit/models/test_config_authentication.py (Authentication/JWK)
- tests/unit/models/test_config_database.py (DB config)
- tests/unit/models/test_config_service_tls_cors.py (Service/TLS/CORS)
- tests/unit/models/test_config_llama_mcp.py (LlamaStack/MCP)
If you need to unblock CI immediately, add at the top of the file:
"""Unit tests for functions defined in src/models/config.py.""" +# pylint: disable=too-many-lines # TODO: split this module into smaller files
🧹 Nitpick comments (3)
tests/unit/models/test_config.py (3)
832-834: Strengthen the positive-path assertion.Also assert identity with the underlying config to guarantee the property returns the exact object.
- assert auth_config.jwk_configuration is not None + assert auth_config.jwk_configuration is auth_config.jwk_config
865-883: Avoid mutating models into invalid states; usemodel_copy(update=...).Mutating
auth_config.jwk_config = Nonecan make debugging flakier. Prefer creating an updated copy.- # emulate broken config - auth_config.jwk_config = None + # emulate broken config without mutating the original instance + auth_config = auth_config.model_copy(update={"jwk_config": None})
953-959: Good negative test; fix comment typos.Minor: “propery” → “property” (twice).
- # access propery to call its getter + # access property to call its getter @@ - # access propery to call its getter + # access property to call its getter
📜 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)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/models/test_config.py (1)
src/models/config.py (5)
jwk_configuration(404-412)AuthenticationConfiguration(376-412)JwkConfiguration(369-373)config(129-135)db_type(120-126)
🪛 GitHub Actions: Python linter
tests/unit/models/test_config.py
[error] 5-5: pylint: C0302 Too many lines in module (1028/1000) (too-many-lines). Command: 'uv run pylint src tests'
⏰ 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 (3)
tests/unit/models/test_config.py (3)
808-814: Good negative coverage for non-JWK module access.The test correctly asserts that accessing
jwk_configurationon non-JWK modules raisesValueError.
926-927: LGTM: validatesDatabaseConfiguration.configfor Postgres.Using identity (
is) ensures the property returns the exact active config object.
937-938: LGTM: validatesDatabaseConfiguration.configfor SQLite.Same identity check here is appropriate.
tisnik
force-pushed
the
lcore-574-database-config-property-test
branch
from
cde3731 to
15b0c5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/unit/models/test_config.py (4)
3-4: OK to disable Pylint for long test module; consider future split.
Not blocking. If this file keeps growing, consider splitting by feature to avoid global disables.
869-887: Robustness test for broken JWK config is useful; prefer non-mutating pattern.
Minor: instead of mutating the model in-place, consider model_copy(update={"jwk_config": None}) to avoid side effects.- # emulate broken config - auth_config.jwk_config = None + # emulate broken config without mutating the original + broken = auth_config.model_copy(update={"jwk_config": None}) - # try to retrieve JWK configuration - with pytest.raises(ValueError, match="JWK configuration should not be None"): - _ = auth_config.jwk_configuration + # try to retrieve JWK configuration + with pytest.raises(ValueError, match="JWK configuration should not be None"): + _ = broken.jwk_configuration
957-963: Typo in comments ("propery" → "property").
Purely cosmetic; keeps comments clean.- # access propery to call its getter + # access property to call its getter ... - # access propery to call its getter + # access property to call its getter
1035-1145: Consider adding tests for remaining operators and negate logic.
Optional coverage ideas:
- JsonPathOperator.CONTAINS and .IN positive/negative paths.
- Behavior when
negate=Trueflips rule outcome.
📜 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(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/models/test_config.py (1)
src/models/config.py (8)
JwtRoleRule(241-299)JsonPathOperator(232-238)jwk_configuration(404-412)AuthenticationConfiguration(376-412)JwkConfiguration(369-373)config(129-135)db_type(120-126)compiled_regex(295-299)
⏰ 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). (1)
- GitHub Check: build-pr
🔇 Additional comments (15)
tests/unit/models/test_config.py (15)
26-27: Good: exercising new public JwtRoleRule and JsonPathOperator.
Imports align with src/models/config.py; coverage expansion looks appropriate.
812-818: Negative-path JWK accessor check is correct.
Asserting the precise ValueError for non-JWK module is valuable.
836-838: Positive-path JWK accessor check is correct.
Verifies accessor availability for JWK module.
930-931: Identity check for active Postgres config is correct.
Assertingisensures the property returns the exact configured instance.
941-942: Identity check for active SQLite config is correct.
Matches semantics of the newconfigproperty.
1033-1034: No action.
Blank lines only.
1035-1039: Missing-attributes validation test is appropriate.
Covers base model required fields behavior.
1041-1053: Happy-path rule with EQUALS operator is well covered.
Also correctly assertscompiled_regex is Nonewhen not MATCH.
1055-1065: Invalid JSONPath case is well asserted.
Message match is stable and focused.
1067-1078: No-roles validation is correct.
Ensures guardrail exists.
1081-1092: Wildcard role rejection covered.
Matches the spec forbidding '*'.
1095-1104: Duplicate roles validation covered.
Ensures uniqueness enforcement.
1107-1118: MATCH operator value-type validation covered.
Catches non-string patterns.
1121-1131: Regex compilation happy-path covered.
compiled_regexpresence check is sufficient.
1133-1144: Invalid regex path covered.
Good negative test.
tisnik
force-pushed
the
lcore-574-database-config-property-test
branch
from
15b0c5f to
6692f27
Compare
1 participant
Description
LCORE-574:
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Bug Fixes
Tests