sadpandajoe commented on code in PR #45029:
URL: https://github.com/apache/superset/pull/45029#discussion_r4199904962
##########
tests/unit_tests/tasks/test_deletion_retention.py:
##########
@@ -573,4 +576,181 @@ def test_standalone_window_bounds_reach_safe_purge_cutoff(
assert result == {"skipped": 1}
purge.assert_not_called()
else:
- purge.assert_called_once_with(Slice, now - timedelta(days=expected),
False)
+ purge.assert_called_once_with(
+ Slice, now - timedelta(days=expected), False, max_per_run=None
+ )
+
+
+def test_purge_cap_counts_committed_roots_across_batches(app_context: None) ->
None:
+ """A failed or blocked root does not consume the successful-purge
budget."""
+ from superset.commands.deletion_retention.purge_cascade import
CascadeResult
+ from superset.models.slice import Slice
+ from superset.tasks import deletion_retention as task
+
+ purged: CascadeResult = CascadeResult(
+ purged=True, entity_type="chart", entity_uuid="purged"
+ )
+ not_purged: CascadeResult = CascadeResult(
+ purged=False, entity_type="chart", entity_uuid="not-purged"
+ )
+ purge_one: MagicMock
+ with (
+ patch.object(task, "_iter_eligible_ids", return_value=[[1, 2], [3,
4]]) as scan,
+ patch.object(
+ task, "_purge_one", side_effect=[not_purged, purged, purged]
+ ) as purge_one,
+ ):
+ result: tuple[int, int, int, int] = task._purge_model(
+ Slice, datetime.now(), dry_run=False, max_per_run=2
+ )
+
+ assert result == (2, 0, 0, 0)
+ assert [call.args[1] for call in purge_one.call_args_list] == [1, 2, 3]
+ scan.assert_called_once()
+
+
+def test_scheduled_purge_rejects_invalid_cap_before_work(app_config: Config)
-> None:
+ """Malformed scheduled budgets fail closed instead of removing
everything."""
+ from superset.tasks import deletion_retention as task
+
+ app_config["SOFT_DELETE_PURGE_MAX_PER_RUN"] = -1
+ purge: MagicMock
+ with (
+ patch.object(task, "_purge_impl") as purge,
+ patch.object(
+ task.feature_flag_manager, "is_feature_enabled", return_value=True
+ ),
+ ):
+ result: dict[str, Any] = task.purge_soft_deleted.run()
+
+ assert result == {"skipped_invalid_cap": 1}
+ purge.assert_not_called()
+
+
+def test_purge_budget_spans_models_and_runs(app_context: None) -> None:
+ """A full first run leaves later models for a subsequent invocation."""
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+ from superset.tasks import deletion_retention as task
+
+ purge: MagicMock
+ with (
+ patch.object(task, "_ordered_purge_models", return_value=[Slice,
Dashboard]),
+ patch.object(
+ task,
+ "_purge_model",
+ side_effect=[
+ (2, 0, 0, 0),
+ (0, 0, 0, 0),
+ (1, 0, 0, 0),
+ ],
+ ) as purge,
+ patch.object(task, "_count_eligible", side_effect=[0, 3, 0, 2]),
+ patch.object(task.audit, "reconcile_pending"),
+ ):
+ first: dict[str, Any] = task._purge_impl(30, False, max_per_run=2)
+ second: dict[str, Any] = task._purge_impl(30, False, max_per_run=2)
+
+ assert first["purged"] == {"slices": 2}
+ assert first["cap_reached"] is True
+ assert first["remaining_eligible"] == 3
+ assert second["purged"] == {"dashboards": 1}
+ assert second["cap_reached"] is False
+ assert second["remaining_eligible"] == 2
+ assert [entry.args[0] for entry in purge.call_args_list] == [
+ Slice,
+ Slice,
+ Dashboard,
+ ]
+ assert [entry.kwargs["max_per_run"] for entry in purge.call_args_list] == [
Review Comment:
This only asserts allowances of `2` for every call, and the first model
either uses the whole budget or none of it. If `_scan_purge_models` passed the
original cap to each model instead of the remaining budget, this test would
still pass, and a run with cap `2` could purge three roots. Could you add a
case where the first model purges one root (cap `2`) and the second has two
eligible roots, asserting the second gets `max_per_run=1` and only one more
root is purged?
--
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]
