codeant-ai-for-open-source[bot] commented on code in PR #34565: URL: https://github.com/apache/superset/pull/34565#discussion_r2771814358
########## tests/unit_tests/views/test_base.py: ########## @@ -0,0 +1,93 @@ +# 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. +"""Tests for superset.views.base module""" + +from unittest.mock import MagicMock, patch + +import pytest + + +@patch("superset.views.base.utils.get_user_id", return_value=1) +@patch( + "superset.views.base.cached_common_bootstrap_data", return_value={"test": "data"} +) +@patch("superset.views.base.get_locale") +def test_common_bootstrap_payload_converts_locale_to_string( + mock_get_locale: MagicMock, + mock_cached: MagicMock, + mock_user_id: MagicMock, +) -> None: + """Test that common_bootstrap_payload converts locale to string for cache key""" + # Mock get_locale to return a Locale-like object with __str__ + mock_locale = MagicMock() + mock_locale.__str__ = lambda self: "de_DE" + mock_get_locale.return_value = mock_locale Review Comment: **Suggestion:** The test attempts to simulate a locale object by assigning a lambda to `mock_locale.__str__`, but Python's `str()` uses the type-level `__str__` special method, so this instance attribute is ignored and `str(mock_locale)` will not return `"de_DE"`, causing the assertion that the cache is called with `"de_DE"` to fail and not accurately testing the intended behavior. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Unit test failure blocking CI test runs. - ⚠️ Test does not validate locale-to-string conversion. - ⚠️ Misleading test coverage for i18n behavior. ``` </details> ```suggestion mock_get_locale.return_value = "de_DE" ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open tests file tests/unit_tests/views/test_base.py and locate the test function test_common_bootstrap_payload_converts_locale_to_string (file content shown in PR final state). The test sets up a mocked locale at lines 36-38 (`mock_locale = MagicMock()`, `mock_locale.__str__ = lambda self: "de_DE"`, `mock_get_locale.return_value = mock_locale`). 2. Run the unit test suite (pytest). The test imports common_bootstrap_payload which calls get_locale() (patched by mock_get_locale). Because mock_get_locale returns a MagicMock instance whose instance attribute __str__ was assigned (line 37), Python's built-in str(mock_locale) does not use that instance attribute. The type's __str__ is looked up on the class, so str(mock_locale) returns MagicMock's normal string representation, not "de_DE". 3. The test expects cached_common_bootstrap_data to be called with "de_DE" (assert at line ~46 in the file). Because the mocked locale does not stringify to "de_DE", common_bootstrap_payload will pass a different value to cached_common_bootstrap_data and mock_cached.assert_called_once_with(1, "de_DE") will fail, causing the unit test to fail. 4. Replace the mocked-locale-with-instance approach with returning the literal string "de_DE" from get_locale (as in improved_code). This directly matches how the production change expects a string cache key and makes the assertion at line 46 valid. Explanation why suggestion is needed: The current test attempts to override an instance __str__ attribute (line 37) which Python ignores for str() lookup; the test therefore does not simulate the intended behavior. Changing the patch to return the string is the simplest accurate simulation of the behavior exercised by the updated cached_common_bootstrap_data function. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/views/test_base.py **Line:** 36:38 **Comment:** *Logic Error: The test attempts to simulate a locale object by assigning a lambda to `mock_locale.__str__`, but Python's `str()` uses the type-level `__str__` special method, so this instance attribute is ignored and `str(mock_locale)` will not return `"de_DE"`, causing the assertion that the cache is called with `"de_DE"` to fail and not accurately testing the intended behavior. 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. ``` </details> -- 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]
