Skip to content

test: assert expired snapshot ids in expire_snapshots tests#3615

Open
anxkhn wants to merge 1 commit into
apache:mainfrom
anxkhn:test/expire-snapshots-assert-remove-update
Open

test: assert expired snapshot ids in expire_snapshots tests#3615
anxkhn wants to merge 1 commit into
apache:mainfrom
anxkhn:test/expire-snapshots-assert-remove-update

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

test_expire_unprotected_snapshot and test_expire_snapshots_by_ids in
tests/table/test_expire_snapshots.py do not actually exercise the expiration
logic. Both tests mock the catalog and hardcode the commit_table response
metadata to snapshots=[KEEP_SNAPSHOT]. After the commit, Table._do_commit
sets self.metadata = response.metadata, so table_v2.metadata.snapshots
becomes that canned value regardless of which ids ExpireSnapshots actually
selected. The post-conditions then only re-read the mock:

  • assert EXPIRE_SNAPSHOT not in remaining_snapshots compares an int against a
    list of a different int, which is always true.
  • assert len(table_v2.metadata.snapshots) == 1 just reads the hardcoded
    single-element list.

As a result, both tests still pass even when ExpireSnapshots.by_id / by_ids
are neutered to no-ops (nothing is expired), so a regression that selected the
wrong snapshot ids, or none, would not be caught.

This mirrors the assertion style already used correctly in the same file by
test_expire_snapshots_by_timestamp_skips_protected, which inspects the
RemoveSnapshotsUpdate sent to commit_table instead of the mocked response.

Are these changes tested?

Yes; this change is entirely to tests. It replaces the assertions in the two
tests above with an inspection of the RemoveSnapshotsUpdate.snapshot_ids
actually passed to commit_table (via call_args), comparing as a set because
the update's ids are built from a set.

  • python -m pytest tests/table/test_expire_snapshots.py -> 9 passed.
  • To confirm the assertions now have teeth: with ExpireSnapshots.by_id /
    by_ids temporarily stubbed to no-ops, both tests now fail (the captured
    snapshot_ids set is empty), whereas before this change they still passed.

Are there any user-facing changes?

No. Test-only change; no library code is modified.


Notes for maintainers (optional comment, not part of the template)

  • No new tests are added; this tightens two existing ones so they assert the
    real expiration result. The other tests in the file are unchanged.
  • The inline call_args inspection is duplicated across three tests now
    (including the pre-existing ..._skips_protected). I kept it inline to match
    the established in-file pattern rather than introduce a helper in this PR; happy
    to factor it out if preferred.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant