bito-code-review[bot] commented on code in PR #44892:
URL: https://github.com/apache/superset/pull/44892#discussion_r4187287476
##########
tests/unit_tests/tasks/test_deletion_retention.py:
##########
@@ -330,6 +331,79 @@ class UnsupportedModel(SoftDeleteMixin):
engine.dispose()
+def test_scan_failure_keeps_the_counts_earned_before_it(app_context: None) ->
None:
+ """A scan that fails part-way reports itself and keeps what it purged.
+
+ By the time a later page fails, the earlier page's deletions are
+ committed. Discarding the counts would make the run's summary and its
+ purge gauges understate what was actually removed.
+ """
+ import superset.tasks.deletion_retention as mod
+ from superset.commands.deletion_retention.purge_cascade import
CascadeResult
+ from superset.models.slice import Slice
+
+ purged_result: CascadeResult = CascadeResult(
+ purged=True, entity_type="chart", entity_uuid="gone"
+ )
+
+ def pages(*args: Any, **kwargs: Any) -> Iterator[list[int]]:
+ yield [1, 2]
+ raise RuntimeError("no such column: id")
+
+ with (
+ patch.object(mod, "_iter_eligible_ids", side_effect=pages),
+ patch.object(mod, "_purge_one", return_value=purged_result),
+ ):
+ purged, would, failures, blocked, scan_failures = mod._purge_model(
+ Slice, datetime.now(), dry_run=False
+ )
+
+ assert (purged, would, failures, blocked) == (2, 0, 0, 0)
+ assert scan_failures == 1
+
+
+def test_root_without_a_table_name_does_not_abort_the_run(
+ app_config: Config,
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """Resolving a root's table name is itself guarded.
+
+ The name is read off the model, so a root that cannot supply one must be
+ counted and skipped like any other failing root -- not end the pass before
+ the roots that could have purged are reached.
+ """
+ # avoid app-init regression: model helpers require the app_config fixture
first.
+ from superset.models.helpers import SoftDeleteMixin
+ from superset.tasks import deletion_retention as task
+
+ supported_models: list[type[SoftDeleteMixin]] =
list(task.purge_policy_registry())
+
+ class NoTableName(SoftDeleteMixin):
+ """A registered root whose table name cannot be read."""
+
+ monkeypatch.setattr(
+ SoftDeleteMixin,
+ "_registered_subclasses",
+ [NoTableName, *supported_models],
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Registry pollution leaks across tests</b></div>
<div id="fix">
`NoTableName` leaks into the real `SoftDeleteMixin._registered_subclasses`:
`__init_subclass__` (superset/models/helpers.py:1374-1382) appends it to the
live list at class-definition time (line 381), before `monkeypatch.setattr`
(384) rebinds the attribute — and setattr restores the original list object,
still containing `NoTableName`. Sibling tests avoid this by swapping the
registry before defining the class (lines 256→259, 420→422). The leaked
unmapped, table-less class is consumed by the visibility listener via
`_all_soft_delete_subclasses` (helpers.py:1489) and by `_soft_delete_models()`
in later tests.
</div>
</div>
<small><i>Code Review Run #7e6878</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
