aminghadersohi commented on code in PR #43838:
URL: https://github.com/apache/superset/pull/43838#discussion_r3992583429
##########
tests/unit_tests/versioning/test_activity.py:
##########
@@ -480,46 +481,130 @@ def test_changed_by_projects_only_display_fields() ->
None:
# ---- impact_for_record (pure, post-batch) -------------------------------
-def test_impact_for_record_dashboard_path_dataset_related_uses_count() -> None:
+def test_impact_for_record_dashboard_path_dataset_related_uses_charts() ->
None:
"""The only path/related shape that carries impact: ``Dashboard`` →
- ``SqlaTable``. The count comes from the pre-batched lookup."""
+ ``SqlaTable``. Count and names both come from the pre-batched lookup
+ (sc-119775: the tooltip needs the names, not just the count)."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
- counts = {(5, 100): 3}
- assert impact_for_record(record, "Dashboard", counts) == {"charts": 3}
+ charts: list[ChartRef] = [
+ {"id": 11, "name": "Alpha"},
+ {"id": 12, "name": "Beta"},
+ {"id": 13, "name": "Gamma"},
+ ]
+ assert impact_for_record(record, "Dashboard", {(5, 100): charts}) == {
+ "charts": 3,
+ "chart_names": charts,
+ }
-def test_impact_for_record_missing_count_yields_none() -> None:
+def test_impact_for_record_missing_pair_yields_none() -> None:
"""A pair the batch query didn't return (no matching siblings)
collapses to ``None`` rather than ``{"charts": 0}``."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
assert impact_for_record(record, "Dashboard", {}) is None
-def test_impact_for_record_zero_count_yields_none() -> None:
- """Explicit zero in the counts map is treated the same as missing —
- no impact field on the wire."""
+def test_impact_for_record_empty_charts_yields_none() -> None:
+ """An explicit empty list in the impacts map is treated the same as
+ missing — no impact field on the wire."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
- assert impact_for_record(record, "Dashboard", {(5, 100): 0}) is None
+ assert impact_for_record(record, "Dashboard", {(5, 100): []}) is None
def test_impact_for_record_dashboard_path_chart_related_yields_none() -> None:
"""Dashboard → chart is a direct dependency; no further sibling
layer to count."""
record = {"entity_kind": "chart", "entity_id": 5, "transaction_id": 100}
- assert impact_for_record(record, "Dashboard", {(5, 100): 999}) is None
+ charts: list[ChartRef] = [{"id": 9, "name": "X"}]
+ assert impact_for_record(record, "Dashboard", {(5, 100): charts}) is None
def test_impact_for_record_chart_path_with_dataset_related_yields_none() ->
None:
"""Chart → dataset: the chart is itself the only dependent of the
dataset edit."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
- assert impact_for_record(record, "Slice", {(5, 100): 999}) is None
+ charts: list[ChartRef] = [{"id": 9, "name": "X"}]
+ assert impact_for_record(record, "Slice", {(5, 100): charts}) is None
def test_impact_for_record_dataset_path_yields_none() -> None:
"""Datasets have no transitive layer (AV-004)."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
- assert impact_for_record(record, "SqlaTable", {(5, 100): 999}) is None
+ charts: list[ChartRef] = [{"id": 9, "name": "X"}]
+ assert impact_for_record(record, "SqlaTable", {(5, 100): charts}) is None
+
+
+def test_sorted_chart_refs_orders_case_insensitively_with_id_tiebreak() ->
None:
+ """The wire order is deterministic: casefolded name, then id; empty
+ names sort first (they render as an Untitled fallback)."""
+ from superset.versioning.activity.impact import _sorted_chart_refs
+
+ refs = _sorted_chart_refs({(5, 100): {3: "beta", 1: "Alpha", 2: "alpha",
4: ""}})
+ assert refs == {
+ (5, 100): [
+ {"id": 4, "name": ""},
+ {"id": 1, "name": "Alpha"},
+ {"id": 2, "name": "alpha"},
+ {"id": 3, "name": "beta"},
+ ]
+ }
Review Comment:
This fixture pins neither half of the sort key: `"", "Alpha", "alpha",
"beta"` already sorts that way raw, so dropping `casefold()`, dropping the `id`
tiebreak, or replacing the key with plain `chart["name"]` each leaves it green
(measured). A fixture that reds on all three:
```suggestion
refs = _sorted_chart_refs({(5, 100): {3: "Beta", 2: "Alpha", 1: "alpha",
4: ""}})
assert refs == {
(5, 100): [
{"id": 4, "name": ""},
{"id": 1, "name": "alpha"},
{"id": 2, "name": "Alpha"},
{"id": 3, "name": "Beta"},
]
}
```
--
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]
