Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
239 changes: 239 additions & 0 deletions packages/smithy-aws-core/tests/functional/test_config_resolution.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,239 @@
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
# SPDX-License-Identifier: Apache-2.0

from typing import Any

import pytest
from pytest import MonkeyPatch
from smithy_aws_core.config.custom_resolvers import resolve_retry_strategy
from smithy_aws_core.config.sources import EnvironmentSource
from smithy_aws_core.config.validators import (
ConfigValidationError,
validate_max_attempts,
validate_region,
validate_retry_mode,
)
from smithy_core.config.property import ConfigProperty
from smithy_core.config.resolver import ConfigResolver
from smithy_core.interfaces.config import ConfigSource
from smithy_core.retries import RetryStrategyOptions


class BaseTestConfig:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that we need this class to simulate the code-generated Config class. Let's add a top-level TODO that these tests will need to be refactored once we have a way to generate test clients. If we use this permanently, there is a good chance there will be drift if we change how the Config class is generated.

There is a similar note in our retries functional tests in smithy-core.

"""Base config class with common functionality for tests."""

_resolver: ConfigResolver

def __init__(self, sources: list[ConfigSource] | None = None) -> None:
if sources is None:
sources = [EnvironmentSource()]
self._resolver = ConfigResolver(sources=sources)

def get_source(self, key: str) -> str | None:
cached = self.__dict__.get(f"_cache_{key}")
return cached[1] if cached else None


def make_config_class(properties: dict[str, ConfigProperty]) -> Any:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should create different Config objects for different functional tests. It seems fragile to do so if we want to test the actual Config class we are generating, which should be consistent from client to client.

It may be better to have one TestConfig class that simulates the code generator and is the source of truth, with all the relevant ConfigProperty descriptors for region, retries, etc. We then use that class to make sure it handles the different test scenarios.

Curious what other reviewers think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functional tests live in smithy-aws-core which is shared by many generated clients. Clients can have different set of config properties. By using dynamic classes, the tests are validating that the different components are working correctly regardless of which properties are present. The factory function mirrors what the code generator does by taking a set of config properties and producing a config class with those descriptors. This lets us validate the resolution mechanism independent of any specific client's config shape.

Being said that, I can also see the value in a single static class for readability. Happy to revisit if others feel strongly, but I think the current approach gives us more flexibility for a shared library.

"""Factory function to create a config class with specified properties.

:param properties: Dict mapping property names to ConfigProperty instances

:returns: A new config class with the specified properties
"""
class_dict: dict[str, Any] = {
"__init__": BaseTestConfig.__init__,
"get_source": BaseTestConfig.get_source,
}
class_dict.update(properties)

return type("TestConfig", (BaseTestConfig,), class_dict)


class TestConfigResolution:
"""Functional tests for complete config resolution flow."""

def test_environment_var_resolution(self, monkeypatch: MonkeyPatch) -> None:
TestConfig = make_config_class(
{"region": ConfigProperty("region", default_value="us-east-1")}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From #646, it looks like we are not setting a default value for region. We should not be including that here since it doesn't match our code generator's behavior.

I code generated the new Config object and noticed we are also missing the validator.

Should we be using this instead for all region tests?

"region": ConfigProperty("region", validator=validate_region)

Looks like we also inconsistent in lines 69, 108, and 128.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validators and custom resolvers are intentionally included only in tests that specifically use those behaviors. Tests without them are focused on other aspects of config resolution. So that's intentional.

Good catch on the region default value. I'll remove it since region no longer has a default.

)
monkeypatch.setenv("AWS_REGION", "eu-west-1")

config = TestConfig()

assert config.region == "eu-west-1"
assert config.get_source("region") == "environment"

def test_instance_overrides_environment(self, monkeypatch: MonkeyPatch) -> None:
TestConfig = make_config_class(
{"region": ConfigProperty("region", default_value="us-east-1")}
)

config = TestConfig()
config.region = "us-west-2"

monkeypatch.setenv("AWS_REGION", "eu-west-1")

assert config.region == "us-west-2"
assert config.get_source("region") == "instance"

def test_complex_resolution_with_custom_resolver(
self, monkeypatch: MonkeyPatch
) -> None:
TestConfig = make_config_class(
{
"retry_strategy": ConfigProperty(
Copy link
Copy Markdown
Contributor

@arandito arandito Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ConfigProperty also doesn't match the generated one for retry_strategy. It's missing the validator parameter.

This is the code generator output:

  "retry_strategy": ConfigProperty(
      "retry_strategy",
      validator=validate_retry_strategy,
      resolver_func=resolve_retry_strategy,
      default_value=RetryStrategyOptions(retry_mode="standard"),
  )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

"retry_strategy",
resolver_func=resolve_retry_strategy,
default_value=RetryStrategyOptions(retry_mode="standard"),
)
}
)

monkeypatch.setenv("AWS_RETRY_MODE", "standard")
monkeypatch.setenv("AWS_MAX_ATTEMPTS", "5")

config = TestConfig()

retry_strategy = config.retry_strategy
assert isinstance(retry_strategy, RetryStrategyOptions)
assert retry_strategy.retry_mode == "standard"
assert retry_strategy.max_attempts == 5

source = config.get_source("retry_strategy")
assert source == "retry_mode=environment, max_attempts=environment"

def test_caching_behavior(self, monkeypatch: MonkeyPatch) -> None:
TestConfig = make_config_class(
{"region": ConfigProperty("region", default_value="us-east-1")}
)

monkeypatch.setenv("AWS_REGION", "ap-south-1")

config = TestConfig()

region1 = config.region

monkeypatch.setenv("AWS_REGION", "eu-central-1")

region2 = config.region
# The first value for region which is cached is returned
assert region1 == region2 == "ap-south-1"

@pytest.mark.parametrize(
"property_name,property_config,expected_value",
[
(
"region",
ConfigProperty("region", default_value="us-east-1"),
"us-east-1",
),
(
"retry_mode",
ConfigProperty(
Copy link
Copy Markdown
Contributor

@arandito arandito Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is retry_mode a ConfigProperty currently? I only see region and retry_strategy in the generated code. Will it be added?

Same question with max-attempts in line 217.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, retry_mode isn't in config property but both retry_mode and max_attempts are used in retry_strategy. This test is testing a behavior that it uses a default value as expected.

"retry_mode",
validator=validate_retry_mode,
default_value="standard",
),
"standard",
),
],
)
def test_uses_default_when_no_sources(
self, property_name: str, property_config: ConfigProperty, expected_value: str
) -> None:
TestConfig = make_config_class({property_name: property_config})
config = TestConfig(sources=[])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is sources supposed to be part of the Config constructor?

It is defined as a property of ConfigResolver not Config. This mixes both abstractions and is confusing. I'm also not sure we ever generate a Config object where sources is empty.

This also true for lines 209 and 236.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed a simplest way to test a scenario where the resolver isn't able to resolve the config value for any of the given sources while also matching with the config resolution behavior as closely as possible. Passing sources=[] was the simplest way to achieve this. I will add a comment clarifying that this is test only.


assert getattr(config, property_name) == expected_value
assert config.get_source(property_name) == "default"

def test_default_value_for_complex_resolution(
self, monkeypatch: MonkeyPatch
) -> None:
TestConfig = make_config_class(
{
"retry_strategy": ConfigProperty(
"retry_strategy",
resolver_func=resolve_retry_strategy,
default_value=RetryStrategyOptions(retry_mode="standard"),
)
}
)

config = TestConfig()

retry_strategy = config.retry_strategy
assert isinstance(retry_strategy, RetryStrategyOptions)
assert retry_strategy.retry_mode == "standard"
# None for max_attempts means the RetryStrategy will use its
# own default max_attempts value for the set retry_mode
assert retry_strategy.max_attempts is None
source = config.get_source("retry_strategy")
assert source == "default"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is a bit weird to me. I would expect retry_strategy.max_attempts to return the default value instead of None. We now have hidden behavior that doesn't make it clear that retry_strategy.max_attempts=None means that we are using the default value. I would also expect source to be "retry_mode=default max_attempts=default".

I may be missing a decision that was made that required this behvaior, so feel free to explain it in case I'm missing something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When neither retry_mode nor max_attempts are found in any source, resolve_retry_strategy returns (None, None) and the descriptor falls back to the default value object: RetryStrategyOptions(retry_mode="standard"). Since max_attempts isn't set on this default object, it's None at the config layer. The actual default value (max_attempts=3) gets applied later when create_retry_strategy is called. That is why the comment.

And similarly, the source is only default because it used the whole default value object instead of getting individual components from different sources.


def test_retry_strategy_combines_multiple_sources(
self, monkeypatch: MonkeyPatch
) -> None:
TestConfig = make_config_class(
{
"retry_strategy": ConfigProperty(
"retry_strategy",
resolver_func=resolve_retry_strategy,
default_value=RetryStrategyOptions(retry_mode="standard"),
)
}
)

monkeypatch.setenv("AWS_MAX_ATTEMPTS", "10")
config = TestConfig()

retry_strategy = config.retry_strategy
assert retry_strategy.retry_mode == "standard"
assert retry_strategy.max_attempts == 10
assert (
config.get_source("retry_strategy")
== "retry_mode=default, max_attempts=environment"
)

def test_validation_error_when_value_assigned(self) -> None:
TestConfig = make_config_class(
{
"retry_mode": ConfigProperty(
"retry_mode",
validator=validate_retry_mode,
default_value="standard",
)
}
)
config = TestConfig(sources=[])

with pytest.raises(ConfigValidationError, match="retry_mode must be one of"):
config.retry_mode = "invalid_mode"

def test_validation_error_during_resolution(self, monkeypatch: MonkeyPatch) -> None:
TestConfig = make_config_class(
{
"max_attempts": ConfigProperty(
"max_attempts", validator=validate_max_attempts, default_value=3
)
}
)

monkeypatch.setenv("AWS_MAX_ATTEMPTS", "invalid")

config = TestConfig()

with pytest.raises(
ConfigValidationError, match="max_attempts must be a number"
):
config.max_attempts

def test_region_validation_fails_when_none(self) -> None:
TestConfig = make_config_class(
{"region": ConfigProperty("region", validator=validate_region)}
)
config = TestConfig(sources=[])

with pytest.raises(ConfigValidationError, match="region not found"):
config.region
26 changes: 26 additions & 0 deletions packages/smithy-core/tests/unit/config/test_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# SPDX-License-Identifier: Apache-2.0
from typing import Any

import pytest
from smithy_core.config.resolver import ConfigResolver


Expand Down Expand Up @@ -110,3 +111,28 @@ def test_treats_empty_string_as_valid_value(self):

assert value == ""
assert source_name == "test"


class TestInvalidSourceHandling:
"""Tests for handling invalid or malformed config sources."""

def test_invalid_source_missing_get_method(self):
class InvalidSource:
name = "invalid"
# Missing get() method

resolver = ConfigResolver(sources=[InvalidSource()]) # type: ignore

with pytest.raises(AttributeError):
resolver.get("region")

def test_invalid_source_missing_name_property(self):
class InvalidSource:
# Missing source name property
def get(self, key: str):
return "value"

resolver = ConfigResolver(sources=[InvalidSource()]) # type: ignore

with pytest.raises(AttributeError):
resolver.get("region")
Loading