Test that the 3MF we write is actually compressed - #90
Merged
Conversation
writeThreeMf() has called writeDeflateZip() all along, but nothing checked it. The round-trip tests cannot: readStoredZipEntry accepts both STORED and DEFLATE, so swapping the writer back to writeStoredZip leaves every existing test green and shows up only as files several times larger than they need to be. Confirmed by doing exactly that -- ThreeMfRoundTripPreservesVolume still passed. ThreeMfIsDeflateCompressed reads the compression method straight out of the central directory of a real exported .3mf and requires method 8, plus a compressed size below the raw one. It fails on that same swap. Four smaller tests do the same for the zip layer itself: writeDeflateZip emits method 8 for compressible input, actually shrinks it, falls back to STORED for input too small to benefit, and writeStoredZip still emits method 0 as the control. export.hpp said the 3MF was "written as a stored (uncompressed) ZIP", which has been wrong since the deflate path landed and is corrected here. That comment was read as fact and repeated as one, which is what prompted this. 830 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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.
writeThreeMf()has calledwriteDeflateZip()all along — but nothing checked it, andexport.hppclaimed the opposite.The gap
The existing round-trip tests cannot catch a regression here.
readStoredZipEntryaccepts both STORED and DEFLATE, so swapping the writer back towriteStoredZipleaves every test green and shows up only as files several times larger than they need to be.Confirmed by doing exactly that:
What's added
ThreeMfIsDeflateCompressedexports a real cube through the evaluator pipeline, then reads the compression method straight out of the archive's central directory (authoritative, unlike the local header) and requires method 8 with a compressed size below the raw one.Four smaller tests pin the zip layer itself:
DeflateZipReallyWritesMethod8DeflateZipActuallyShrinksCompressibleDataDeflateZipStoresDataThatWouldNotShrinkStoredZipWritesMethod0The comment
export.hppdescribed the 3MF as "written as a stored (uncompressed) ZIP". That has been wrong since the deflate path landed. Corrected.Worth saying plainly why this PR exists: I read that comment, reported it downstream as fact, and only found the code said otherwise when asked a follow-up question. A stale comment that contradicts the code one file over is worth a test, not just a fix.
830 tests pass. Version 0.29.1 → 0.29.2.
🤖 Generated with Claude Code