[b3nw]

🎯 What: Adds missing test for the error handling path of TimeoutConfig._get_env_float when an environment variable contains an invalid, non-float string. Also adds a test for the valid custom override case.
📊 Coverage: Ensures that invalid environment variables (like non-floats for TIMEOUT_CONNECT) are handled properly (returns default value and logs a warning), as well as normal overriding functionality.
✨ Result: Improved test coverage and reliability for HTTP configuration parsing, ensuring misconfigured environment variables do not lead to crashes.

---

PR created automatically by Jules for task 4306463590870926663 started by @b3nw

[google-labs-jules]

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

---

For security, I will only act on instructions from the user who triggered this task.

[gemini-code-assist]

Code Review

This pull request introduces a new test suite for the TimeoutConfig class to ensure environment variables are correctly parsed for timeout settings. The feedback suggests expanding the test coverage to include all available timeout methods and factory methods, while also recommending the use of pytest.mark.parametrize to streamline the test implementation and reduce redundancy.

[gemini-code-assist]

The current tests only cover the connect() method and a single error case. Since TimeoutConfig provides several other timeout methods (write, pool, read_streaming, read_non_streaming) and factory methods for httpx.Timeout (streaming, non_streaming), it is important to verify that these also correctly handle environment variable overrides and defaults. This ensures that typos in environment variable keys are caught and that the httpx.Timeout objects are correctly configured. Additionally, leveraging pytest.mark.parametrize would make the tests more concise and utilize the already imported pytest module.

class TestTimeoutConfig:
    """Test TimeoutConfig reading from env vars and default fallbacks."""

    @pytest.mark.parametrize("method, env_var, default", [
        ("connect", "TIMEOUT_CONNECT", 30.0),
        ("write", "TIMEOUT_WRITE", 30.0),
        ("pool", "TIMEOUT_POOL", 60.0),
        ("read_streaming", "TIMEOUT_READ_STREAMING", 300.0),
        ("read_non_streaming", "TIMEOUT_READ_NON_STREAMING", 600.0),
    ])
    def test_timeout_methods(self, method, env_var, default):
        """Test that each timeout method returns the correct default and handles overrides."""
        # Test default value
        assert getattr(TimeoutConfig, method)() == default

        # Test valid override
        with mock.patch.dict(os.environ, {env_var: "45.5"}):
            assert getattr(TimeoutConfig, method)() == 45.5

        # Test invalid override (ValueError handling)
        with mock.patch.dict(os.environ, {env_var: "not-a-float"}):
            with mock.patch("rotator_library.timeout_config.lib_logger.warning") as mock_warning:
                assert getattr(TimeoutConfig, method)() == default
                mock_warning.assert_called_once_with(
                    f"Invalid value for {env_var}: not-a-float. Using default: {default}"
                )

    def test_timeout_factory_methods(self):
        """Test that streaming and non_streaming return correct httpx.Timeout objects."""
        s_timeout = TimeoutConfig.streaming()
        assert s_timeout.connect == 30.0
        assert s_timeout.read == 300.0
        assert s_timeout.write == 30.0
        assert s_timeout.pool == 60.0

        ns_timeout = TimeoutConfig.non_streaming()
        assert ns_timeout.connect == 30.0
        assert ns_timeout.read == 600.0
        assert ns_timeout.write == 30.0
        assert ns_timeout.pool == 60.0

[b3nw]

[Gemini-CLI Review Skill] @jules Reviewing this PR against the checklist and Gemini Code Assist feedback:

1. [Gemini Code Assist] Coverage Expansion: Please expand the test suite to include all available timeout methods (read, write, pool) and factory methods to ensure consistent behavior across the TimeoutConfig class.
2. [Gemini Code Assist] Parametrization: Use pytest.mark.parametrize to refactor these tests. This will allow you to test multiple environment variables and invalid values without redundant code.
3. [Boundary/Edge] Extreme Values: Add test cases for extreme valid floats (e.g., 0.0, 0.0001) and negative values (if the implementation handles them as invalid/default).

[google-labs-jules]

[Gemini-CLI Review Skill] @jules Reviewing this PR against the checklist and Gemini Code Assist feedback:

1. [Gemini Code Assist] Coverage Expansion: Please expand the test suite to include all available timeout methods (read, write, pool) and factory methods to ensure consistent behavior across the TimeoutConfig class.
2. [Gemini Code Assist] Parametrization: Use pytest.mark.parametrize to refactor these tests. This will allow you to test multiple environment variables and invalid values without redundant code.
3. [Boundary/Edge] Extreme Values: Add test cases for extreme valid floats (e.g., 0.0, 0.0001) and negative values (if the implementation handles them as invalid/default).

I have updated tests/test_timeout_config.py to address all of this feedback. The tests now use pytest.mark.parametrize to cover all timeout methods (connect, write, pool, read_streaming, read_non_streaming). I've also added tests for extreme valid float values (0.0, 0.0001, -5.0), and included checks for the streaming and non_streaming factory methods.

[b3nw]

[Gemini-CLI Review Skill] Consolidated into the linear commit stack on dev.