LCORE-1028: unit tests for quota scheduler configuration model#827
Conversation
WalkthroughAdds a public alias Changes
Sequence Diagram(s)sequenceDiagram
participant Tests as Tests
participant AuthConfig as AuthenticationConfiguration
participant RHConfig as RHIdentityConfiguration
note right of AuthConfig `#E8F6FF`: Alias accessor
Tests->>AuthConfig: access rh_identity_configuration
AuthConfig->>RHConfig: return rh_identity_config (alias)
Tests->>RHConfig: inspect required_entitlements
RHConfig-->>Tests: returns None (when unspecified)
rect rgba(200,255,200,0.25)
note right of RHConfig: Validation for missing RH config
Tests->>AuthConfig: validate presence of rh_identity_config
AuthConfig-->>Tests: raise ValidationError: "RH Identity configuration must be specified"
end
sequenceDiagram
participant Tests as Tests
participant QuotaCfg as QuotaSchedulerConfiguration
note right of QuotaCfg `#FFF4E6`: Period validation (>0)
Tests->>QuotaCfg: construct with period = 0 or -10
QuotaCfg-->>Tests: raise ValidationError "Input should be greater than 0"
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/models/config/test_quota_scheduler_config.py (2)
25-28: Consider improving the docstring for clarity.The test correctly validates that
period=0raises aValidationError. However, the docstring "Test the custom configuration." is generic and doesn't describe what this test specifically validates. Consider making it more descriptive.Apply this diff to improve the docstring:
-def test_quota_scheduler_custom_configuration_zero_period() -> None: - """Test the custom configuration.""" +def test_quota_scheduler_custom_configuration_zero_period() -> None: + """Test that zero period value raises ValidationError."""
31-34: Consider improving the docstring for clarity.The test correctly validates that negative
periodvalues raise aValidationError. However, the docstring "Test the custom configuration." is generic and doesn't describe what this test specifically validates. Consider making it more descriptive.Apply this diff to improve the docstring:
-def test_quota_scheduler_custom_configuration_negative_period() -> None: - """Test the custom configuration.""" +def test_quota_scheduler_custom_configuration_negative_period() -> None: + """Test that negative period value raises ValidationError."""
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/models/config/test_authentication_configuration.py(4 hunks)tests/unit/models/config/test_quota_scheduler_config.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/models/config/test_quota_scheduler_config.py (1)
src/models/config.py (1)
QuotaSchedulerConfiguration(605-608)
tests/unit/models/config/test_authentication_configuration.py (1)
src/models/config.py (1)
rh_identity_configuration(465-473)
⏰ 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). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (6)
tests/unit/models/config/test_quota_scheduler_config.py (1)
3-5: LGTM!The imports are correctly added to support the new validation tests.
tests/unit/models/config/test_authentication_configuration.py (5)
67-68: LGTM!The assertions correctly verify that
rh_identity_configurationis a property alias that returns the samerh_identity_configobject and thatrequired_entitlementsis set to an empty list as configured.
87-88: LGTM!The assertions correctly verify the default behavior where
required_entitlementsisNonewhenRHIdentityConfigurationis instantiated without arguments.
107-108: LGTM!The assertions correctly verify that
required_entitlementsis set to["foo"]when explicitly configured with a single entitlement.
129-134: LGTM!The assertions correctly verify that
required_entitlementsis set to the configured list of multiple entitlements. The multi-line formatting improves readability.
140-142: LGTM!The test correctly validates that a
ValidationErroris raised with the expected message whenAUTH_MOD_RH_IDENTITYis specified without the requiredrh_identity_config. This appears to be testing Pydantic model-level validation.
3cf0aef to
70bea89
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/models/config/test_authentication_configuration.py (1)
67-68: Consider adding error case tests forrh_identity_configurationproperty.The tests thoroughly cover the happy path for the new
rh_identity_configurationalias, but error cases are missing. Based on the relevant code snippet and the pattern established forjwk_configuration(lines 44-48 and 200-217), consider adding tests for:
- Accessing
rh_identity_configurationwhenmoduleis notAUTH_MOD_RH_IDENTITY(should raise ValueError with message: "RH Identity configuration is only available for RH Identity authentication module")- Accessing
rh_identity_configurationwhenrh_identity_configisNonebut module isRH_IDENTITY(broken config scenario - should raise ValueError with message: "RH Identity configuration should not be None")These tests would ensure the property's validation behavior is properly covered, similar to the JWK configuration tests.
Based on relevant code snippets showing the property implementation.
Example test for case 1:
def test_authentication_configuration_rh_identity_config_wrong_module() -> None: """Test accessing rh_identity_configuration with wrong module.""" auth_config = AuthenticationConfiguration( module=AUTH_MOD_NOOP, skip_tls_verification=False, k8s_ca_cert_path=None, k8s_cluster_api=None, ) with pytest.raises( ValueError, match="RH Identity configuration is only available for RH Identity authentication module", ): _ = auth_config.rh_identity_configurationExample test for case 2:
def test_authentication_configuration_rh_identity_broken_config() -> None: """Test accessing rh_identity_configuration with None config.""" auth_config = AuthenticationConfiguration( module=AUTH_MOD_RH_IDENTITY, skip_tls_verification=False, k8s_ca_cert_path=None, k8s_cluster_api=None, rh_identity_config=RHIdentityConfiguration(), ) # Emulate broken config auth_config.rh_identity_config = None with pytest.raises( ValueError, match="RH Identity configuration should not be None", ): _ = auth_config.rh_identity_configuration
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/models/config/test_authentication_configuration.py(4 hunks)tests/unit/models/config/test_quota_scheduler_config.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/config/test_quota_scheduler_config.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/models/config/test_authentication_configuration.py (1)
src/models/config.py (1)
rh_identity_configuration(465-473)
⏰ 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). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (4)
tests/unit/models/config/test_authentication_configuration.py (4)
67-68: LGTM! Correct alias verification.The assertions properly verify that
rh_identity_configurationis an alias (same object reference) and that explicitly provided empty entitlements are preserved.
87-88: LGTM! Correct default value verification.The assertions properly test the new default behavior where
required_entitlementsisNonewhen not explicitly specified.
107-108: LGTM! Single entitlement scenario covered.The assertions properly verify that a single entitlement in the list is preserved.
129-134: LGTM! Multiple entitlements scenario covered.The assertions properly verify that multiple entitlements are preserved. The multi-line formatting enhances readability.
| with pytest.raises( | ||
| ValidationError, match="RH Identity configuration must be specified" | ||
| ): |
There was a problem hiding this comment.
🧩 Analysis chain
Verify error message format consistency.
The validation error test looks correct, but there's a potential inconsistency: the JWK validation error test on line 189 matches "Value error, JWK configuration must be specified..." with the "Value error, " prefix, while this test matches "RH Identity configuration must be specified" without that prefix.
If both validators are the same type (e.g., both using @model_validator), they should have consistent error message formats. Please verify this is intentional.
🏁 Script executed:
#!/bin/bash
# Description: Check how ValidationError messages are raised in the model code
# to verify if the "Value error, " prefix inconsistency is intentional
# Search for RH Identity validation logic
echo "=== RH Identity validation ==="
rg -n -A5 -B2 "RH Identity configuration must be specified" src/
# Search for JWK validation logic
echo "=== JWK validation ==="
rg -n -A5 -B2 "JWK configuration must be specified" src/
# Check for model_validator or field_validator usage patterns
echo "=== Validator decorators in config.py ==="
rg -n -B2 -A8 "@(model_validator|field_validator)" src/models/config.py | head -100Length of output: 5096
Fix error message match pattern to include Pydantic's "Value error, " prefix.
The test is checking for the wrong error message format. Both the RH Identity and JWK validators raise ValueError inside @model_validator methods. When Pydantic catches a ValueError in a model validator, it wraps it as a ValidationError and prefixes the message with "Value error, ".
The test at line 140-142 should match "Value error, RH Identity configuration must be specified" instead of just "RH Identity configuration must be specified". Without the prefix, the test will fail to catch the exception.
🤖 Prompt for AI Agents
In tests/unit/models/config/test_authentication_configuration.py around lines
140 to 142, the pytest.raises match string misses Pydantic's "Value error, "
prefix; update the match to "Value error, RH Identity configuration must be
specified" so the test matches the ValidationError message Pydantic produces
when a ValueError is raised in a model_validator.
Description
LCORE-1028: unit tests for quota scheduler configuration model
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.