fix(ysync): wire dialect == file dialect, uuid churn, drift noise
The Y.Doc is the source of truth for the FILE, but both live wires wrote KiCad's CLIPBOARD dialect — a lossy, paste-oriented format. Every difference was permanent, unfixable drift. - pcbnew: serialize footprint blobs with CTL_FOR_BOARD, not CLIPBOARD_IO's CTL_FOR_CLIPBOARD, which emitted (version)(generator)(generator_version) inside every (footprint …). Keep (locked yes). - eeschema: aForClipboard=false — clipboard mode collapsed every symbol's (instances … (path "/sheet")) to (path ""). - Re-supply (version) at PARSE time only (withFootprintVersion): the token is invalid file content but load-bearing on decode — without it the parser starts at m_requiredVersion=0 and stamps (hide yes) on every mandatory field. - FOOTPRINT copy ctor: restore mandatory-field uuids (EDA_ITEM::operator= keeps the target's const m_Uuid, so Clone() rerolled all four). - drift: classify order-only diffs as `reordered` — y-sexpr v2 reorders legitimately; excluded from counts, report-worthiness and dedupe hashes. Migration 0017. - fpedit from eeschema: AsyncLoad()+BlockUntilLoaded() in initLibraryTree — FACE_PCB starts lazily there and never preloaded its libraries. Guards: wire-vs-file round-trip tests (pcbnew + eeschema), fpedit-from-eeschema (verified red without the fix), symedit-from-eeschema. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016p9kjdGBdcpwUSjJ3q5xg2
This commit is contained in:
Gergő Törcsvári
parent
a35eeb7e8b
commit
f92266fcee
8 changed files with 528 additions and 44 deletions
|
|
@ -37,6 +37,14 @@ interface ToolCfg {
|
|||
empty: string;
|
||||
/** Known-volatile top-level tokens to ignore (serializer nondeterminism). */
|
||||
ignoreTokens?: string[];
|
||||
/**
|
||||
* uuids that live in the FILE but are legitimately absent from the items wire,
|
||||
* because they identify the document rather than an item — e.g. a schematic's
|
||||
* root `(uuid …)`, which travels in kdoc_meta/kdoc_layout, not kdoc_items.
|
||||
* Listed explicitly per fixture so `expectWireMatchesFile` can subtract them
|
||||
* without blanket-ignoring "the wire dropped an item", which is a real defect.
|
||||
*/
|
||||
wireOmitsUuids?: string[];
|
||||
}
|
||||
|
||||
type Mod = {
|
||||
|
|
@ -161,6 +169,62 @@ async function roundTrip(
|
|||
return { orig, regen };
|
||||
}
|
||||
|
||||
/**
|
||||
* One boot: the file the tool would SAVE, and the per-item blobs it would put on
|
||||
* the collab WIRE, taken from the same model at the same moment.
|
||||
*/
|
||||
async function fileAndWire(
|
||||
context: BrowserContext,
|
||||
cfg: ToolCfg,
|
||||
): Promise<{ file: string; wire: string }> {
|
||||
const page = await context.newPage();
|
||||
await bootOpen(page, cfg, cfg.fixture, "rt");
|
||||
const file = await saveRead(page, cfg, "orig_dump");
|
||||
const snap = await page.evaluate(() => window.Module.kicadCollabSnapshotItems());
|
||||
await page.close();
|
||||
|
||||
// Splice the blobs into one synthetic document so sexprDiff can index them by
|
||||
// uuid. Non-footprint blobs already arrive wrapped in their own `(kicad_pcb …)`
|
||||
// envelope; nesting is harmless — only uuid-bearing forms are compared, and the
|
||||
// envelope's layers/version carry none.
|
||||
const blobs: string[] = JSON.parse(snap).added.map((w: { sexpr: string }) => w.sexpr);
|
||||
return { file, wire: `(${cfg.tool}_wire ${blobs.join("\n")})` };
|
||||
}
|
||||
|
||||
/**
|
||||
* THE invariant this suite was missing. `roundTrip` compares two FILE saves, so a
|
||||
* wire blob written in a different dialect than the file writer round-trips
|
||||
* perfectly and still corrupts the Y.Doc — which is exactly what happened:
|
||||
*
|
||||
* - pcbnew serialized wire footprints through CLIPBOARD_IO (CTL_FOR_CLIPBOARD),
|
||||
* which unlike CTL_FOR_BOARD emits `(version …) (generator …)
|
||||
* (generator_version …)` INSIDE every `(footprint …)`. Every footprint of
|
||||
* every board drifted, permanently.
|
||||
* - eeschema serialized symbols with `aForClipboard=true`, whose
|
||||
* `MakeRelativeTo(currentSheet)` collapses `(instances … (path "/<sheet>" …))`
|
||||
* to `(path "")` — so materializing the Y.Doc would strip every symbol's sheet
|
||||
* path, reference and unit.
|
||||
*
|
||||
* The Y.Doc is the source of truth for the FILE, so the two must agree token for
|
||||
* token. NOTE: no `ignoreTokens` here on purpose — `generator_version` is exactly
|
||||
* one of the tokens the pcbnew bug leaked, and ignoring it would re-mask it. (The
|
||||
* fixture-vs-build version mismatch that `ignoreTokens` exists for lives on the
|
||||
* ROOT form, which carries no uuid and is therefore never compared.)
|
||||
*/
|
||||
async function expectWireMatchesFile(context: BrowserContext, cfg: ToolCfg): Promise<void> {
|
||||
const { file, wire } = await fileAndWire(context, cfg);
|
||||
const raw = sexprDiff(file, wire);
|
||||
const omitted = new Set(cfg.wireOmitsUuids ?? []);
|
||||
const diff = {
|
||||
...raw,
|
||||
removed: raw.removed.filter((u) => !omitted.has(u)),
|
||||
};
|
||||
expect(
|
||||
diff.added.length === 0 && diff.removed.length === 0 && diff.changed.length === 0,
|
||||
`wire blobs disagree with the file save:\n${JSON.stringify(diff, null, 2)}`,
|
||||
).toBe(true);
|
||||
}
|
||||
|
||||
// ── Fixtures ────────────────────────────────────────────────────────────────
|
||||
|
||||
const PL: ToolCfg = {
|
||||
|
|
@ -215,6 +279,68 @@ const SCH: ToolCfg = {
|
|||
ignoreTokens: ["generator_version"],
|
||||
};
|
||||
|
||||
// A schematic carrying a real SYMBOL with `(instances …)`. The wire-vs-file test
|
||||
// below needs one: the eeschema clipboard dialect rewrote the instance path
|
||||
// relative to the current sheet, collapsing it to `(path "")`. A wire-only
|
||||
// fixture (SCH above) cannot catch that — only symbols have instances.
|
||||
const SCH_SYM: ToolCfg = {
|
||||
...SCH,
|
||||
// The schematic's own root uuid is document identity (kdoc_meta/kdoc_layout),
|
||||
// never an item on the items wire — so the file has it and the wire does not.
|
||||
wireOmitsUuids: ["11111111-1111-1111-1111-111111111111"],
|
||||
fixture: `(kicad_sch
|
||||
(version 20250114)
|
||||
(generator "eeschema")
|
||||
(generator_version "9.0")
|
||||
(uuid "11111111-1111-1111-1111-111111111111")
|
||||
(paper "A4")
|
||||
(lib_symbols
|
||||
(symbol "Device:R"
|
||||
(pin_numbers (hide yes))
|
||||
(pin_names (offset 0))
|
||||
(exclude_from_sim no)
|
||||
(in_bom yes)
|
||||
(on_board yes)
|
||||
(property "Reference" "R" (at 2.032 0 90) (effects (font (size 1.27 1.27))))
|
||||
(property "Value" "R" (at 0 0 90) (effects (font (size 1.27 1.27))))
|
||||
(symbol "R_0_1"
|
||||
(rectangle (start -1.016 -2.54) (end 1.016 2.54)
|
||||
(stroke (width 0.254) (type default)) (fill (type none)))
|
||||
)
|
||||
(symbol "R_1_1"
|
||||
(pin passive line (at 0 3.81 270) (length 1.27)
|
||||
(name "~" (effects (font (size 1.27 1.27))))
|
||||
(number "1" (effects (font (size 1.27 1.27)))))
|
||||
(pin passive line (at 0 -3.81 90) (length 1.27)
|
||||
(name "~" (effects (font (size 1.27 1.27))))
|
||||
(number "2" (effects (font (size 1.27 1.27)))))
|
||||
)
|
||||
)
|
||||
)
|
||||
(symbol
|
||||
(lib_id "Device:R")
|
||||
(at 100 100 0)
|
||||
(unit 1)
|
||||
(exclude_from_sim no)
|
||||
(in_bom yes)
|
||||
(on_board yes)
|
||||
(dnp no)
|
||||
(uuid "33333333-0000-0000-0000-000000000001")
|
||||
(property "Reference" "R1" (at 102 99 0) (effects (font (size 1.27 1.27)) (justify left)))
|
||||
(property "Value" "R" (at 102 101 0) (effects (font (size 1.27 1.27)) (justify left)))
|
||||
(pin "1" (uuid "33333333-0000-0000-0000-0000000000a1"))
|
||||
(pin "2" (uuid "33333333-0000-0000-0000-0000000000a2"))
|
||||
(instances
|
||||
(project "rt"
|
||||
(path "/11111111-1111-1111-1111-111111111111" (reference "R1") (unit 1))
|
||||
)
|
||||
)
|
||||
)
|
||||
(sheet_instances (path "/" (page "1")))
|
||||
)
|
||||
`,
|
||||
};
|
||||
|
||||
const PCB: ToolCfg = {
|
||||
tool: "pcbnew",
|
||||
// pcbnew-collab.html seeds kicad_common.json to skip the first-run setup wizard;
|
||||
|
|
@ -357,6 +483,26 @@ test.describe("round trip: file → yjs → file", () => {
|
|||
expect(hasAbort(testLogger), "no WASM abort").toBe(false);
|
||||
});
|
||||
|
||||
// The wire dialect must BE the file dialect — see expectWireMatchesFile. These
|
||||
// two would both have failed before the CTL_FOR_BOARD / aForClipboard=false fix:
|
||||
// pcbnew on the `(version)(generator)(generator_version)` triple inside every
|
||||
// footprint, eeschema on `(instances … (path ""))`.
|
||||
test("pcbnew wire blobs match the file save token for token", async ({
|
||||
context,
|
||||
testLogger,
|
||||
}) => {
|
||||
await expectWireMatchesFile(context, PCB_FP);
|
||||
expect(hasAbort(testLogger), "no WASM abort").toBe(false);
|
||||
});
|
||||
|
||||
test("eeschema wire blobs keep the symbol's instance path", async ({
|
||||
context,
|
||||
testLogger,
|
||||
}) => {
|
||||
await expectWireMatchesFile(context, SCH_SYM);
|
||||
expect(hasAbort(testLogger), "no WASM abort").toBe(false);
|
||||
});
|
||||
|
||||
// REMAINING KNOWN GAP (ysync 0008 status, known limit 1 — tracked, not a test
|
||||
// bug): pcbnew track/via/zone/text APPLY rides the `(kicad_pcb …)` envelope
|
||||
// parse, the codebase's documented asyncify-fragile path (even a verbatim
|
||||
|
|
|
|||
138
tests/web/fpedit-from-eeschema.spec.ts
Normal file
138
tests/web/fpedit-from-eeschema.spec.ts
Normal file
|
|
@ -0,0 +1,138 @@
|
|||
import { test, expect, type Page } from '@playwright/test';
|
||||
import { clickByTooltip, stableShot, waitForWxApp } from '../e2e/utils/element-tracker';
|
||||
|
||||
/**
|
||||
* Footprint Editor opened FROM a schematic session must show its libraries.
|
||||
*
|
||||
* Regression guard for the ysync-0010 §4 bug: `single_top.cpp` preloads libraries
|
||||
* only for the face the app booted into. Booting `--frame=fpedit` (what
|
||||
* `-/footprint_editor` does, and what every other fp-editor spec exercises)
|
||||
* preloads FACE_PCB and works. But the eeschema toolbar button routes through
|
||||
* `Kiway().Player( FRAME_FOOTPRINT_EDITOR )`, which starts FACE_PCB lazily at
|
||||
* click time and never preloads — so nothing had walked the footprint libraries,
|
||||
* `LIBRARY_MANAGER_ADAPTER::GetLibraryNames()` reported only rows whose status is
|
||||
* LOADED (i.e. none), and `FP_TREE_SYNCHRONIZING_ADAPTER::Sync` skipped
|
||||
* everything. The editor opened with a completely empty library tree.
|
||||
*
|
||||
* Fixed by `AsyncLoad()` + `BlockUntilLoaded()` in
|
||||
* `FOOTPRINT_EDIT_FRAME::initLibraryTree()`.
|
||||
*
|
||||
* Two things this spec must NOT do, both learned the hard way:
|
||||
*
|
||||
* - Don't assert on `document.title`. It stays "… — Schematic Editor" even after
|
||||
* a second editor frame opens (verified with Symbol Editor too), so a title
|
||||
* poll reports failure on a working transition. The wx FRAME LIST is the
|
||||
* signal: `ModEditFrame` appearing IS the Footprint Editor opening.
|
||||
* - Don't use the `demo.kicad_sch` route. It boots its document from the Y.Doc
|
||||
* and needs apps/sync (:3055), which this suite's stack does not run; the doc
|
||||
* stalls at "untitled [Unsaved]" and FACE_PCB never starts. The fileless
|
||||
* `-/eeschema` route needs no sync, and nothing here needs a loaded schematic —
|
||||
* the bug is about which kiface preloaded its libraries.
|
||||
*
|
||||
* The tree assertion is a DELTA, not an absolute count: the schematic editor
|
||||
* already renders its own dataviewitem rows (symbol libs), so "> 0" would pass
|
||||
* even with a totally empty footprint tree. Growth past that baseline is exactly
|
||||
* "the footprint tree got rows".
|
||||
*
|
||||
* Determinism: no blind waits — readiness comes from waitForWxApp, the app's own
|
||||
* `kicadLibs` observable, and polling for the state each step produces.
|
||||
*/
|
||||
|
||||
const SCOPE = 'default';
|
||||
const BOOT_TIMEOUT = 180000;
|
||||
|
||||
/** Names of the live wx top-level frames. */
|
||||
function frameNames(page: Page): Promise<string[]> {
|
||||
return page.evaluate(() =>
|
||||
(window as unknown as { wxElementRegistry: any }).wxElementRegistry
|
||||
.findAll({})
|
||||
.filter((e: any) => /Frame$/.test(e.typeName || ''))
|
||||
.map((e: any) => e.name as string),
|
||||
);
|
||||
}
|
||||
|
||||
/** Rendered library-tree rows across whatever frames are up. */
|
||||
function treeRowCount(page: Page): Promise<number> {
|
||||
return page.evaluate(
|
||||
() =>
|
||||
(window as unknown as { wxElementRegistry: any }).wxElementRegistry.findAllRendered({
|
||||
elementType: 'dataviewitem',
|
||||
}).length,
|
||||
);
|
||||
}
|
||||
|
||||
test.describe('footprint editor reached from eeschema', () => {
|
||||
test.describe.configure({ timeout: 420000 });
|
||||
|
||||
test('opens with a populated library tree', async ({ page }) => {
|
||||
const aborts: string[] = [];
|
||||
page.on('console', (m) => {
|
||||
if (/Aborted\(/i.test(m.text())) aborts.push(m.text());
|
||||
});
|
||||
|
||||
// 1) Boot the SCHEMATIC editor — the face whose preload does not cover footprints.
|
||||
await page.goto(`/${SCOPE}/projects/demo/-/eeschema`);
|
||||
await waitForWxApp(page, { timeout: BOOT_TIMEOUT });
|
||||
await expect
|
||||
.poll(() => page.title(), { message: 'schematic editor up', timeout: BOOT_TIMEOUT })
|
||||
.toMatch(/Schematic Editor/i);
|
||||
|
||||
// The libs bridge must exist before we cross over, or a miss below would be an
|
||||
// app-plumbing failure rather than the kiface-preload bug under test.
|
||||
await page.waitForFunction(
|
||||
() => !!(window as unknown as { kicadLibs?: unknown }).kicadLibs,
|
||||
null,
|
||||
{ timeout: 60000 },
|
||||
);
|
||||
|
||||
expect(await frameNames(page)).toEqual(['SchematicFrame']);
|
||||
|
||||
// Baseline: the schematic editor's OWN tree rows (symbol libs) — see header.
|
||||
// It must be SETTLED before we read it: those rows render asynchronously, and
|
||||
// a baseline sampled too early (0) would weaken the delta below into "> 0",
|
||||
// which the schematic's own rows could satisfy on their own. Two consecutive
|
||||
// equal reads = settled.
|
||||
let baseline = -1;
|
||||
await expect
|
||||
.poll(
|
||||
async () => {
|
||||
const n = await treeRowCount(page);
|
||||
const settled = n === baseline;
|
||||
baseline = n;
|
||||
return settled;
|
||||
},
|
||||
{ message: 'schematic tree row count settled', timeout: 60000, intervals: [1000] },
|
||||
)
|
||||
.toBe(true);
|
||||
|
||||
// 2) Cross to the Footprint Editor the way a user does: the toolbar button
|
||||
// (ACTIONS::showFootprintEditor → Kiway().Player( FRAME_FOOTPRINT_EDITOR )).
|
||||
expect(
|
||||
await clickByTooltip(page, 'Create, delete and edit board footprints', {
|
||||
elementType: 'tool',
|
||||
}),
|
||||
'Footprint Editor toolbar button found and clicked',
|
||||
).toBe(true);
|
||||
|
||||
// FACE_PCB starts lazily here — this is the first time the pcbnew kiface runs.
|
||||
await expect
|
||||
.poll(() => frameNames(page), {
|
||||
message: 'Footprint Editor frame (ModEditFrame) opened',
|
||||
timeout: BOOT_TIMEOUT,
|
||||
})
|
||||
.toContain('ModEditFrame');
|
||||
|
||||
// 3) THE ASSERTION: its library tree is populated. Before the fix the frame
|
||||
// opened exactly like this and the tree stayed empty, so the row count
|
||||
// never moved off the schematic editor's baseline.
|
||||
await expect
|
||||
.poll(() => treeRowCount(page), {
|
||||
message: `footprint library tree rows rendered (baseline was ${baseline})`,
|
||||
timeout: 120000,
|
||||
})
|
||||
.toBeGreaterThan(baseline);
|
||||
|
||||
await stableShot(page, 'fpedit-from-eeschema-tree.png');
|
||||
expect(aborts, 'no WASM abort').toEqual([]);
|
||||
});
|
||||
});
|
||||
112
tests/web/symedit-from-eeschema.spec.ts
Normal file
112
tests/web/symedit-from-eeschema.spec.ts
Normal file
|
|
@ -0,0 +1,112 @@
|
|||
import { test, expect, type Page } from '@playwright/test';
|
||||
import { clickByTooltip, waitForWxApp } from '../e2e/utils/element-tracker';
|
||||
|
||||
/**
|
||||
* Symbol Editor opened from a schematic session must show its libraries.
|
||||
*
|
||||
* Companion to `fpedit-from-eeschema.spec.ts`, and deliberately the WEAKER of the
|
||||
* two: this path is expected to be green already. It is here because
|
||||
* `SYMBOL_EDIT_FRAME::SyncLibraries()` carries the same latent defect shape the
|
||||
* footprint side had —
|
||||
*
|
||||
* SYMBOL_LIBRARY_ADAPTER* adapter = PROJECT_SCH::SymbolLibAdapter( &Prj() );
|
||||
* adapter->BlockUntilLoaded(); // ← no AsyncLoad() first
|
||||
*
|
||||
* — which `FOOTPRINT_LIST_IMPL::ReadFootprintFiles()` explicitly warns against
|
||||
* ("AsyncLoad() must be called before BlockUntilLoaded() to ensure library
|
||||
* loading is started"). `BlockUntilLoaded()` on a face that never preloaded
|
||||
* returns immediately with nothing loaded, and the tree comes up empty.
|
||||
*
|
||||
* It cannot fail TODAY because every route to the Symbol Editor already has
|
||||
* FACE_SCH running: `--frame=symedit` preloads it, and the only UI entry point is
|
||||
* eeschema's own toolbar (`ACTIONS::showSymbolEditor` is appended solely by
|
||||
* toolbars_sch_editor.cpp:201 — nothing in pcbnew/ references it). So this spec
|
||||
* pins the currently-working behaviour; it turns into a real regression guard the
|
||||
* moment a cross-kiface entry point is added.
|
||||
*
|
||||
* Same two traps as the footprint spec: assert on the wx FRAME LIST, not
|
||||
* `document.title` (which stays "… — Schematic Editor" even after a second editor
|
||||
* opens), and use the fileless `-/eeschema` route so no apps/sync is required.
|
||||
*/
|
||||
|
||||
const SCOPE = 'default';
|
||||
const BOOT_TIMEOUT = 180000;
|
||||
|
||||
function frameNames(page: Page): Promise<string[]> {
|
||||
return page.evaluate(() =>
|
||||
(window as unknown as { wxElementRegistry: any }).wxElementRegistry
|
||||
.findAll({})
|
||||
.filter((e: any) => /Frame$/.test(e.typeName || ''))
|
||||
.map((e: any) => e.name as string),
|
||||
);
|
||||
}
|
||||
|
||||
function treeRowCount(page: Page): Promise<number> {
|
||||
return page.evaluate(
|
||||
() =>
|
||||
(window as unknown as { wxElementRegistry: any }).wxElementRegistry.findAllRendered({
|
||||
elementType: 'dataviewitem',
|
||||
}).length,
|
||||
);
|
||||
}
|
||||
|
||||
test.describe('symbol editor reached from eeschema', () => {
|
||||
test.describe.configure({ timeout: 420000 });
|
||||
|
||||
test('opens with a populated library tree', async ({ page }) => {
|
||||
const aborts: string[] = [];
|
||||
page.on('console', (m) => {
|
||||
if (/Aborted\(/i.test(m.text())) aborts.push(m.text());
|
||||
});
|
||||
|
||||
await page.goto(`/${SCOPE}/projects/demo/-/eeschema`);
|
||||
await waitForWxApp(page, { timeout: BOOT_TIMEOUT });
|
||||
await expect
|
||||
.poll(() => page.title(), { message: 'schematic editor up', timeout: BOOT_TIMEOUT })
|
||||
.toMatch(/Schematic Editor/i);
|
||||
await page.waitForFunction(
|
||||
() => !!(window as unknown as { kicadLibs?: unknown }).kicadLibs,
|
||||
null,
|
||||
{ timeout: 60000 },
|
||||
);
|
||||
|
||||
expect(await frameNames(page)).toEqual(['SchematicFrame']);
|
||||
|
||||
// Settle the baseline before reading it — see the footprint spec's header.
|
||||
let baseline = -1;
|
||||
await expect
|
||||
.poll(
|
||||
async () => {
|
||||
const n = await treeRowCount(page);
|
||||
const settled = n === baseline;
|
||||
baseline = n;
|
||||
return settled;
|
||||
},
|
||||
{ message: 'schematic tree row count settled', timeout: 60000, intervals: [1000] },
|
||||
)
|
||||
.toBe(true);
|
||||
|
||||
expect(
|
||||
await clickByTooltip(page, 'Create, delete and edit schematic symbols', {
|
||||
elementType: 'tool',
|
||||
}),
|
||||
'Symbol Editor toolbar button found and clicked',
|
||||
).toBe(true);
|
||||
|
||||
await expect
|
||||
.poll(() => frameNames(page), {
|
||||
message: 'Symbol Editor frame (LibeditFrame) opened',
|
||||
timeout: BOOT_TIMEOUT,
|
||||
})
|
||||
.toContain('LibeditFrame');
|
||||
|
||||
await expect
|
||||
.poll(() => treeRowCount(page), {
|
||||
message: `symbol library tree rows rendered (baseline was ${baseline})`,
|
||||
timeout: 120000,
|
||||
})
|
||||
.toBeGreaterThan(baseline);
|
||||
|
||||
expect(aborts, 'no WASM abort').toEqual([]);
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue