Skip to content

Conversation

@jsamuel1
Copy link
Contributor

Overview

This PR adds a comprehensive test suite for the critical Bedrock configuration fix implemented in PR #11. The original issue caused AttributeError: 'dict' object has no attribute 'merge' when launching Strands agents with JSON-parsed Bedrock configurations.

Problem Statement

PR #11 fixed the bug where boto_client_config parameters from JSON configuration files were passed as plain dictionaries instead of BotocoreConfig objects, causing runtime errors. However, this critical fix lacked comprehensive test coverage to:

  • Verify the fix works correctly in all scenarios
  • Prevent future regressions
  • Document the expected behavior
  • Validate edge cases and error conditions

Solution

This PR adds 20 comprehensive tests organized in 4 logical test classes:

🧪 Test Coverage

1. TestBedrockInstance (10 tests)

  • Core functionality: Dict → BotocoreConfig conversion
  • Backward compatibility: Existing BotocoreConfig objects unchanged
  • Edge cases: None values, empty dictionaries
  • Complex scenarios: Real-world configuration validation
  • Immutability: Original config objects remain unmodified

2. TestBedrockConfigConversion (4 tests)

  • Type validation: BotocoreConfig creation from various dict structures
  • Detection logic: isinstance() verification for both dict and BotocoreConfig
  • Parameter validation: Various configuration combinations

3. TestBedrockIntegrationScenarios (3 tests)

  • JSON parsing simulation: Exact failing scenario from user reports
  • Strands launch configs: Typical agent configuration patterns
  • Mixed sources: JSON + code configuration combinations

4. TestRegressionPrevention (3 tests)

  • Root cause demonstration: Shows the original AttributeError
  • Fix verification: Confirms the fix prevents the error
  • Attribute validation: Verifies merge() method availability

Key Test Scenarios

✅ Main Fix Verification

# This would have failed before PR #11
config_dict = {"read_timeout": 900, "retries": {"max_attempts": 3}}
result = bedrock.instance(model_id="test", boto_client_config=config_dict)
# Now passes: dict is automatically converted to BotocoreConfig

✅ JSON Configuration Scenario

# Simulates real-world JSON config parsing that was failing
json_config = '{"model_id": "test", "boto_client_config": {"read_timeout": 900}}'
parsed_config = json.loads(json_config)  # Creates dict, not BotocoreConfig
result = bedrock.instance(**parsed_config)  # Now works with PR #11 fix

✅ Error Demonstration

# Shows what was happening before the fix
config_dict = {"read_timeout": 900}
with pytest.raises(AttributeError, match="'dict' object has no attribute 'merge'"):
    config_dict.merge({})  # This was the root cause

Changes Made

Files Added

  • tests/test_models_bedrock.py - Complete test suite (590 lines, 20 tests)

Test Quality Features

  • Comprehensive mocking: All BedrockModel calls properly isolated
  • Realistic scenarios: Tests simulate actual user configurations
  • Error demonstration: Shows exactly what would happen without the fix
  • Type safety: Verifies proper isinstance() detection and conversions
  • Immutability: Ensures original configuration objects aren't modified
  • Documentation: Clear test names and comprehensive docstrings

Test Results

$ python -m pytest tests/test_models_bedrock.py -v
============================== test session starts ==============================
collected 20 items

tests/test_models_bedrock.py::TestBedrockInstance::test_instance_with_dict_boto_client_config PASSED [  5%]
tests/test_models_bedrock.py::TestBedrockInstance::test_instance_with_botocore_config_object PASSED [ 10%]
# ... (all 20 tests)
============================== 20 passed in 0.05s ==============================

Benefits

  1. 🛡️ Regression Protection: Future changes cannot break this critical functionality
  2. 📚 Documentation: Tests serve as living documentation of expected behavior
  3. 🔍 Debugging Aid: Clear test failures will pinpoint issues immediately
  4. ✅ Confidence: Comprehensive coverage ensures the fix works in all scenarios
  5. 🚀 Maintainability: Well-organized tests make future maintenance easier

Relationship to PR #11

This PR directly supports PR #11 by:

  • Validating the fix with comprehensive test coverage
  • Preventing regressions through automated testing
  • Documenting behavior for future developers
  • Ensuring reliability for production deployments

Testing Instructions

# Run all new Bedrock tests
python -m pytest tests/test_models_bedrock.py -v

# Run specific test class
python -m pytest tests/test_models_bedrock.py::TestBedrockInstance -v

# Run individual test
python -m pytest tests/test_models_bedrock.py::TestBedrockInstance::test_instance_with_dict_boto_client_config -v

Backwards Compatibility

No breaking changes - This PR only adds tests
No production impact - Tests run in isolation with mocking
Fast execution - All 20 tests complete in 0.05 seconds


Closes: Testing gap for PR #11
References: #11 (Bedrock boto_client_config fix)
Type: Test Enhancement
Risk: Low (test-only changes)

- Add 20 tests covering dict->BotocoreConfig conversion (ref strands-agents#11)
- Test backward compatibility with existing BotocoreConfig objects
- Validate JSON config parsing scenarios that were failing
- Include regression prevention tests
- Demonstrate original AttributeError and verify fix prevents it
- Cover edge cases: None values, empty dicts, complex configurations
- Organize tests in logical classes for maintainability

Fixes the testing gap for the critical Bedrock configuration bug
that prevented users from launching Strands with JSON-parsed configs.
@jsamuel1 jsamuel1 requested a review from a team as a code owner May 27, 2025 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant