Skip to content

Make the input buffer tests of IgzipDecompressor reach the input buffer - #306

Open
ptweezy wants to merge 2 commits into
pycompression:developfrom
ptweezy:test-decompressor-inputbuf
Open

ptweezy wants to merge 2 commits into
pycompression:developfrom
ptweezy:test-decompressor-inputbuf

Conversation

@ptweezy

@ptweezy ptweezy commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

test_decompressor_inputbuf_1, _2, and _3 come from CPython's test_bz2. They expect part of a 100 to 300 byte input to be left over in the input buffer of IgzipDecompressor. But isal_inflate decompresses up to about 64 KiB ahead into a buffer of its own, so it reads these inputs in full. With gcov, the input buffer code in decompress() in igzip_libmodule.c runs in none of the three tests.

This PR uses 512 KiB of random data, which needs about as much input as output, and multiplies the offsets by 1,000. In the first test, the second call asks for 80,000 bytes instead of 2, because isal_inflate first returns the data in its own buffer and reads more input only after that. With gcov, the tests now do what their comments say: the first moves the contents of the input buffer to its start with memmove, the second appends to the buffer without moving or resizing it, and the third resizes it with PyMem_Realloc.

To test it, I made two changes to decompress(). Without the memmove, the first test fails. When the offset is dropped after PyMem_Realloc, the second test fails. All tests in tests/test_igzip_lib.py on develop pass with both changes. With an AddressSanitizer build of igzip_lib, the new tests pass. Together, the three tests take about 0.2 s.

This PR changes only tests, so it has no CHANGELOG entry.

Checklist

  • Pull request details were added to CHANGELOG.rst
  • Documentation was updated (if needed)

isal_inflate decompresses up to about 64 KiB ahead into its own buffer, so it read the 100 to 300 byte inputs of these tests in full, and the input buffer was never used. Use 512 KiB of random data and multiply the offsets by 1,000.

@rhpvorderman rhpvorderman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a minor naming nitpick.

Comment thread tests/test_igzip_lib.py Outdated
Comment on lines +80 to +81
RANDOM_TEXT = random.Random(0).randbytes(512 * 1024)
RANDOM_DATA = igzip_lib.compress(RANDOM_TEXT)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit unfortunate naming of the variables. This should be RANDOM_DATA as it is not text and COMPRESSED_RANDOM_DATA respectively.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, renamed them in 5ae2de7.

The uncompressed data is random bytes, not text, so RANDOM_TEXT becomes RANDOM_DATA and the compressed data becomes COMPRESSED_RANDOM_DATA.
@ptweezy
ptweezy requested a review from rhpvorderman October 28, 2026 13:13

This branch has not been deployed

No deployments
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.

2 participants