codeant-ai-for-open-source[bot] commented on code in PR #41549: URL: https://github.com/apache/superset/pull/41549#discussion_r3573700134
########## tests/unit_tests/tasks/test_deletion_retention.py: ########## @@ -0,0 +1,131 @@ +# 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 deletion-retention configuration and window resolution. + +The shared value overrides config, an unset value falls back to config, ``0`` +is preserved as the disable value, and malformed shared values use the fallback. +""" + +from datetime import datetime +from typing import Any +from unittest.mock import patch + +import pytest +from flask.config import Config + + [email protected] +def app_config(app_context: None) -> Config: + from flask import current_app + + current_app.config["SUPERSET_SOFT_DELETE_RETENTION_DAYS"] = 30 + return current_app.config + + +def _resolve() -> int: + from superset.commands.deletion_retention.window import resolve_retention_window + + return resolve_retention_window() + + +def test_unset_falls_back_to_config(app_config: Config) -> None: + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=None, + ): + assert _resolve() == 30 + + +def test_shared_value_overrides_config(app_config: Config) -> None: + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=7, + ): + assert _resolve() == 7 + + +def test_zero_shared_value_is_preserved_not_coerced(app_config: Config) -> None: + # `0` is a meaningful "disable"; it must survive (never `or`-coerced to 30). + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=0, + ): + assert _resolve() == 0 + + +def test_malformed_shared_value_falls_back(app_config: Config) -> None: + for bad in ("oops", -3, True, 1.5): + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=bad, + ): + assert _resolve() == 30 + + +def test_window_zero_disables_the_task(app_context: None) -> None: + # A zero window short-circuits the purge entirely. + import superset.tasks.deletion_retention as mod + + with patch.object(mod, "_soft_delete_models") as models: + result = mod._purge_impl(0, dry_run=False) + assert result == {"skipped": 1} + models.assert_not_called() + + +def test_clock_uses_now_not_utcnow() -> None: + # The cutoff uses the same clock as the soft-delete substrate's deleted_at. + import inspect + + import superset.tasks.deletion_retention as mod + + src = inspect.getsource(mod._purge_impl) + assert "datetime.now()" in src + assert "datetime.utcnow()" not in src Review Comment: **Suggestion:** This assertion strategy is brittle because it inspects source-code text rather than behavior, so harmless refactors (aliases, wrappers, formatting) can fail the test while runtime behavior remains correct. Replace this with a behavioral assertion (for example by controlling time in execution and validating cutoff behavior) so the test checks semantics, not string literals. [code quality] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Harmless refactors break tests inspecting string literals. ⚠️ Test fails while datetime cutoff behavior remains correct. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `tests/unit_tests/tasks/test_deletion_retention.py` and locate `test_clock_uses_now_not_utcnow()` at lines 89-97, which is intended to verify that the purge cutoff uses the same clock as the soft-delete substrate’s `deleted_at`. 2. See that the test imports `superset.tasks.deletion_retention` as `mod`, then uses `inspect.getsource(mod._purge_impl)` at line 95 to retrieve the function source and asserts the literal strings `"datetime.now()"` is present and `"datetime.utcnow()"` is absent in `src` (lines 96-97). 3. Inspect `_purge_impl` in `superset/tasks/deletion_retention.py:110-118` and confirm that the cutoff is computed behaviorally as `cutoff = datetime.now() - timedelta(days=window_days)`, matching the intended clock semantics. 4. Recognize that if `_purge_impl` were refactored to use an equivalent expression without the exact substrings (for example, `from datetime import datetime as dt` in this module and `cutoff = dt.now() - timedelta(days=window_days)`), the runtime behavior would remain correct, but this test would fail solely because the source text no longer contains `"datetime.now()"`, demonstrating that the test is brittle and asserts string literals rather than actual cutoff behavior. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a48b5251ecef434290cfdb054774d26e&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=a48b5251ecef434290cfdb054774d26e&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/tasks/test_deletion_retention.py **Line:** 95:97 **Comment:** *Code Quality: This assertion strategy is brittle because it inspects source-code text rather than behavior, so harmless refactors (aliases, wrappers, formatting) can fail the test while runtime behavior remains correct. Replace this with a behavioral assertion (for example by controlling time in execution and validating cutoff behavior) so the test checks semantics, not string literals. 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%2F41549&comment_hash=3b6effec8cf218686649983c114ed6e2437c5b459d6f845a812ccb8f3594e459&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=3b6effec8cf218686649983c114ed6e2437c5b459d6f845a812ccb8f3594e459&reaction=dislike'>👎</a> ########## tests/integration_tests/deletion_retention/window_tests.py: ########## @@ -0,0 +1,82 @@ +# 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. +"""Integration coverage for the per-workspace retention window.""" + +from __future__ import annotations + +from superset.commands.deletion_retention.window import resolve_retention_window +from superset.key_value.shared_entries import get_shared_value, upsert_shared_value +from superset.key_value.types import SharedKey +from superset.models.slice import Slice +from superset.tasks.deletion_retention import _purge_impl + +from ._base import DeletionRetentionTestBase + + +class TestRetentionWindow(DeletionRetentionTestBase): + def tearDown(self) -> None: + # clear any shared window value this test set so the env default is + # restored for other tests + from uuid import uuid3 + + from superset import db + from superset.daos.key_value import KeyValueDAO + from superset.key_value.shared_entries import RESOURCE + from superset.key_value.utils import get_uuid_namespace + + try: + KeyValueDAO.delete_entry( + RESOURCE, + uuid3(get_uuid_namespace(""), SharedKey.SOFT_DELETE_RETENTION_DAYS), + ) + db.session.commit() + except Exception: # pylint: disable=broad-except + db.session.rollback() + super().tearDown() + + def test_shared_value_overrides_env_and_is_used_by_task(self) -> None: + """A per-workspace shared value takes + precedence over the env default and is honored by the purge.""" + upsert_shared_value(SharedKey.SOFT_DELETE_RETENTION_DAYS, 10) + assert resolve_retention_window() == 10 + + # 20 days old: still inside the env default (30) but past the 10-day + # per-workspace override, so the override is what gets it purged + chart = self.make_chart("c") + chart_id = chart.id + self.soft_delete(chart, days_ago=20) + + _purge_impl(resolve_retention_window(), dry_run=False) Review Comment: **Suggestion:** This test bypasses the Celery task entrypoint by calling the internal implementation with a pre-resolved window, so it does not actually verify that the task resolves and honors the shared retention value at runtime. If `purge_soft_deleted()` regresses to ignore shared values, this test will still pass. Exercise the task entrypoint itself (with the flag/config arranged for execution) to validate the real integration path. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Celery purge task regressions may bypass retention window tests. ⚠️ Shared window misconfiguration unnoticed by current integration tests. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run `pytest` targeting `TestRetentionWindow.test_shared_value_overrides_env_and_is_used_by_task` in `tests/integration_tests/deletion_retention/window_tests.py:51-64` to see how the integration test exercises the retention window behavior. 2. Observe that the test sets `SharedKey.SOFT_DELETE_RETENTION_DAYS` via `upsert_shared_value` (lines 54-55), asserts `resolve_retention_window() == 10`, soft-deletes a `Slice` chart, and then calls `_purge_impl(resolve_retention_window(), dry_run=False)` directly at line 63 instead of invoking the Celery task entrypoint. 3. Inspect `superset/tasks/deletion_retention.py:229-243` where `purge_soft_deleted()` is defined; note that this beat task resolves the window itself via `resolve_retention_window()`, reads `SUPERSET_SOFT_DELETE_PURGE_DRY_RUN` from `current_app.config`, checks the `SOFT_DELETE` feature flag, and then delegates to `_purge_impl(window_days, dry_run)`. 4. Because the integration test bypasses `purge_soft_deleted()` and only asserts behavior of `_purge_impl` with an already-resolved window, any future regression inside `purge_soft_deleted()` (for example, hardcoding the config value or skipping `resolve_retention_window()` and thus ignoring the shared per-workspace retention setting) would not be detected by this test even though `_purge_impl` still behaves correctly for the passed-in window. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4255d5321bbb4352b63fcef4e46b808f&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=4255d5321bbb4352b63fcef4e46b808f&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/integration_tests/deletion_retention/window_tests.py **Line:** 63:63 **Comment:** *Incomplete Implementation: This test bypasses the Celery task entrypoint by calling the internal implementation with a pre-resolved window, so it does not actually verify that the task resolves and honors the shared retention value at runtime. If `purge_soft_deleted()` regresses to ignore shared values, this test will still pass. Exercise the task entrypoint itself (with the flag/config arranged for execution) to validate the real integration path. 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%2F41549&comment_hash=2c9e1391d4a4d7a4c9fa45eda992a1631a0ae6bbf2e1d58ad6af86bd9f8932a6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=2c9e1391d4a4d7a4c9fa45eda992a1631a0ae6bbf2e1d58ad6af86bd9f8932a6&reaction=dislike'>👎</a> ########## tests/unit_tests/tasks/test_deletion_retention.py: ########## @@ -0,0 +1,131 @@ +# 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 deletion-retention configuration and window resolution. + +The shared value overrides config, an unset value falls back to config, ``0`` +is preserved as the disable value, and malformed shared values use the fallback. +""" + +from datetime import datetime +from typing import Any +from unittest.mock import patch + +import pytest +from flask.config import Config + + [email protected] +def app_config(app_context: None) -> Config: + from flask import current_app + + current_app.config["SUPERSET_SOFT_DELETE_RETENTION_DAYS"] = 30 + return current_app.config + + +def _resolve() -> int: + from superset.commands.deletion_retention.window import resolve_retention_window + + return resolve_retention_window() + + +def test_unset_falls_back_to_config(app_config: Config) -> None: + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=None, + ): + assert _resolve() == 30 + + +def test_shared_value_overrides_config(app_config: Config) -> None: + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=7, + ): + assert _resolve() == 7 + + +def test_zero_shared_value_is_preserved_not_coerced(app_config: Config) -> None: + # `0` is a meaningful "disable"; it must survive (never `or`-coerced to 30). + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=0, + ): + assert _resolve() == 0 + + +def test_malformed_shared_value_falls_back(app_config: Config) -> None: + for bad in ("oops", -3, True, 1.5): + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=bad, + ): + assert _resolve() == 30 + + +def test_window_zero_disables_the_task(app_context: None) -> None: + # A zero window short-circuits the purge entirely. + import superset.tasks.deletion_retention as mod + + with patch.object(mod, "_soft_delete_models") as models: + result = mod._purge_impl(0, dry_run=False) + assert result == {"skipped": 1} + models.assert_not_called() + + +def test_clock_uses_now_not_utcnow() -> None: + # The cutoff uses the same clock as the soft-delete substrate's deleted_at. + import inspect + + import superset.tasks.deletion_retention as mod + + src = inspect.getsource(mod._purge_impl) + assert "datetime.now()" in src + assert "datetime.utcnow()" not in src + + +def test_default_config_is_safe() -> None: + from superset import config + + assert config.SUPERSET_SOFT_DELETE_RETENTION_DAYS == 30 + assert config.SUPERSET_SOFT_DELETE_PURGE_DRY_RUN is True + + +def test_default_celery_config_registers_daily_purge() -> None: + from superset import config + + assert "superset.tasks.deletion_retention" in config.CeleryConfig.imports + entry: dict[str, Any] = config.CeleryConfig.beat_schedule[ + "deletion_retention.purge_soft_deleted" + ] + assert entry["task"] == "deletion_retention.purge_soft_deleted" Review Comment: **Suggestion:** The test name says it verifies a daily schedule, but the body only checks task registration and never validates cadence fields (cron/hour/minute), so it can pass even if the job is scheduled hourly or weekly. Add assertions on the schedule expression to match the “daily purge” contract. [inconsistent naming] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Celery schedule frequency regressions pass without detection. ⚠️ Daily purge contract unvalidated despite descriptive test name. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `tests/unit_tests/tasks/test_deletion_retention.py` and inspect `test_default_celery_config_registers_daily_purge()` at lines 107-114, noting the test name explicitly claims to verify a “daily purge”. 2. See that the test asserts `"superset.tasks.deletion_retention"` is present in `config.CeleryConfig.imports` and then retrieves the beat schedule entry `config.CeleryConfig.beat_schedule["deletion_retention.purge_soft_deleted"]` into `entry` (lines 110-113). 3. Observe that the only assertion on the entry is `assert entry["task"] == "deletion_retention.purge_soft_deleted"` at line 114; there is no check on `entry["schedule"]` or its `crontab` fields, despite the “daily purge” intent. 4. Inspect the actual schedule configuration in `superset/config.py:1646-1659`, where `CeleryConfig.beat_schedule["deletion_retention.purge_soft_deleted"]["schedule"]` is currently `crontab(minute=0, hour=0)` (once daily at midnight), and reason that a change to a non-daily cadence (for example, `crontab(minute="*", hour="*")` for hourly) would still satisfy the existing test’s assertions and pass, allowing schedule-frequency regressions to slip through despite the test’s descriptive name. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=67776a0062694fd69d254aeff703be46&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=67776a0062694fd69d254aeff703be46&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/tasks/test_deletion_retention.py **Line:** 107:114 **Comment:** *Inconsistent Naming: The test name says it verifies a daily schedule, but the body only checks task registration and never validates cadence fields (cron/hour/minute), so it can pass even if the job is scheduled hourly or weekly. Add assertions on the schedule expression to match the “daily purge” contract. 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%2F41549&comment_hash=8f7086a88c2ce3e021c819e69af0ca9246b833dca1511c1af910348d8e32271a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=8f7086a88c2ce3e021c819e69af0ca9246b833dca1511c1af910348d8e32271a&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]
