codeant-ai-for-open-source[bot] commented on code in PR #41097:
URL: https://github.com/apache/superset/pull/41097#discussion_r3417398288
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -898,3 +898,229 @@ def evaluate_side_effect(script):
"dashboard",
"http://example.com";,
)
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
Review Comment:
**Suggestion:** Add explicit type annotations for the helper method
parameters and return value so the new method is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is newly added Python code and the helper method has no type hints for
its parameter or return value. The custom rule requires new Python
functions/methods to be fully typed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=43cdc4731198456a953012303a5ed8fd&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=43cdc4731198456a953012303a5ed8fd&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/utils/webdriver_test.py
**Line:** 920:920
**Comment:**
*Custom Rule: Add explicit type annotations for the helper method
parameters and return value so the new method is fully typed.
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%2F41097&comment_hash=a41e2cae4b75ff84657a96be5ef4a04eb01388cfdc5bc72a40beaadb0f806dfa&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=a41e2cae4b75ff84657a96be5ef4a04eb01388cfdc5bc72a40beaadb0f806dfa&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -898,3 +898,229 @@ def evaluate_side_effect(script):
"dashboard",
"http://example.com";,
)
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
+ mock_playwright_instance = MagicMock()
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_sync_playwright.return_value.__enter__.return_value = (
+ mock_playwright_instance
+ )
+ mock_playwright_instance.chromium.launch.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_sync_playwright
+ ):
Review Comment:
**Suggestion:** Add type hints for all parameters and the return type of
this new test method to comply with full typing requirements. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python test method, and its parameters and return
value are not type-annotated. The rule says new Python code should be fully
typed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9f4468a46350457fb68a10a7a5aada07&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=9f4468a46350457fb68a10a7a5aada07&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/utils/webdriver_test.py
**Line:** 940:942
**Comment:**
*Custom Rule: Add type hints for all parameters and the return type of
this new test method to comply with full typing requirements.
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%2F41097&comment_hash=0b054e6148ec631c804ae3d39eea2092e8a3dcecf47ae028924762d7cea3aa85&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=0b054e6148ec631c804ae3d39eea2092e8a3dcecf47ae028924762d7cea3aa85&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/test_screenshot_cache_fix.py:
##########
@@ -149,6 +149,25 @@ def test_cache_saved_only_when_image_generated(
assert cached_value["status"] == "Updated"
assert cached_value["image"] is not None
+ def test_cache_error_status_when_screenshot_returns_empty_bytes(
+ self, mocker: MockerFixture, screenshot_obj, mock_user
+ ):
Review Comment:
**Suggestion:** Add full type annotations to this newly added test method by
typing all parameters and the return value (`-> None`) so new code is fully
typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added test method in the changed hunk, and it lacks type
hints for
`screenshot_obj`, `mock_user`, and the return value. The custom rule
requires new
Python code to be fully typed, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=87297c65b7674c5f9d1be1df283f832d&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=87297c65b7674c5f9d1be1df283f832d&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/utils/test_screenshot_cache_fix.py
**Line:** 152:154
**Comment:**
*Custom Rule: Add full type annotations to this newly added test method
by typing all parameters and the return value (`-> None`) so new code is fully
typed.
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%2F41097&comment_hash=66cdb6ef04a206e095b17e1916108b6640baddce4a530f8800b78a7fa23bc820&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=66cdb6ef04a206e095b17e1916108b6640baddce4a530f8800b78a7fa23bc820&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -898,3 +898,229 @@ def evaluate_side_effect(script):
"dashboard",
"http://example.com";,
)
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
+ mock_playwright_instance = MagicMock()
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_sync_playwright.return_value.__enter__.return_value = (
+ mock_playwright_instance
+ )
+ mock_playwright_instance.chromium.launch.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_sync_playwright
+ ):
+ """Non-tiled path: animation wait runs after spinner
wait_for_function."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED":
False}
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
Review Comment:
**Suggestion:** Add type annotations for `*args`, `**kwargs`, and the return
type in this newly introduced local function. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added local function omits type annotations for `*args`,
`**kwargs`, and the return type. The custom rule requires new Python functions
to be fully typed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=db74f7549d644c38989de2b7d40dff19&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=db74f7549d644c38989de2b7d40dff19&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/utils/webdriver_test.py
**Line:** 952:952
**Comment:**
*Custom Rule: Add type annotations for `*args`, `**kwargs`, and the
return type in this newly introduced local function.
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%2F41097&comment_hash=493ef1061110c5653f62f52341b022f6adea09d54a92d93f492f769d4879d6f5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=493ef1061110c5653f62f52341b022f6adea09d54a92d93f492f769d4879d6f5&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -898,3 +898,229 @@ def evaluate_side_effect(script):
"dashboard",
"http://example.com";,
)
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
+ mock_playwright_instance = MagicMock()
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_sync_playwright.return_value.__enter__.return_value = (
+ mock_playwright_instance
+ )
+ mock_playwright_instance.chromium.launch.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_sync_playwright
+ ):
+ """Non-tiled path: animation wait runs after spinner
wait_for_function."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED":
False}
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
+ call_order.append("spinner_wait")
+
+ def record_wait_for_timeout(ms):
+ if ms == 2 * 1000:
+ call_order.append("animation_wait")
Review Comment:
**Suggestion:** Add a docstring to this newly added local function so its
role in call-order tracking is documented. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added local function has no docstring. The custom rule requires
newly added Python functions and classes to include docstrings.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9c37ebf47a574c4eb2c7781d5befea4b&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=9c37ebf47a574c4eb2c7781d5befea4b&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/utils/webdriver_test.py
**Line:** 955:957
**Comment:**
*Custom Rule: Add a docstring to this newly added local function so its
role in call-order tracking is documented.
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%2F41097&comment_hash=e9f1c05f639fa377c021a4755042661eb243ee0cb2e4fd9290aa8bc20c16bfee&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=e9f1c05f639fa377c021a4755042661eb243ee0cb2e4fd9290aa8bc20c16bfee&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]
