anxkhn opened a new pull request, #3612:
URL: https://github.com/apache/iceberg-python/pull/3612
# Rationale for this change
Six assertions in `tests/utils/test_manifest.py` are written as:
```python
assert x == a if format_version == 1 else b
```
In Python a conditional expression (`a if cond else b`) binds looser than
`==`, so this parses as:
```python
assert (x == a) if (format_version == 1) else b
```
Both enclosing tests, `test_write_manifest` and `test_write_manifest_list`,
are
`@pytest.mark.parametrize("format_version", [1, 2])`. For the
`format_version == 2` case each
statement therefore collapses to `assert b`, where `b` is a truthy constant
(`3`, or the enum
member `ManifestContent.DELETES`). The v2 branch always passed without ever
comparing the value
read back from the manifest, so the following reads were effectively
unverified for v2:
- `manifest_entry.sequence_number`
- `manifest_file.content`, `manifest_file.sequence_number`,
`manifest_file.min_sequence_number`
- `entry.sequence_number`, `entry.file_sequence_number`
This is a masking / weak-test issue, not a production bug. Wrapping the
right-hand side of each
assertion in parentheses restores the intended comparison for both format
versions:
```python
assert x == (a if format_version == 1 else b)
```
The values already present in the tests (`3`, `ManifestContent.DELETES`) are
the correct
round-trip values; confirmed by temporarily injecting a wrong v2 value
(`999`), which the old
shape accepted but the parenthesized shape correctly rejects.
## Are these changes tested?
Yes, this change is itself a test-only strengthening.
- `pytest tests/utils/test_manifest.py -k "test_write_manifest or
test_write_manifest_list"`
passes (14 passed) for both `format_version` 1 and 2.
- To confirm the assertions now actually verify the v2 values, I temporarily
set a v2 expected
value to a deliberately wrong `999`: with the original shape all v2
parametrizations still
passed (the bug), and with the parenthesized shape the v2 cases fail with
`AssertionError: assert 3 == 999`, i.e. the value is now checked. (The
temporary edit was
reverted; it is not part of this PR.)
- `make lint` passes.
## Are there any user-facing changes?
No. This only affects test code; there is no change to library behavior.
<!-- Note for reviewers: there are two remaining instances of the same
operator-precedence
shape in tests/integration/test_writes/test_writes.py (around lines
1650-1651), where the v1
half of the assertion is the unverified branch. Those live in a
@pytest.mark.integration test
(Spark/Docker), so I left them out of this PR to keep it to the unit tests I
could run and
verify locally, and to avoid mixing test tiers. Happy to send a follow-up
for those, or fold
them in here if you prefer. -->
--
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]
