test: fix operator precedence in manifest v2 assertions#3612
Open
anxkhn wants to merge 1 commit into
Open
Conversation
Six assertions in test_write_manifest and test_write_manifest_list used the form `assert x == a if format_version == 1 else b`. Because a conditional expression binds looser than `==`, Python parses this as `assert (x == a) if (format_version == 1) else b`, so for the format_version == 2 parametrization the whole statement collapses to `assert b`, where b is a truthy constant (3, or ManifestContent.DELETES). The v2 branch therefore always passed and never compared the actual value read back from the manifest. Wrap the right-hand side in parentheses so the conditional expression is the comparison target. This restores verification of the v2 sequence_number, content, min_sequence_number, and file_sequence_number on the manifest read/round-trip path. Test-only change; all parametrized cases (v1 and v2) pass. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Six assertions in
tests/utils/test_manifest.pyare written as:In Python a conditional expression (
a if cond else b) binds looser than==, so this parses as:Both enclosing tests,
test_write_manifestandtest_write_manifest_list, are@pytest.mark.parametrize("format_version", [1, 2]). For theformat_version == 2case eachstatement therefore collapses to
assert b, wherebis a truthy constant (3, or the enummember
ManifestContent.DELETES). The v2 branch always passed without ever comparing the valueread back from the manifest, so the following reads were effectively unverified for v2:
manifest_entry.sequence_numbermanifest_file.content,manifest_file.sequence_number,manifest_file.min_sequence_numberentry.sequence_number,entry.file_sequence_numberThis 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:
The values already present in the tests (
3,ManifestContent.DELETES) are the correctround-trip values; confirmed by temporarily injecting a wrong v2 value (
999), which the oldshape 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_version1 and 2.value to a deliberately wrong
999: with the original shape all v2 parametrizations stillpassed (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 wasreverted; it is not part of this PR.)
make lintpasses.Are there any user-facing changes?
No. This only affects test code; there is no change to library behavior.