test: Add unit tests for ParseNumberFromBrackets - #3878
Conversation
pyiceberg/utils/parsing.py was the only module under pyiceberg/utils without a dedicated test. ParseNumberFromBrackets backs parsing of bucket[N], truncate[N] and fixed[L], so cover its match behaviour and the ValidationError raised on malformed input.
|
Hi @kevinjqliu, @Fokko |
|
|
||
|
|
||
| def test_match_reads_multi_digit_values() -> None: | ||
| assert ParseNumberFromBrackets("fixed").match("fixed[1024]") == 1024 |
There was a problem hiding this comment.
The multi digit is already covered by test_match_returns_the_bracketed_number, right?
There was a problem hiding this comment.
You're right, fixed[22] / truncate[16] already cover it. Removed the extra case, thanks.
| def test_match_ignores_text_around_the_prefix() -> None: | ||
| # the implementation searches for the pattern, so surrounding text is tolerated | ||
| assert ParseNumberFromBrackets("fixed").match(" fixed[5] ") == 5 | ||
| assert ParseNumberFromBrackets("bucket").match("transform=bucket[4]") == 4 | ||
|
|
||
|
|
||
| def test_match_returns_the_first_occurrence() -> None: | ||
| assert ParseNumberFromBrackets("bucket").match("bucket[3] bucket[7]") == 3 |
There was a problem hiding this comment.
That's true today, but I'm curious if that should be the expected behavior. Raising an exception for such cases is standard practice for me.
There was a problem hiding this comment.
Fair point, I'd lean the same way. I've dropped the tests that relied on that lenient re.search behavior so this PR doesn't pin it, tightening match to reject surrounding text seems worth a separate follow-up.
| def test_match_raises_when_brackets_are_missing() -> None: | ||
| with pytest.raises(ValidationError): | ||
| ParseNumberFromBrackets("fixed").match("fixed") | ||
|
|
||
|
|
||
| def test_match_raises_for_a_non_numeric_argument() -> None: | ||
| with pytest.raises(ValidationError): | ||
| ParseNumberFromBrackets("truncate").match("truncate[abc]") | ||
|
|
||
|
|
||
| def test_match_raises_for_a_negative_number() -> None: | ||
| # the pattern only accepts digits, so a leading minus sign does not match | ||
| with pytest.raises(ValidationError): | ||
| ParseNumberFromBrackets("truncate").match("truncate[-1]") |
There was a problem hiding this comment.
Can we verify the exception message?
There was a problem hiding this comment.
Done, the invalid-input cases are parametrized now and assert the full message (Could not match , expected format [22]).
Verify the full ValidationError message for the invalid-input cases and parametrize them. Remove the multi-digit case (already covered) and the cases that relied on re.search tolerating text around the pattern.
2 participants
pyiceberg/utils/parsing.py was the only module under pyiceberg/utils without a dedicated test. ParseNumberFromBrackets backs parsing of bucket[N], truncate[N] and fixed[L], so cover its match behaviour and the ValidationError raised on malformed input.
Rationale for this change
Are these changes tested?
It is a unit tests which are tested in local console.
Are there any user-facing changes?