-
Notifications
You must be signed in to change notification settings - Fork 29
Add support for sdk_ua_app_id. #653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,6 +96,25 @@ def test_different_properties_resolve_independently(self) -> None: | |
| assert region == "us-west-2" | ||
| assert retry_mode == "adaptive" | ||
|
|
||
| def test_unresolved_ua_app_id_defaults_to_none(self) -> None: | ||
| class ConfigWithNoDefault: | ||
| sdk_ua_app_id = ConfigProperty("sdk_ua_app_id") | ||
|
|
||
| def __init__(self, resolver: ConfigResolver) -> None: | ||
| self._resolver = resolver | ||
|
|
||
| source = StubSource("environment", {}) | ||
| resolver = ConfigResolver(sources=[source]) | ||
| config = ConfigWithNoDefault(resolver) | ||
|
|
||
| result = config.sdk_ua_app_id | ||
|
|
||
| assert result is None | ||
| assert getattr(config, "_cache_sdk_ua_app_id") == ( | ||
| None, | ||
| SimpleSource("default"), | ||
| ) | ||
|
|
||
|
|
||
| class TestConfigPropertyValidation: | ||
| """Test suite for ConfigProperty validation behavior.""" | ||
|
|
@@ -107,7 +126,6 @@ def _create_config_with_validator( | |
|
|
||
| class ConfigWithValidator: | ||
| region = ConfigProperty("region", validator=validator) | ||
| retry_strategy = ConfigProperty("retry_strategy", validator=validator) | ||
|
|
||
| def __init__(self, resolver: ConfigResolver) -> None: | ||
| self._resolver = resolver | ||
|
|
@@ -153,9 +171,7 @@ class ConfigWithComplexResolver: | |
| retry_strategy = ConfigProperty( | ||
| "retry_strategy", | ||
| resolver_func=mock_resolver, | ||
| default_value=RetryStrategyOptions( | ||
| retry_mode="standard", max_attempts=3 | ||
| ), | ||
| default_value=RetryStrategyOptions(retry_mode="standard"), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you help me understand why we are making these changes?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default value for retry_strategy should only specify retry_mode, since max_attempts will be determined later based on the selected retry mode. It would have worked either way though. |
||
| ) | ||
|
|
||
| def __init__(self, resolver: ConfigResolver) -> None: | ||
|
|
@@ -171,7 +187,7 @@ def __init__(self, resolver: ConfigResolver) -> None: | |
|
|
||
| assert isinstance(result, RetryStrategyOptions) | ||
| assert result.retry_mode == "standard" | ||
| assert result.max_attempts == 3 | ||
| assert result.max_attempts is None | ||
| assert source_info == SimpleSource("default") | ||
|
|
||
| def test_validator_not_called_on_cached_access(self) -> None: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| validate_max_attempts, | ||
| validate_region, | ||
| validate_retry_mode, | ||
| validate_ua_string, | ||
| ) | ||
|
|
||
|
|
||
|
|
@@ -49,3 +50,20 @@ def test_invalid_retry_mode_error_message(self) -> None: | |
| "Invalid value for 'retry_mode': 'random_mode'. retry_mode must be one " | ||
| "of ('standard',), got random_mode" in str(exc_info.value) | ||
| ) | ||
|
|
||
|
|
||
| class TestValidateUaString: | ||
| def test_allows_string(self) -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: all of these tests except the error test could easily be one test with pytest.mark.parametrize; everything is only validating that it's passed through, so the test body could just be |
||
| assert validate_ua_string("abc123") == "abc123" | ||
|
|
||
| def test_none_returns_none(self) -> None: | ||
| assert validate_ua_string(None) is None | ||
|
|
||
| def test_empty_string_passthrough(self) -> None: | ||
| assert validate_ua_string("") == "" | ||
|
|
||
| def test_rejects_non_string(self) -> None: | ||
| with pytest.raises(ConfigValidationError) as exc_info: | ||
| validate_ua_string(123) | ||
|
|
||
| assert exc_info.value.key == "sdk_ua_app_id" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me understand what value this test provides? Are we just testing that the default works?
I don't think this test has anything to do with the other code changes- again, unless you changed
ConfigPropertyand broke it, this test wouldn't fail? There's nothing about this PR that is being tested here, so I'd hope we already had proper tests on TestConfigProperty