From 9391011b8f99d0be208b4626bc300b19ab051e99 Mon Sep 17 00:00:00 2001
From: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Date: Sun, 5 Jul 2026 10:24:11 +0530
Subject: [PATCH] test: assert expired snapshot ids in expire_snapshots tests

test_expire_unprotected_snapshot and test_expire_snapshots_by_ids mocked the
catalog and hardcoded the commit_table response metadata to
snapshots=[KEEP_SNAPSHOT]. After commit, Table._do_commit sets
self.metadata = response.metadata, so the post-conditions only re-checked that
canned value: "EXPIRE_SNAPSHOT not in [KEEP_SNAPSHOT]" is int-not-in-list (always
true for distinct ids) and len(snapshots) == 1 just reads the mock. Both tests
still passed with ExpireSnapshots.by_id/by_ids neutered to no-ops, so they did
not exercise the expiration logic.

Assert the RemoveSnapshotsUpdate.snapshot_ids actually passed to commit_table
instead, matching the existing pattern in
test_expire_snapshots_by_timestamp_skips_protected. Compare as a set because the
update's ids come from a set. With this change both tests fail when the wrong
ids (or none) are expired.

Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
---
 tests/table/test_expire_snapshots.py | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/tests/table/test_expire_snapshots.py b/tests/table/test_expire_snapshots.py
index 106e5b786c..f95e8b31e6 100644
--- a/tests/table/test_expire_snapshots.py
+++ b/tests/table/test_expire_snapshots.py
@@ -105,9 +105,13 @@ def test_expire_unprotected_snapshot(table_v2: Table) -> None:
     table_v2.maintenance.expire_snapshots().by_id(EXPIRE_SNAPSHOT).commit()
 
     table_v2.catalog.commit_table.assert_called_once()
-    remaining_snapshots = table_v2.metadata.snapshots
-    assert EXPIRE_SNAPSHOT not in remaining_snapshots
-    assert len(table_v2.metadata.snapshots) == 1
+    # Inspect the RemoveSnapshotsUpdate actually sent to the catalog rather than the
+    # mocked commit response, so the assertion fails if the wrong ids are expired.
+    args, _ = table_v2.catalog.commit_table.call_args
+    updates = args[2] if len(args) > 2 else ()
+    remove_update = next((u for u in updates if getattr(u, "action", None) == "remove-snapshots"), None)
+    assert remove_update is not None
+    assert set(remove_update.snapshot_ids) == {EXPIRE_SNAPSHOT}
 
 
 def test_expire_nonexistent_snapshot_raises(table_v2: Table) -> None:
@@ -222,10 +226,13 @@ def test_expire_snapshots_by_ids(table_v2: Table) -> None:
     table_v2.maintenance.expire_snapshots().by_ids([EXPIRE_SNAPSHOT_1, EXPIRE_SNAPSHOT_2]).commit()
 
     table_v2.catalog.commit_table.assert_called_once()
-    remaining_snapshots = table_v2.metadata.snapshots
-    assert EXPIRE_SNAPSHOT_1 not in remaining_snapshots
-    assert EXPIRE_SNAPSHOT_2 not in remaining_snapshots
-    assert len(table_v2.metadata.snapshots) == 1
+    # Inspect the RemoveSnapshotsUpdate actually sent to the catalog rather than the
+    # mocked commit response, so the assertion fails if the wrong ids are expired.
+    args, _ = table_v2.catalog.commit_table.call_args
+    updates = args[2] if len(args) > 2 else ()
+    remove_update = next((u for u in updates if getattr(u, "action", None) == "remove-snapshots"), None)
+    assert remove_update is not None
+    assert set(remove_update.snapshot_ids) == {EXPIRE_SNAPSHOT_1, EXPIRE_SNAPSHOT_2}
 
 
 def test_thread_safety_fix() -> None: