anxkhn opened a new pull request, #3615:
URL: https://github.com/apache/iceberg-python/pull/3615
# 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.
--
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]
