metaUtils: make the (de)compression chunk size settable, and cover the trailer case - #148
Conversation
|
@dzenanz would you review this one as well? It should merge after #147, and I don't have merge rights here. The red GitHub Actions check is the intended failure and will clear once #147 merges. This PR adds the regression test for the defect #147 fixes, so against Exactly one test fails, and it is the new one. The other 14 pass. Proposed sequence
The two changes touch different regions of Verification that the test detects this specific defectI disabled only the fix loop from #147 with
So the failure tracks the fix rather than an unrelated build difference. The test uses 64 KiB of data and runs in 0.01 s. Happy to drop the test, or move |
706d433 to
9a10b9a
Compare
…e trailer case MET_MaxChunkSize was a file-local constexpr of 1 GiB, so the input-chunking path in MET_PerformCompression and MET_PerformUncompression could only be reached by a test allocating several gigabytes. Expose MET_SetMaxChunkSize and MET_GetMaxChunkSize so the boundary can be placed where a test needs it, following the existing META_DEBUG precedent for a settable diagnostic knob. Values of zero or less are ignored. testMeta15UncompressChunkBoundary uses this to put a chunk boundary inside the 8-byte gzip trailer with 64 KiB of data, and asserts both that a valid stream is accepted and that a corrupt CRC trailer is still rejected. The test runs in 0.01 s. The accepted case fails on master today; it passes with the fix in Kitware#147. This change is independent of that one and can merge in either order. Assisted-by: Claude Code -- test design and verification
9a10b9a to
bab6acf
Compare
|
Thanks for merging #147, @dzenanz. Rebased on top of it — GitHub Actions is green now (15/15 tests pass, including the new The remaining red CircleCI check is the pre-existing project-wide one: |
Makes
MET_MaxChunkSizesettable so the input-chunking path can be reached by a cheap test, and adds a regression test for the deferred-trailer case.Depends on #147; stays draft until then. The new test exercises the defect #147 fixes, so it fails against
masteras it stands. The plan is to merge #147 first, rebase this branch on top of it, and only then mark this ready — CI here is not expected to be green before that.The two changes are otherwise independent: they touch different regions of
metaUtils.cxx, share no commits, and merge cleanly in either order (verified both directions).Why the API change is needed for the test
MET_MaxChunkSizewas a file-localconstexprof 1 GiB, so the only way to reach the chunk boundary was to allocate several gigabytes: the smallest reproduction I could build against the shipped constant needed a 1073741829-byte compressed stream, found by binary-searching the raw size until the boundary landed inside the 8-byte trailer. That is not something to put in CI.With a settable chunk size the test simply chooses the boundary —
MET_SetMaxChunkSize(compressedSize - 4)places the split inside the trailer deterministically, with 64 KiB of data and no search.metaUtils.halready exportsextern bool META_DEBUG, so a settable knob is not a new pattern here. Values of zero or less are ignored, and the test saves and restores the previous value so test ordering is unaffected.What the test asserts
testMeta15UncompressChunkBoundary, 0.01 s:The second case matters: without it, the test could be satisfied by simply weakening the stream-end requirement rather than by consuming the trailer.
Verified by disabling only the fix loop from #147 while keeping this API, so the test is known to detect that specific defect rather than an unrelated build difference:
master(no fix)zlib error -5