Skip to content

Commit af48854

Browse files
BIMvoicelouistrue
andauthored
test(export): validate glTF against Khronos, and read our DXF back with a foreign parser (#3091)
* test(export): check glTF, COLLADA and DXF against the formats, not our own readers Three export formats had no external validator and no third-party fixture anywhere in the repo. The only reader of a GLB we write was our own `parseGLB`; the only reader of a DXF we write was our own `parser.ts`; COLLADA had no reader at all and every assertion was a substring of the output. A writer and a reader that agree with each other prove they share a convention, not that the convention is the format. Test-only. No exporter behaviour changed, and every check below was mutation-proved: the writer was broken, the check was confirmed to fail, and the writer was restored. glTF - `scripts/test-wasm-contract.mjs` now runs the Khronos glTF-Validator (`gltf-validator`, the reference implementation, ~400 KB of self-contained JS, no network at runtime, pinned to an exact version because its output IS the assertion) over the GLBs the real wasm exporter produces on both entry points, failing on errors AND warnings. `gltf_tests.rs` has NAMED that validator in a comment since #1516 without ever invoking it. It reports 0 errors and 0 warnings on today's output, across the from-bytes and from-meshes paths and across the committed samples, so a second test pins that the validator saw real geometry - a clean report over an empty GLB is the "check that cannot fail" trap, and the validator is perfectly happy with a file that declares none. `rust/export/src/gltf_conformance_tests.rs` adds the spec rules that lane cannot reach: `quantize` and the bounded/streaming assembler have no wasm binding, and they carry the unusual byte layouts. Checked there and nowhere else before: accessor TOTAL byteOffset alignment (bufferView offset plus accessor offset, against component size), declared `min`/`max` recomputed from the bytes actually written, index values against the primitive's own vertex count, `mode`/`componentType` legality on the f32 path, and the GLB chunk framing including which byte each chunk pads with. Writing it found a test-side trap worth naming: reaching the streaming assembler by setting `IFC_LITE_GLB_STREAM_THRESHOLD_MB=0` silently exercises the IN-MEMORY path, because 0 means "disabled" (`usize::MAX`). Only mutating the bounded assembler's bufferView offsets and seeing nothing fail revealed it; the test now calls `export_glb_streaming_bounded` directly. COLLADA - `rust/export/src/collada_conformance_tests.rs` checks the document's internal agreement: `count=` attributes against the data they introduce, every `#reference` resolving to a declared `id`, `<p>` indices inside the accessor they index, and `<input offset>` against the `<p>` stride. An out-of-range `<p>` index leaves all eleven pre-existing COLLADA tests green. DXF - `packages/drawing-2d/src/dxf/writer-interop.test.ts` reads the writer's output back with `dxf-parser` (npm, MIT), an unrelated third-party reader, and separately pins the raw group codes against the R12 rules a lenient reader never needs, because it does not need them: POLYLINE's `66` vertices-follow flag, the TEXT alignment point `11/21/31` that must accompany a non-zero `72`/`73`, section/table balance, and the absence of any post-R12 group code. Dropping the alignment point leaves all 74 other DXF tests green - our own parser cannot see it. No CI step gained a network dependency and no fixture was committed: all three additions run inside lanes that already exist. * test(drawing-2d): group 5 is not a post-R12 code, so stop asserting it is The R12 conformance test forbade group 5 (handle) alongside 100/330/370/ 410/420 under the rationale that all of them are post-R12. That is wrong for 5: R12 permits optional entity handles, gated on $HANDLING. The assertion was right about our output and wrong about the format, and the comment stated the false half. Split it. The post-R12 test keeps the five codes that genuinely postdate AC1009. A separate test pins the absence of handles as what it actually is — our deliberate choice, carrying the contract a writer would take on by emitting them ($HANDLING plus a $HANDSEED above every handle used). No assertion is dropped: group 5 is still required to be absent, and the new test says what a future writer emitting handles must do instead of deleting it. Mutation-checked: emitting `5\n2F` on LINE in writer.ts fails "emits no entity handles, so no $HANDLING/$HANDSEED contract is implied" and nothing else. Reverted. drawing-2d 402 passed across 35 files, with writer-interop 12 -> 13. --------- Co-authored-by: Louis Trümpler <78563314+louistrue@users.noreply.github.com>
1 parent bc2e5e5 commit af48854

10 files changed

Lines changed: 1143 additions & 11 deletions

File tree

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
---
2+
'@ifc-lite/drawing-2d': patch
3+
'@ifc-lite/export': patch
4+
---
5+
6+
Check the glTF, COLLADA and DXF exporters against the formats, not against our
7+
own readers.
8+
9+
Test-only; no exporter behaviour changed. Three export formats had no external
10+
validator and no third-party fixture anywhere in the repo: the only reader of a
11+
GLB we write was our own `parseGLB`, the only reader of a DXF we write was our
12+
own `parser.ts`, and COLLADA had no reader at all — every assertion was a
13+
substring of the output. A writer and a reader that agree with each other prove
14+
they share a convention, not that the convention is the format.
15+
16+
- **glTF**`scripts/test-wasm-contract.mjs` now runs the Khronos
17+
glTF-Validator (`gltf-validator`, the reference implementation, pinned exact)
18+
over the GLBs the real wasm exporter produces on both entry points, failing on
19+
errors *and* warnings, plus a guard that the validator saw actual geometry so
20+
a silently-empty export cannot pass vacuously. It reports 0 errors and 0
21+
warnings on today's output. `rust/export/src/gltf_conformance_tests.rs` adds
22+
the spec rules that lane cannot reach (`quantize`, the bounded/streaming
23+
assembler and the multi-buffer path have no wasm binding): accessor TOTAL
24+
byteOffset alignment, declared `min`/`max` recomputed from the bytes actually
25+
written, index values against the primitive's own vertex count,
26+
`mode`/`componentType` legality, and the GLB chunk framing and padding bytes.
27+
- **COLLADA**`rust/export/src/collada_conformance_tests.rs` checks the
28+
document's internal agreement: `count=` attributes against the data they
29+
introduce, every `#reference` resolving to a declared `id`, `<p>` indices
30+
inside the accessor they index, and `<input offset>` against the `<p>` stride.
31+
An out-of-range `<p>` index leaves all eleven pre-existing COLLADA tests green.
32+
- **DXF**`packages/drawing-2d/src/dxf/writer-interop.test.ts` reads the
33+
writer's output back with `dxf-parser` (npm, MIT), an unrelated third-party
34+
reader, and separately pins the raw group codes against the R12 rules a
35+
lenient reader never needs: POLYLINE's `66` vertices-follow flag, the TEXT
36+
alignment point `11/21/31` that must accompany a non-zero `72`/`73`, section
37+
balance, and the absence of any post-R12 group code. Dropping the alignment
38+
point leaves all 74 other DXF tests green.
39+
40+
Every check was mutation-proved: the writer was broken, the check was confirmed
41+
to fail, and the writer was restored.

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@
9090
"@playwright/test": "^1.62.1",
9191
"@posthog/cli": "0.11.1",
9292
"@types/node": "^26.2.0",
93+
"gltf-validator": "2.0.0-dev.3.10",
9394
"knip": "^6.32.2",
9495
"oxlint": "^1.42.0",
9596
"tsx": "^4.23.12",

packages/drawing-2d/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
},
2424
"devDependencies": {
2525
"@webgpu/types": "^0.1.71",
26+
"dxf-parser": "1.1.2",
2627
"typescript": "^6.0.3",
2728
"vitest": "^4.1.10"
2829
},
Lines changed: 242 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,242 @@
1+
/* This Source Code Form is subject to the terms of the Mozilla Public
2+
* License, v. 2.0. If a copy of the MPL was not distributed with this
3+
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */
4+
5+
/**
6+
* DXF writer conformance: read back by something that is NOT ours, and pinned
7+
* against the DXF R12 group codes the specification names.
8+
*
9+
* `writer.test.ts` round-trips every entity kind through `parser.ts` — this
10+
* package's own DXF reader. A writer and a reader that agree with each other
11+
* prove they share a convention, not that the convention is DXF: our parser is
12+
* deliberately lenient (it is built to survive files other tools wrote), so it
13+
* happily accepts output a strict CAD reader would reject or repair. There is
14+
* no `.dxf` file anywhere in this repository, so nothing else could disagree.
15+
*
16+
* Two independent checks close that:
17+
*
18+
* 1. `dxf-parser` (npm, MIT) — an unrelated third-party DXF reader, added as a
19+
* dev dependency. It is not a validator (no DXF validator exists as a
20+
* library), but it IS a foreign implementation: it has its own idea of what
21+
* the group codes mean, so agreement with it is evidence about the format
22+
* rather than about us.
23+
* 2. Assertions on the raw (code, value) pair stream against the R12 rules
24+
* themselves — the parts a lenient reader will not miss because it does not
25+
* need them (POLYLINE's `66` vertices-follow flag, the TEXT alignment point
26+
* `11/21/31` that must accompany a non-zero `72`/`73`, section balance).
27+
*
28+
* A third-party DXF *fixture* would be the other half — proving we can READ
29+
* what other tools write. None is committed here: model fixtures in this repo
30+
* are fetched from the manifest bucket rather than committed
31+
* (AGENTS.md §Test fixtures), and this suite has no fetch step.
32+
*/
33+
34+
import { describe, expect, it } from 'vitest';
35+
import DxfParserModule from 'dxf-parser';
36+
import type {
37+
DxfParser as DxfParserInstance,
38+
ILineEntity,
39+
IPolylineEntity,
40+
ITextEntity,
41+
} from 'dxf-parser';
42+
import { DxfWriter } from './writer.js';
43+
import { cssToAci } from './aci-colors.js';
44+
45+
/**
46+
* `dxf-parser` ships two builds: an ESM one whose `.d.ts` TypeScript resolves
47+
* (named + default export) and a CJS bundle whose `module.exports` IS the class,
48+
* which is what vitest loads. The repo compiles without `esModuleInterop`, so
49+
* the default import is typed as a namespace rather than the constructor.
50+
* Bridge the two once, here, instead of at every call site.
51+
*/
52+
const DxfParser = DxfParserModule as unknown as new () => DxfParserInstance;
53+
54+
/** One document exercising every entity kind the writer emits. */
55+
function buildDocument(): string {
56+
const w = new DxfWriter({ headerComment: 'ifc-lite export - units: metres' });
57+
const walls = w.layer('Wände', '#ff0000');
58+
const anno = w.layer('anno', '#000000', 'DASHED');
59+
w.addLine({ x: 1.5, y: -2.25 }, { x: 10, y: 20 }, walls);
60+
w.addPolyline([{ x: 0, y: 0 }, { x: 5, y: 0 }, { x: 5, y: 5 }, { x: 0, y: 5 }], walls, true);
61+
w.addPolyline([{ x: 0, y: 0 }, { x: 1, y: 0 }, { x: 2, y: 1 }], anno, false);
62+
w.addText({ x: 3, y: 4 }, 'Room 101', 0.25, anno, {
63+
rotationDeg: 90,
64+
hAlign: 'center',
65+
vAlign: 'middle',
66+
});
67+
w.addText({ x: 0, y: 0 }, 'plain', 0.2, anno, { colorOverride: '#00ff00' });
68+
return w.toString();
69+
}
70+
71+
/** The file as the (group code, value) pairs DXF is defined in terms of. */
72+
function pairs(dxf: string): Array<[string, string]> {
73+
const lines = dxf.split('\n');
74+
// A DXF file is an even number of lines: code, value, code, value…
75+
// (`toString` ends with a trailing newline, hence the pop.)
76+
if (lines[lines.length - 1] === '') lines.pop();
77+
expect(lines.length % 2, 'DXF is a stream of code/value PAIRS').toBe(0);
78+
const out: Array<[string, string]> = [];
79+
for (let i = 0; i < lines.length; i += 2) {
80+
expect(lines[i], `line ${i} must be a group code`).toMatch(/^-?\d+$/);
81+
out.push([lines[i].trim(), lines[i + 1]]);
82+
}
83+
return out;
84+
}
85+
86+
describe('DXF writer read back by an independent reader (dxf-parser, MIT)', () => {
87+
const dxf = buildDocument();
88+
const doc = new DxfParser().parseSync(dxf);
89+
90+
it('parses at all — a foreign reader accepts the file', () => {
91+
expect(doc).toBeTruthy();
92+
expect(doc?.entities ?? []).toHaveLength(5);
93+
});
94+
95+
it('declares R12 and the extents a foreign reader will frame the drawing by', () => {
96+
expect(doc?.header?.['$ACADVER']).toBe('AC1009');
97+
// Extents come from the finite geometry: x spans 0..10, y spans -2.25..20.
98+
expect(doc?.header?.['$EXTMIN']).toMatchObject({ x: 0, y: -2.25 });
99+
expect(doc?.header?.['$EXTMAX']).toMatchObject({ x: 10, y: 20 });
100+
});
101+
102+
it('carries both layers, sanitized, with their linetypes and colours', () => {
103+
const layers = doc?.tables?.layer?.layers ?? {};
104+
// 'Wände' sanitizes to the R12 symbol-name charset.
105+
expect(layers['W_nde']).toBeTruthy();
106+
expect(layers['W_nde'].color).toBe(0xff0000);
107+
expect(layers['anno']).toBeTruthy();
108+
const ltypes = doc?.tables?.lineType?.lineTypes ?? {};
109+
expect(Object.keys(ltypes).sort()).toEqual(['CONTINUOUS', 'DASHED']);
110+
});
111+
112+
it('reads the LINE at the coordinates it was given', () => {
113+
const line = doc?.entities.find((e) => e.type === 'LINE') as ILineEntity | undefined;
114+
expect(line?.layer).toBe('W_nde');
115+
expect(line?.vertices[0]).toMatchObject({ x: 1.5, y: -2.25 });
116+
expect(line?.vertices[1]).toMatchObject({ x: 10, y: 20 });
117+
});
118+
119+
it('reads the closed and open POLYLINEs with the right vertex counts and closed flag', () => {
120+
const polys = (doc?.entities ?? []).filter(
121+
(e): e is IPolylineEntity => e.type === 'POLYLINE',
122+
);
123+
expect(polys).toHaveLength(2);
124+
// `shape` is dxf-parser's name for group 70 bit 1 (closed).
125+
expect(polys[0].shape).toBe(true);
126+
expect(polys[0].vertices).toHaveLength(4);
127+
expect(polys[0].vertices[2]).toMatchObject({ x: 5, y: 5 });
128+
expect(polys[1].shape).toBe(false);
129+
expect(polys[1].vertices).toHaveLength(3);
130+
});
131+
132+
it('reads TEXT height, rotation, justification and the per-entity colour override', () => {
133+
const texts = (doc?.entities ?? []).filter((e): e is ITextEntity => e.type === 'TEXT');
134+
expect(texts).toHaveLength(2);
135+
expect(texts[0].text).toBe('Room 101');
136+
expect(texts[0].textHeight).toBe(0.25);
137+
expect(texts[0].rotation).toBe(90);
138+
expect(texts[0].halign).toBe(1); // group 72, centre
139+
expect(texts[0].valign).toBe(2); // group 73, middle
140+
expect(texts[0].startPoint).toMatchObject({ x: 3, y: 4 });
141+
expect(texts[1].text).toBe('plain');
142+
expect(texts[1].colorIndex).toBe(cssToAci('#00ff00'));
143+
});
144+
});
145+
146+
describe('DXF writer against the R12 group codes themselves (no parser involved)', () => {
147+
const dxf = buildDocument();
148+
const p = pairs(dxf);
149+
150+
it('opens with a 999 comment and closes with the mandatory 0/EOF', () => {
151+
expect(p[0]).toEqual(['999', 'ifc-lite export - units: metres']);
152+
expect(p[p.length - 1]).toEqual(['0', 'EOF']);
153+
});
154+
155+
it('balances every SECTION with an ENDSEC and every TABLE with an ENDTAB', () => {
156+
const count = (code: string, value: string) =>
157+
p.filter(([c, v]) => c === code && v === value).length;
158+
expect(count('0', 'SECTION')).toBe(3); // HEADER, TABLES, ENTITIES
159+
expect(count('0', 'ENDSEC')).toBe(3);
160+
expect(count('0', 'TABLE')).toBe(3); // LTYPE, STYLE, LAYER
161+
expect(count('0', 'ENDTAB')).toBe(3);
162+
});
163+
164+
it('gives POLYLINE the 66 "vertices follow" flag the R12 entity requires', () => {
165+
// A lenient reader infers the VERTEX chain from what follows and never
166+
// needs 66; a strict one treats its absence as "no vertices" and drops the
167+
// whole polyline. Nothing but this asserts it.
168+
const polyStarts = p.flatMap(([c, v], i) => (c === '0' && v === 'POLYLINE' ? [i] : []));
169+
expect(polyStarts).toHaveLength(2);
170+
for (const i of polyStarts) {
171+
const body = p.slice(i + 1, i + 12);
172+
expect(body.find(([c]) => c === '66')?.[1], 'POLYLINE needs 66/1').toBe('1');
173+
// The 10/20/30 "dummy point" is part of the R12 POLYLINE entity; the
174+
// real coordinates live on the VERTEX chain.
175+
expect(body.find(([c]) => c === '10')?.[1]).toBe('0.0');
176+
expect(body.find(([c]) => c === '70')).toBeTruthy();
177+
}
178+
});
179+
180+
it('closes every VERTEX chain with SEQEND, and every entity names its layer', () => {
181+
const entityStarts = p.flatMap(([c, v], i) =>
182+
c === '0' && ['LINE', 'POLYLINE', 'VERTEX', 'SEQEND', 'TEXT'].includes(v) ? [i] : [],
183+
);
184+
for (const i of entityStarts) {
185+
// Group 8 (layer) is mandatory on every entity; a missing one silently
186+
// reassigns the entity to layer "0" in a real reader.
187+
const eight = p.slice(i + 1, i + 3).find(([c]) => c === '8');
188+
expect(eight, `entity ${p[i][1]} at pair ${i} must carry group 8`).toBeTruthy();
189+
expect(eight?.[1]).not.toBe('');
190+
}
191+
const kinds = entityStarts.map((i) => p[i][1]);
192+
expect(kinds.filter((k) => k === 'SEQEND')).toHaveLength(
193+
kinds.filter((k) => k === 'POLYLINE').length,
194+
);
195+
});
196+
197+
it('emits the TEXT alignment point 11/21/31 exactly when 72/73 are non-zero', () => {
198+
// R12 TEXT: groups 72/73 select a justification, but the point the text is
199+
// justified ABOUT is 11/21/31 — 10/20/30 stays the (unused) first
200+
// alignment point. Set 72/73 without 11/21 and a conforming reader draws
201+
// the string at the origin. Set 11/21 with 72=73=0 and it is ignored.
202+
const starts = p.flatMap(([c, v], i) => (c === '0' && v === 'TEXT' ? [i] : []));
203+
expect(starts).toHaveLength(2);
204+
for (const i of starts) {
205+
const end = p.findIndex(([c], j) => j > i && c === '0');
206+
const body = p.slice(i + 1, end === -1 ? undefined : end);
207+
const g = (code: string) => body.find(([c]) => c === code)?.[1];
208+
expect(g('1'), 'TEXT needs its string in group 1').toBeTruthy();
209+
expect(g('40'), 'TEXT needs a height in group 40').toBeTruthy();
210+
const justified = g('72') !== '0' || g('73') !== '0';
211+
if (justified) {
212+
expect(g('11'), 'justified TEXT needs alignment point 11').toBeTruthy();
213+
expect(g('21'), 'justified TEXT needs alignment point 21').toBeTruthy();
214+
expect(g('31')).toBe('0.0');
215+
// …and it must be the SAME point as 10/20, not a stale or zeroed one.
216+
expect(g('11')).toBe(g('10'));
217+
expect(g('21')).toBe(g('20'));
218+
} else {
219+
expect(g('11'), 'unjustified TEXT must not carry an alignment point').toBeUndefined();
220+
}
221+
}
222+
});
223+
224+
it('uses no group code that only exists from R13 on', () => {
225+
// 100 (subclass marker), 330 (owner), 370 (lineweight), 410 (layout) and
226+
// 420 (true colour) are all post-R12; emitting one while declaring AC1009
227+
// makes a hybrid file that strict readers repair or reject.
228+
for (const forbidden of ['100', '330', '370', '410', '420']) {
229+
expect(p.some(([c]) => c === forbidden), `group ${forbidden} is post-R12`).toBe(false);
230+
}
231+
});
232+
233+
it('emits no entity handles, so no $HANDLING/$HANDSEED contract is implied', () => {
234+
// Group 5 is NOT post-R12 — R12 permits optional handles. This pins a
235+
// deliberate choice of ours, not a version rule: we emit none, and so we
236+
// owe no $HANDLING=1 header or $HANDSEED above every handle used. A writer
237+
// that starts emitting 5 must supply both, and should update this test
238+
// rather than delete it.
239+
expect(p.some(([c]) => c === '5'), 'group 5 (handle) must stay absent').toBe(false);
240+
expect(p.some(([c, v]) => c === '9' && v === '$HANDLING')).toBe(false);
241+
});
242+
});

pnpm-lock.yaml

Lines changed: 24 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)