fix: teach the api round-trip harness the page-margins both-collapse - #327
Merged
Conversation
PageMarginsData::same() (documented on the class itself) intentionally collapses a <page-margins type="odd">/<page-margins type="even"> pair with identical values into a single type="both" element on write. Verified against 99 corpus files with this drop signature: every one has byte-identical odd/even margin values (typical of real-world exporters using symmetric page margins), so this is the documented api simplification firing repeatedly, not a fidelity bug -- exactly the "api design choice incompatible with round-trip fidelity" case the round-trip harness is supposed to absorb. Added collapseEqualPageMargins() to CorpusRoundtripMain.cpp, mirroring the existing canonicalizeEncodingChildOrder() pattern (#220): pre-collapse the expected document's odd/even page-margins pair the same way the api does before comparing, so only a genuine drop or value mismatch surfaces. Applied in both the regression/discovery comparison and the dump path that feeds the failure classifier. ## Testing - [x] make test: all pass (4724 assertions in 378 test cases, plus examples) - [x] make test-api-roundtrip: 158 passed, 0 failed (of 158 pinned; no regressions -- a harness-only change, no api behavior changed, so no new file fully passes, but drop:page-margins / value:page-margins@type are gone from make classify-api-roundtrip's worklist across the whole corpus) - [x] make fmt / make check: clean Closes #277
This was referenced Jul 7, 2026
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28513 / 36640 |
| Functions | 74.3% | 6352 / 8551 |
| Branches | 50.7% | 22678 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 80.8% | 6391 / 7913 |
| Functions | 67.7% | 2217 / 3275 |
| Branches | 49.5% | 5476 / 11052 |
Core HTML report | API HTML report
Commit 3a92fe3ec6d356853fb0c4d6a26fb3ebbcd37c73.
gen-quality
|
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.
Human Summary
I'm not sure exactly why this PR didn't unblock any corpus files for mx::api round trip testing, and I'm a little bit concerned that someday someone will prefer that we not do this page margin collapse "simplification", but... it seems pretty harmless and it will make more files roundtripable for testing.
Summary
PageMarginsData::same()(documented on the class itself) intentionally collapses a<page-margins type="odd">/<page-margins type="even">pair with identical values into a singletype="both"element on write. Verified against all 99 corpus files carrying this drop signature:every one has byte-identical odd/even margin values (typical of real-world exporters using
symmetric page margins), so this is the documented api simplification firing repeatedly, not a
fidelity bug -- exactly the "api design choice incompatible with round-trip fidelity" case the
round-trip harness is supposed to absorb, per the same reasoning already applied to
<encoding>child order (#220).
Added
collapseEqualPageMargins()toCorpusRoundtripMain.cpp, mirroring the existingcanonicalizeEncodingChildOrder()pattern: pre-collapse the expected document's odd/evenpage-marginspair the same way the api does before comparing, so only a genuine drop or valuemismatch surfaces. Applied in both the regression/discovery comparison and the dump path that
feeds the failure classifier.
No
mx::api/mx::implbehavior changes -- this is a test-harness-only fix.Testing
make test: all pass (4724 assertions in 378 test cases, plus the three examples)make test-api-roundtrip: 158 passed, 0 failed (of 158 pinned; no regressions -- aharness-only change, no api behavior changed, so no new file fully passes, but
drop:page-margins/value:page-margins@typeare gone frommake classify-api-roundtrip'sworklist across the whole corpus)
make fmt/make check: cleanReferences