codeant-ai-for-open-source[bot] commented on code in PR #41924: URL: https://github.com/apache/superset/pull/41924#discussion_r3573710155
########## tests/unit_tests/mcp_service/utils/test_retry_utils.py: ########## @@ -0,0 +1,396 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Unit tests for MCP service retry utilities. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from sqlalchemy.exc import OperationalError + +from superset.mcp_service.utils.retry_utils import ( + async_retry_database_operation, + async_retry_on_exception, + exponential_backoff, + retry_database_operation, + retry_on_exception, + retry_screenshot_operation, + RetryableOperation, +) + +SLEEP = "superset.mcp_service.utils.retry_utils.time.sleep" Review Comment: **Suggestion:** Add an explicit type annotation to this module-level constant to satisfy the type-hint requirement for annotatable variables. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This is a newly added Python module-level variable that can be annotated, but it is assigned without a type hint. That matches the type-hint rule for annotatable variables. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c9ad69218a8c4aee98007dceee740451&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c9ad69218a8c4aee98007dceee740451&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/utils/test_retry_utils.py **Line:** 37:37 **Comment:** *Custom Rule: Add an explicit type annotation to this module-level constant to satisfy the type-hint requirement for annotatable variables. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=2955976c212be97b076e64fd23528282ad20d6d3cb06d5d2266366199bd2df00&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=2955976c212be97b076e64fd23528282ad20d6d3cb06d5d2266366199bd2df00&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/utils/test_retry_utils.py: ########## @@ -0,0 +1,396 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Unit tests for MCP service retry utilities. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from sqlalchemy.exc import OperationalError + +from superset.mcp_service.utils.retry_utils import ( + async_retry_database_operation, + async_retry_on_exception, + exponential_backoff, + retry_database_operation, + retry_on_exception, + retry_screenshot_operation, + RetryableOperation, +) + +SLEEP = "superset.mcp_service.utils.retry_utils.time.sleep" +ASYNC_SLEEP = "superset.mcp_service.utils.retry_utils.asyncio.sleep" Review Comment: **Suggestion:** Add an explicit type annotation to this module-level constant to keep new variables consistently type-hinted. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This is another new module-level constant in a Python file that could be type-annotated, but it is declared without a type hint. The custom rule applies here. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=42a81f1ad3a04dbfaa266aee78759847&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=42a81f1ad3a04dbfaa266aee78759847&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/utils/test_retry_utils.py **Line:** 38:38 **Comment:** *Custom Rule: Add an explicit type annotation to this module-level constant to keep new variables consistently type-hinted. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=024c7546d07c9e0584436d44008de669903d0c97e87f218c25e1b5665f0ecce7&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=024c7546d07c9e0584436d44008de669903d0c97e87f218c25e1b5665f0ecce7&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/utils/test_retry_utils.py: ########## @@ -0,0 +1,396 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Unit tests for MCP service retry utilities. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from sqlalchemy.exc import OperationalError + +from superset.mcp_service.utils.retry_utils import ( + async_retry_database_operation, + async_retry_on_exception, + exponential_backoff, + retry_database_operation, + retry_on_exception, + retry_screenshot_operation, + RetryableOperation, +) + +SLEEP = "superset.mcp_service.utils.retry_utils.time.sleep" +ASYNC_SLEEP = "superset.mcp_service.utils.retry_utils.asyncio.sleep" + + +# --------------------------------------------------------------------------- +# exponential_backoff +# --------------------------------------------------------------------------- + + +def test_exponential_backoff_without_jitter_doubles_each_attempt() -> None: + """Should double the delay for each subsequent attempt when jitter is off.""" + assert exponential_backoff(0, base_delay=1.0, jitter=False) == 1.0 + assert exponential_backoff(1, base_delay=1.0, jitter=False) == 2.0 + assert exponential_backoff(2, base_delay=1.0, jitter=False) == 4.0 + + +def test_exponential_backoff_caps_at_max_delay() -> None: + """Should never return a delay larger than max_delay.""" + delay = exponential_backoff(10, base_delay=1.0, max_delay=5.0, jitter=False) + assert delay == 5.0 + + +def test_exponential_backoff_jitter_stays_within_twenty_five_percent() -> None: + """Should keep jittered delays within +/-25% of the base delay.""" + for _ in range(50): + delay = exponential_backoff(2, base_delay=1.0, jitter=True) + # Unjittered delay for attempt=2, base_delay=1.0 is 4.0. + assert 3.0 <= delay <= 5.0 + + +def test_exponential_backoff_never_negative() -> None: + """Should clamp the delay to be non-negative even with jitter applied.""" + for _ in range(50): + delay = exponential_backoff(0, base_delay=0.001, jitter=True) + assert delay >= 0 + + +# --------------------------------------------------------------------------- +# retry_on_exception (sync) +# --------------------------------------------------------------------------- + + +def test_retry_on_exception_succeeds_first_try() -> None: + """Should call the wrapped function once when it succeeds immediately.""" + mock_func = MagicMock(return_value="ok") + wrapped = retry_on_exception(max_attempts=3)(mock_func) + + result = wrapped() + + assert result == "ok" + assert mock_func.call_count == 1 + + +def test_retry_on_exception_retries_then_succeeds() -> None: + """Should retry on retryable exceptions and return the eventual success.""" + mock_func = MagicMock( + side_effect=[ConnectionError("fail"), ConnectionError("fail"), "ok"] + ) + mock_func.__name__ = "mock_func" + + with patch(SLEEP) as mock_sleep: + wrapped = retry_on_exception(max_attempts=3, base_delay=0.01, jitter=False)( + mock_func + ) + result = wrapped() + + assert result == "ok" + assert mock_func.call_count == 3 + assert mock_sleep.call_count == 2 + + +def test_retry_on_exception_exhausts_retries_and_raises_last_exception() -> None: + """Should raise the last retryable exception once max_attempts is reached.""" + mock_func = MagicMock(side_effect=ConnectionError("always fails")) + mock_func.__name__ = "mock_func" + + with patch(SLEEP) as mock_sleep: + wrapped = retry_on_exception(max_attempts=3, base_delay=0.01, jitter=False)( + mock_func + ) + with pytest.raises(ConnectionError, match="always fails"): + wrapped() + + assert mock_func.call_count == 3 + # No sleep after the final (failed) attempt. + assert mock_sleep.call_count == 2 + + +def test_retry_on_exception_non_retryable_exception_fails_immediately() -> None: + """Should not retry exceptions outside the configured retryable tuple.""" + mock_func = MagicMock(side_effect=ValueError("bad input")) + mock_func.__name__ = "mock_func" + + with patch(SLEEP) as mock_sleep: + wrapped = retry_on_exception(max_attempts=3, exceptions=(ConnectionError,))( + mock_func + ) + with pytest.raises(ValueError, match="bad input"): + wrapped() + + assert mock_func.call_count == 1 + mock_sleep.assert_not_called() + + +def test_retry_on_exception_respects_custom_exceptions_tuple() -> None: + """Should retry only on the exception types passed via `exceptions`.""" + mock_func = MagicMock(side_effect=[TimeoutError("timed out"), "ok"]) + mock_func.__name__ = "mock_func" + + with patch(SLEEP): + wrapped = retry_on_exception( + max_attempts=2, base_delay=0.01, jitter=False, exceptions=(TimeoutError,) + )(mock_func) + result = wrapped() + + assert result == "ok" + assert mock_func.call_count == 2 + + +def test_retry_on_exception_max_attempts_zero_raises_runtime_error() -> None: + """Should raise RuntimeError when max_attempts leaves no attempt to run.""" + mock_func = MagicMock(return_value="ok") + mock_func.__name__ = "mock_func" + wrapped = retry_on_exception(max_attempts=0)(mock_func) + + with pytest.raises(RuntimeError, match="All 0 attempts failed"): + wrapped() + + mock_func.assert_not_called() + + +# --------------------------------------------------------------------------- +# async_retry_on_exception +# --------------------------------------------------------------------------- + + [email protected] +async def test_async_retry_on_exception_succeeds_first_try() -> None: + """Should await the wrapped coroutine once when it succeeds immediately.""" + mock_func = AsyncMock(return_value="ok") + wrapped = async_retry_on_exception(max_attempts=3)(mock_func) + + result = await wrapped() + + assert result == "ok" + assert mock_func.await_count == 1 + + [email protected] +async def test_async_retry_on_exception_retries_then_succeeds() -> None: + """Should retry a failing coroutine on retryable exceptions until success.""" + mock_func = AsyncMock(side_effect=[ConnectionError("fail"), "ok"]) + + with patch(ASYNC_SLEEP, new_callable=AsyncMock) as mock_sleep: + wrapped = async_retry_on_exception( + max_attempts=3, base_delay=0.01, jitter=False + )(mock_func) + result = await wrapped() + + assert result == "ok" + assert mock_func.await_count == 2 + mock_sleep.assert_awaited_once() + + [email protected] +async def test_async_retry_on_exception_exhausts_retries_and_raises() -> None: + """Should raise the last retryable exception once max_attempts is reached.""" + mock_func = AsyncMock(side_effect=ConnectionError("nope")) + + with patch(ASYNC_SLEEP, new_callable=AsyncMock): + wrapped = async_retry_on_exception( + max_attempts=2, base_delay=0.01, jitter=False + )(mock_func) + with pytest.raises(ConnectionError, match="nope"): + await wrapped() + + assert mock_func.await_count == 2 + + [email protected] +async def test_async_retry_on_exception_non_retryable_fails_immediately() -> None: + """Should not retry exceptions outside the configured retryable tuple.""" + mock_func = AsyncMock(side_effect=ValueError("bad")) + + with patch(ASYNC_SLEEP, new_callable=AsyncMock) as mock_sleep: + wrapped = async_retry_on_exception( + max_attempts=3, exceptions=(ConnectionError,) + )(mock_func) + with pytest.raises(ValueError, match="bad"): + await wrapped() + + assert mock_func.await_count == 1 + mock_sleep.assert_not_awaited() + + +# --------------------------------------------------------------------------- +# RetryableOperation +# --------------------------------------------------------------------------- + + +def test_retryable_operation_success_does_not_suppress_or_retry() -> None: + """Should leave attempt count untouched when the block succeeds.""" + op = RetryableOperation("test_op", max_attempts=3) + + with op: + pass + + assert op.current_attempt == 0 + assert op.last_exception is None + + +def test_retryable_operation_retries_until_success() -> None: + """Should suppress retryable exceptions and allow the loop to retry.""" + op = RetryableOperation("test_op", max_attempts=3, base_delay=0.01, jitter=False) + attempts = 0 + result = None Review Comment: **Suggestion:** Annotate this nullable local variable with its intended optional type to comply with the type-hint rule. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> The variable is initialized to None and later assigned a string, so it is a nullable local that can and should be annotated. This omission matches the type-hint rule. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2ac92853de4745a69d839b67c86ed4ac&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2ac92853de4745a69d839b67c86ed4ac&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/utils/test_retry_utils.py **Line:** 252:252 **Comment:** *Custom Rule: Annotate this nullable local variable with its intended optional type to comply with the type-hint rule. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=0bebc483b7f58fedf48b35cf2c26b3270f87cafa18a22256c58dcd50f0e40438&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=0bebc483b7f58fedf48b35cf2c26b3270f87cafa18a22256c58dcd50f0e40438&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/utils/test_retry_utils.py: ########## @@ -0,0 +1,396 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Unit tests for MCP service retry utilities. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from sqlalchemy.exc import OperationalError + +from superset.mcp_service.utils.retry_utils import ( + async_retry_database_operation, + async_retry_on_exception, + exponential_backoff, + retry_database_operation, + retry_on_exception, + retry_screenshot_operation, + RetryableOperation, +) + +SLEEP = "superset.mcp_service.utils.retry_utils.time.sleep" +ASYNC_SLEEP = "superset.mcp_service.utils.retry_utils.asyncio.sleep" + + +# --------------------------------------------------------------------------- +# exponential_backoff +# --------------------------------------------------------------------------- + + +def test_exponential_backoff_without_jitter_doubles_each_attempt() -> None: + """Should double the delay for each subsequent attempt when jitter is off.""" + assert exponential_backoff(0, base_delay=1.0, jitter=False) == 1.0 + assert exponential_backoff(1, base_delay=1.0, jitter=False) == 2.0 + assert exponential_backoff(2, base_delay=1.0, jitter=False) == 4.0 + + +def test_exponential_backoff_caps_at_max_delay() -> None: + """Should never return a delay larger than max_delay.""" + delay = exponential_backoff(10, base_delay=1.0, max_delay=5.0, jitter=False) + assert delay == 5.0 + + +def test_exponential_backoff_jitter_stays_within_twenty_five_percent() -> None: + """Should keep jittered delays within +/-25% of the base delay.""" + for _ in range(50): + delay = exponential_backoff(2, base_delay=1.0, jitter=True) + # Unjittered delay for attempt=2, base_delay=1.0 is 4.0. + assert 3.0 <= delay <= 5.0 + + +def test_exponential_backoff_never_negative() -> None: + """Should clamp the delay to be non-negative even with jitter applied.""" + for _ in range(50): + delay = exponential_backoff(0, base_delay=0.001, jitter=True) + assert delay >= 0 + + +# --------------------------------------------------------------------------- +# retry_on_exception (sync) +# --------------------------------------------------------------------------- + + +def test_retry_on_exception_succeeds_first_try() -> None: + """Should call the wrapped function once when it succeeds immediately.""" + mock_func = MagicMock(return_value="ok") + wrapped = retry_on_exception(max_attempts=3)(mock_func) + + result = wrapped() + + assert result == "ok" + assert mock_func.call_count == 1 + + +def test_retry_on_exception_retries_then_succeeds() -> None: + """Should retry on retryable exceptions and return the eventual success.""" + mock_func = MagicMock( + side_effect=[ConnectionError("fail"), ConnectionError("fail"), "ok"] + ) + mock_func.__name__ = "mock_func" + + with patch(SLEEP) as mock_sleep: + wrapped = retry_on_exception(max_attempts=3, base_delay=0.01, jitter=False)( + mock_func + ) + result = wrapped() + + assert result == "ok" + assert mock_func.call_count == 3 + assert mock_sleep.call_count == 2 + + +def test_retry_on_exception_exhausts_retries_and_raises_last_exception() -> None: + """Should raise the last retryable exception once max_attempts is reached.""" + mock_func = MagicMock(side_effect=ConnectionError("always fails")) + mock_func.__name__ = "mock_func" + + with patch(SLEEP) as mock_sleep: + wrapped = retry_on_exception(max_attempts=3, base_delay=0.01, jitter=False)( + mock_func + ) + with pytest.raises(ConnectionError, match="always fails"): + wrapped() + + assert mock_func.call_count == 3 + # No sleep after the final (failed) attempt. + assert mock_sleep.call_count == 2 + + +def test_retry_on_exception_non_retryable_exception_fails_immediately() -> None: + """Should not retry exceptions outside the configured retryable tuple.""" + mock_func = MagicMock(side_effect=ValueError("bad input")) + mock_func.__name__ = "mock_func" + + with patch(SLEEP) as mock_sleep: + wrapped = retry_on_exception(max_attempts=3, exceptions=(ConnectionError,))( + mock_func + ) + with pytest.raises(ValueError, match="bad input"): + wrapped() + + assert mock_func.call_count == 1 + mock_sleep.assert_not_called() + + +def test_retry_on_exception_respects_custom_exceptions_tuple() -> None: + """Should retry only on the exception types passed via `exceptions`.""" + mock_func = MagicMock(side_effect=[TimeoutError("timed out"), "ok"]) + mock_func.__name__ = "mock_func" + + with patch(SLEEP): + wrapped = retry_on_exception( + max_attempts=2, base_delay=0.01, jitter=False, exceptions=(TimeoutError,) + )(mock_func) + result = wrapped() + + assert result == "ok" + assert mock_func.call_count == 2 + + +def test_retry_on_exception_max_attempts_zero_raises_runtime_error() -> None: + """Should raise RuntimeError when max_attempts leaves no attempt to run.""" + mock_func = MagicMock(return_value="ok") + mock_func.__name__ = "mock_func" + wrapped = retry_on_exception(max_attempts=0)(mock_func) + + with pytest.raises(RuntimeError, match="All 0 attempts failed"): + wrapped() + + mock_func.assert_not_called() + + +# --------------------------------------------------------------------------- +# async_retry_on_exception +# --------------------------------------------------------------------------- + + [email protected] +async def test_async_retry_on_exception_succeeds_first_try() -> None: + """Should await the wrapped coroutine once when it succeeds immediately.""" + mock_func = AsyncMock(return_value="ok") + wrapped = async_retry_on_exception(max_attempts=3)(mock_func) + + result = await wrapped() + + assert result == "ok" + assert mock_func.await_count == 1 + + [email protected] +async def test_async_retry_on_exception_retries_then_succeeds() -> None: + """Should retry a failing coroutine on retryable exceptions until success.""" + mock_func = AsyncMock(side_effect=[ConnectionError("fail"), "ok"]) + + with patch(ASYNC_SLEEP, new_callable=AsyncMock) as mock_sleep: + wrapped = async_retry_on_exception( + max_attempts=3, base_delay=0.01, jitter=False + )(mock_func) + result = await wrapped() + + assert result == "ok" + assert mock_func.await_count == 2 + mock_sleep.assert_awaited_once() + + [email protected] +async def test_async_retry_on_exception_exhausts_retries_and_raises() -> None: + """Should raise the last retryable exception once max_attempts is reached.""" + mock_func = AsyncMock(side_effect=ConnectionError("nope")) + + with patch(ASYNC_SLEEP, new_callable=AsyncMock): + wrapped = async_retry_on_exception( + max_attempts=2, base_delay=0.01, jitter=False + )(mock_func) + with pytest.raises(ConnectionError, match="nope"): + await wrapped() + + assert mock_func.await_count == 2 + + [email protected] +async def test_async_retry_on_exception_non_retryable_fails_immediately() -> None: + """Should not retry exceptions outside the configured retryable tuple.""" + mock_func = AsyncMock(side_effect=ValueError("bad")) + + with patch(ASYNC_SLEEP, new_callable=AsyncMock) as mock_sleep: + wrapped = async_retry_on_exception( + max_attempts=3, exceptions=(ConnectionError,) + )(mock_func) + with pytest.raises(ValueError, match="bad"): + await wrapped() + + assert mock_func.await_count == 1 + mock_sleep.assert_not_awaited() + + +# --------------------------------------------------------------------------- +# RetryableOperation +# --------------------------------------------------------------------------- + + +def test_retryable_operation_success_does_not_suppress_or_retry() -> None: + """Should leave attempt count untouched when the block succeeds.""" + op = RetryableOperation("test_op", max_attempts=3) + + with op: + pass + + assert op.current_attempt == 0 + assert op.last_exception is None + + +def test_retryable_operation_retries_until_success() -> None: + """Should suppress retryable exceptions and allow the loop to retry.""" + op = RetryableOperation("test_op", max_attempts=3, base_delay=0.01, jitter=False) + attempts = 0 Review Comment: **Suggestion:** Declare an explicit type for this local counter variable since its type is stable and can be annotated. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> The local counter variable is clearly inferable but still explicitly type-annotatable. Since the rule flags new Python code omitting type hints on relevant variables, this is a real violation. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3ee8846a371d42c2a87c196f060dda0f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3ee8846a371d42c2a87c196f060dda0f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/utils/test_retry_utils.py **Line:** 251:251 **Comment:** *Custom Rule: Declare an explicit type for this local counter variable since its type is stable and can be annotated. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=ff5b17f4d1ee87a0409866696a866fce18d2904614542de1afb354fa201375d0&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=ff5b17f4d1ee87a0409866696a866fce18d2904614542de1afb354fa201375d0&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/utils/test_cache_utils.py: ########## @@ -0,0 +1,268 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Unit tests for MCP service cache utilities. +""" + +from datetime import datetime, timedelta, timezone +from typing import Any + +from superset.mcp_service.utils.cache_utils import ( + apply_cache_control_to_query_context, + get_cache_key_info, + get_cache_status_from_result, + should_use_metadata_cache, +) + +# --------------------------------------------------------------------------- +# get_cache_status_from_result +# --------------------------------------------------------------------------- + + +def test_get_cache_status_from_result_reads_from_queries_list() -> None: + """Should pull cache_hit from the first entry of a 'queries' list.""" + result: dict[str, Any] = {"queries": [{"is_cached": True}]} + Review Comment: **Suggestion:** Add an explicit type annotation for this local variable to comply with the type-hint requirement for relevant variables. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> The file is new Python code and this local variable is introduced without any type annotation, which matches the rule requiring type hints on relevant variables that can be annotated. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9ff99c2389f843b5a6322bacd50d77ad&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9ff99c2389f843b5a6322bacd50d77ad&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/utils/test_cache_utils.py **Line:** 40:40 **Comment:** *Custom Rule: Add an explicit type annotation for this local variable to comply with the type-hint requirement for relevant variables. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=d1a7348e2d5ddf5c62b8d72b28bf2fec7cc12e5a29faa06a50418e315568ef48&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=d1a7348e2d5ddf5c62b8d72b28bf2fec7cc12e5a29faa06a50418e315568ef48&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/utils/test_cache_utils.py: ########## @@ -0,0 +1,268 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Unit tests for MCP service cache utilities. +""" + +from datetime import datetime, timedelta, timezone +from typing import Any + +from superset.mcp_service.utils.cache_utils import ( + apply_cache_control_to_query_context, + get_cache_key_info, + get_cache_status_from_result, + should_use_metadata_cache, +) + +# --------------------------------------------------------------------------- +# get_cache_status_from_result +# --------------------------------------------------------------------------- + + +def test_get_cache_status_from_result_reads_from_queries_list() -> None: + """Should pull cache_hit from the first entry of a 'queries' list.""" + result: dict[str, Any] = {"queries": [{"is_cached": True}]} + + status = get_cache_status_from_result(result) + + assert status.cache_hit is True + assert status.cache_type == "query" + + +def test_get_cache_status_from_result_falls_back_to_top_level_dict() -> None: + """Should read cache info directly from the result when no 'queries' key.""" + result: dict[str, Any] = {"is_cached": False} + + status = get_cache_status_from_result(result) + + assert status.cache_hit is False + assert status.cache_type == "none" + + +def test_get_cache_status_from_result_empty_queries_list_uses_top_level() -> None: + """Should fall back to the top-level dict when 'queries' is an empty list.""" + result: dict[str, Any] = {"queries": [], "is_cached": True} + + status = get_cache_status_from_result(result) + + assert status.cache_hit is True + + +def test_get_cache_status_from_result_defaults_cache_hit_to_false() -> None: + """Should treat a missing 'is_cached' key as not cached.""" + result: dict[str, Any] = {} + + status = get_cache_status_from_result(result) + + assert status.cache_hit is False + assert status.cache_type == "none" + + +def test_get_cache_status_from_result_reflects_force_refresh_flag() -> None: + """Should surface the caller-provided force_refresh value on the status.""" + result: dict[str, Any] = {"is_cached": True} + + status = get_cache_status_from_result(result, force_refresh=True) + + assert status.refreshed is True + + +def test_get_cache_status_from_result_parses_iso_string_cache_age() -> None: + """Should compute cache_age_seconds from an ISO-formatted cache_dttm string.""" + cache_dt = datetime.now(timezone.utc) - timedelta(seconds=120) + result: dict[str, Any] = { + "is_cached": True, + "cache_dttm": cache_dt.isoformat(), + } + + status = get_cache_status_from_result(result) + + assert status.cache_age_seconds is not None + assert status.cache_age_seconds >= 119 + + +def test_get_cache_status_from_result_parses_z_suffixed_string() -> None: + """Should handle a trailing 'Z' UTC designator in the cache_dttm string.""" + result: dict[str, Any] = { + "is_cached": True, + "cache_dttm": "2020-01-01T00:00:00Z", + } + + status = get_cache_status_from_result(result) + + assert status.cache_age_seconds is not None + assert status.cache_age_seconds > 0 + + +def test_get_cache_status_from_result_parses_datetime_object_cache_age() -> None: + """Should compute cache_age_seconds when cache_dttm is already a datetime.""" + cache_dt = datetime.now(timezone.utc) - timedelta(seconds=60) + result: dict[str, Any] = {"is_cached": True, "cache_dttm": cache_dt} + + status = get_cache_status_from_result(result) + + assert status.cache_age_seconds is not None + assert status.cache_age_seconds >= 59 + + +def test_get_cache_status_from_result_handles_unparseable_cache_dttm() -> None: + """Should swallow parse errors and leave cache_age_seconds as None.""" + result: dict[str, Any] = {"is_cached": True, "cache_dttm": "not-a-date"} + + status = get_cache_status_from_result(result) + + assert status.cache_age_seconds is None + # The rest of the status should still be populated correctly. + assert status.cache_hit is True + + +def test_get_cache_status_from_result_no_cache_dttm_leaves_age_none() -> None: + """Should leave cache_age_seconds as None when cache_dttm is absent.""" + result: dict[str, Any] = {"is_cached": True} + + status = get_cache_status_from_result(result) + + assert status.cache_age_seconds is None + + +# --------------------------------------------------------------------------- +# apply_cache_control_to_query_context +# --------------------------------------------------------------------------- + + +def test_apply_cache_control_sets_force_when_cache_disabled() -> None: + """Should set force=True on the query context when use_cache is False.""" + ctx = apply_cache_control_to_query_context({"queries": []}, use_cache=False) + + assert ctx["force"] is True + + +def test_apply_cache_control_sets_force_when_force_refresh_requested() -> None: + """Should set force=True on the query context when force_refresh is True.""" + ctx = apply_cache_control_to_query_context( + {"queries": []}, use_cache=True, force_refresh=True + ) + + assert ctx["force"] is True + + +def test_apply_cache_control_does_not_set_force_when_using_cache() -> None: + """Should leave 'force' unset when caching is enabled and not refreshing.""" + ctx = apply_cache_control_to_query_context( + {"queries": []}, use_cache=True, force_refresh=False + ) + + assert "force" not in ctx + + +def test_apply_cache_control_applies_cache_timeout_to_every_query() -> None: + """Should stamp cache_timeout onto every query in the context.""" + ctx = apply_cache_control_to_query_context( + {"queries": [{"metric": "a"}, {"metric": "b"}]}, cache_timeout=60 + ) + + assert ctx["queries"][0]["cache_timeout"] == 60 + assert ctx["queries"][1]["cache_timeout"] == 60 + + +def test_apply_cache_control_leaves_queries_untouched_when_timeout_is_none() -> None: + """Should not add a cache_timeout key when none is provided.""" + ctx = apply_cache_control_to_query_context( + {"queries": [{"metric": "a"}]}, cache_timeout=None + ) + + assert "cache_timeout" not in ctx["queries"][0] + + +def test_apply_cache_control_missing_queries_key_is_a_noop_for_timeout() -> None: + """Should not raise or add a 'queries' key when applying a cache_timeout.""" + ctx = apply_cache_control_to_query_context({}, cache_timeout=60) + + assert "queries" not in ctx + + +def test_apply_cache_control_returns_the_same_dict_instance() -> None: + """Should mutate and return the same query_context object it was given.""" + original: dict[str, Any] = {"queries": []} + + result = apply_cache_control_to_query_context(original, use_cache=False) + + assert result is original + + +# --------------------------------------------------------------------------- +# should_use_metadata_cache +# --------------------------------------------------------------------------- + + +def test_should_use_metadata_cache_true_by_default() -> None: + """Should default to using the metadata cache.""" + assert should_use_metadata_cache() is True + + +def test_should_use_metadata_cache_false_when_cache_disabled() -> None: + """Should return False when use_cache is False.""" + assert should_use_metadata_cache(use_cache=False) is False + + +def test_should_use_metadata_cache_false_when_refresh_requested() -> None: + """Should return False when refresh_metadata is True, even with use_cache.""" + assert should_use_metadata_cache(use_cache=True, refresh_metadata=True) is False + + +def test_should_use_metadata_cache_true_when_enabled_and_not_refreshing() -> None: + """Should return True when caching is enabled and no refresh is requested.""" + assert should_use_metadata_cache(use_cache=True, refresh_metadata=False) is True + + +# --------------------------------------------------------------------------- +# get_cache_key_info +# --------------------------------------------------------------------------- + + +def test_get_cache_key_info_returns_none_for_none_input() -> None: + """Should return None when no cache key is provided.""" + assert get_cache_key_info(None) is None + + +def test_get_cache_key_info_returns_none_for_empty_string() -> None: + """Should treat an empty string cache key the same as no key.""" + assert get_cache_key_info("") is None + + +def test_get_cache_key_info_returns_short_keys_unchanged() -> None: + """Should return short cache keys unmodified.""" + key = "short_cache_key" Review Comment: **Suggestion:** Add an explicit string type annotation for this local variable to align with the project's type-hint policy. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This is a new Python local variable with an obvious string type that can be annotated, and it is currently untyped, matching the custom rule. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e745eaf7288844a99af12ed840381384&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e745eaf7288844a99af12ed840381384&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/utils/test_cache_utils.py **Line:** 250:250 **Comment:** *Custom Rule: Add an explicit string type annotation for this local variable to align with the project's type-hint policy. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=754f62f2c527aafbe5019b9fb691b5b1d4b5a70f3831462b28ebec62ee4c369c&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=754f62f2c527aafbe5019b9fb691b5b1d4b5a70f3831462b28ebec62ee4c369c&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/utils/test_cache_utils.py: ########## @@ -0,0 +1,268 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Unit tests for MCP service cache utilities. +""" + +from datetime import datetime, timedelta, timezone +from typing import Any + +from superset.mcp_service.utils.cache_utils import ( + apply_cache_control_to_query_context, + get_cache_key_info, + get_cache_status_from_result, + should_use_metadata_cache, +) + +# --------------------------------------------------------------------------- +# get_cache_status_from_result +# --------------------------------------------------------------------------- + + +def test_get_cache_status_from_result_reads_from_queries_list() -> None: + """Should pull cache_hit from the first entry of a 'queries' list.""" + result: dict[str, Any] = {"queries": [{"is_cached": True}]} + + status = get_cache_status_from_result(result) + + assert status.cache_hit is True + assert status.cache_type == "query" + + +def test_get_cache_status_from_result_falls_back_to_top_level_dict() -> None: + """Should read cache info directly from the result when no 'queries' key.""" + result: dict[str, Any] = {"is_cached": False} + + status = get_cache_status_from_result(result) + + assert status.cache_hit is False + assert status.cache_type == "none" + + +def test_get_cache_status_from_result_empty_queries_list_uses_top_level() -> None: + """Should fall back to the top-level dict when 'queries' is an empty list.""" + result: dict[str, Any] = {"queries": [], "is_cached": True} + + status = get_cache_status_from_result(result) + + assert status.cache_hit is True + + +def test_get_cache_status_from_result_defaults_cache_hit_to_false() -> None: + """Should treat a missing 'is_cached' key as not cached.""" + result: dict[str, Any] = {} + + status = get_cache_status_from_result(result) + + assert status.cache_hit is False + assert status.cache_type == "none" + + +def test_get_cache_status_from_result_reflects_force_refresh_flag() -> None: + """Should surface the caller-provided force_refresh value on the status.""" + result: dict[str, Any] = {"is_cached": True} + + status = get_cache_status_from_result(result, force_refresh=True) + + assert status.refreshed is True + + +def test_get_cache_status_from_result_parses_iso_string_cache_age() -> None: + """Should compute cache_age_seconds from an ISO-formatted cache_dttm string.""" + cache_dt = datetime.now(timezone.utc) - timedelta(seconds=120) Review Comment: **Suggestion:** Annotate this datetime local variable explicitly to satisfy the rule requiring type hints on relevant variables. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> The variable is newly added Python code, its type is clear and can be annotated, but it currently lacks a type hint, so the rule applies. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a0842772f586405b9f368b5dda5c7224&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a0842772f586405b9f368b5dda5c7224&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/utils/test_cache_utils.py **Line:** 87:87 **Comment:** *Custom Rule: Annotate this datetime local variable explicitly to satisfy the rule requiring type hints on relevant variables. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=604f38dc9d6551478e5aba9913d8133a329a36db61aa754ffbbee8d6b02abd6d&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=604f38dc9d6551478e5aba9913d8133a329a36db61aa754ffbbee8d6b02abd6d&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/utils/test_cache_utils.py: ########## @@ -0,0 +1,268 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Unit tests for MCP service cache utilities. +""" + +from datetime import datetime, timedelta, timezone +from typing import Any + +from superset.mcp_service.utils.cache_utils import ( + apply_cache_control_to_query_context, + get_cache_key_info, + get_cache_status_from_result, + should_use_metadata_cache, +) + +# --------------------------------------------------------------------------- +# get_cache_status_from_result +# --------------------------------------------------------------------------- + + +def test_get_cache_status_from_result_reads_from_queries_list() -> None: + """Should pull cache_hit from the first entry of a 'queries' list.""" + result: dict[str, Any] = {"queries": [{"is_cached": True}]} + + status = get_cache_status_from_result(result) + + assert status.cache_hit is True + assert status.cache_type == "query" + + +def test_get_cache_status_from_result_falls_back_to_top_level_dict() -> None: + """Should read cache info directly from the result when no 'queries' key.""" + result: dict[str, Any] = {"is_cached": False} + + status = get_cache_status_from_result(result) + + assert status.cache_hit is False + assert status.cache_type == "none" + + +def test_get_cache_status_from_result_empty_queries_list_uses_top_level() -> None: + """Should fall back to the top-level dict when 'queries' is an empty list.""" + result: dict[str, Any] = {"queries": [], "is_cached": True} + + status = get_cache_status_from_result(result) + + assert status.cache_hit is True + + +def test_get_cache_status_from_result_defaults_cache_hit_to_false() -> None: + """Should treat a missing 'is_cached' key as not cached.""" + result: dict[str, Any] = {} + + status = get_cache_status_from_result(result) + + assert status.cache_hit is False + assert status.cache_type == "none" + + +def test_get_cache_status_from_result_reflects_force_refresh_flag() -> None: + """Should surface the caller-provided force_refresh value on the status.""" + result: dict[str, Any] = {"is_cached": True} + + status = get_cache_status_from_result(result, force_refresh=True) + + assert status.refreshed is True + + +def test_get_cache_status_from_result_parses_iso_string_cache_age() -> None: + """Should compute cache_age_seconds from an ISO-formatted cache_dttm string.""" + cache_dt = datetime.now(timezone.utc) - timedelta(seconds=120) + result: dict[str, Any] = { + "is_cached": True, + "cache_dttm": cache_dt.isoformat(), + } + + status = get_cache_status_from_result(result) + + assert status.cache_age_seconds is not None + assert status.cache_age_seconds >= 119 + + +def test_get_cache_status_from_result_parses_z_suffixed_string() -> None: + """Should handle a trailing 'Z' UTC designator in the cache_dttm string.""" + result: dict[str, Any] = { + "is_cached": True, + "cache_dttm": "2020-01-01T00:00:00Z", + } + + status = get_cache_status_from_result(result) + + assert status.cache_age_seconds is not None + assert status.cache_age_seconds > 0 + + +def test_get_cache_status_from_result_parses_datetime_object_cache_age() -> None: + """Should compute cache_age_seconds when cache_dttm is already a datetime.""" + cache_dt = datetime.now(timezone.utc) - timedelta(seconds=60) + result: dict[str, Any] = {"is_cached": True, "cache_dttm": cache_dt} + + status = get_cache_status_from_result(result) + + assert status.cache_age_seconds is not None + assert status.cache_age_seconds >= 59 + + +def test_get_cache_status_from_result_handles_unparseable_cache_dttm() -> None: + """Should swallow parse errors and leave cache_age_seconds as None.""" + result: dict[str, Any] = {"is_cached": True, "cache_dttm": "not-a-date"} + + status = get_cache_status_from_result(result) + + assert status.cache_age_seconds is None + # The rest of the status should still be populated correctly. + assert status.cache_hit is True + + +def test_get_cache_status_from_result_no_cache_dttm_leaves_age_none() -> None: + """Should leave cache_age_seconds as None when cache_dttm is absent.""" + result: dict[str, Any] = {"is_cached": True} + + status = get_cache_status_from_result(result) + + assert status.cache_age_seconds is None + + +# --------------------------------------------------------------------------- +# apply_cache_control_to_query_context +# --------------------------------------------------------------------------- + + +def test_apply_cache_control_sets_force_when_cache_disabled() -> None: + """Should set force=True on the query context when use_cache is False.""" + ctx = apply_cache_control_to_query_context({"queries": []}, use_cache=False) Review Comment: **Suggestion:** Add a concrete type annotation to this local variable so the test code consistently follows the type-hint rule. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This is a new Python local variable with an inferable type that is left unannotated, so it violates the type-hint rule for relevant variables. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e21e5cb5eb924df0b1b875f764dea1ce&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e21e5cb5eb924df0b1b875f764dea1ce&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/utils/test_cache_utils.py **Line:** 150:150 **Comment:** *Custom Rule: Add a concrete type annotation to this local variable so the test code consistently follows the type-hint rule. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=d8ae391e5e7a7fa69a9078adda335eda525bee3da90231e2ef962b10f27aa7e0&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=d8ae391e5e7a7fa69a9078adda335eda525bee3da90231e2ef962b10f27aa7e0&reaction=dislike'>๐</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
