From 0f21a3321607f75917a4001e141f8d1242b3a068 Mon Sep 17 00:00:00 2001 From: Istvan Matejcsok <119620946+matejcsok-ee@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:37:21 +0200 Subject: [PATCH] findings(E-5,E-6,E-7,E-8,E-9): close the test-coverage gaps + kicad ifdef bump MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverage audit found three fixes without a test that could catch their reversion: - E-6 ack-on-enqueue: new unit case "acks a pre-handler queued frame at enqueue so the transport window never starves" (ngspice-service.test.ts; false-green audited — fails with the ack removed, exactly one ack per frame, none repeated on drain). - E-5 / E-8 wiring / E-9 / E-7 shape: new source-contract tripwire tests/tools/findings-e-source-contract.ts (npm run findings-e:contract) — the codex-thread contract style for C++/EM_JS code that cannot be behaviorally unit-tested without a wasm build. Asserts: the module identity stamp + self-disarm and ABSENCE of the install-once presence guard (E-5); all four completion sites use runWaitCompletion, no stub resolves a wait directly, token<=0 bails, the shim exports the gate surface (E-8); the run-generation mechanism present with its behavioral drops confined to the wasm build (E-7); the identity-checked destructor unregistration (E-9). Also bumps kicad to the E-7 ifdef-confinement commit. Co-Authored-By: Claude Fable 5 --- kicad | 2 +- tests/package.json | 3 +- tests/tools/findings-e-source-contract.ts | 85 +++++++++++++++++++ .../src/wasm/ngspice-service.test.ts | 40 +++++++++ 4 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 tests/tools/findings-e-source-contract.ts diff --git a/kicad b/kicad index 1c67b78..2306829 160000 --- a/kicad +++ b/kicad @@ -1 +1 @@ -Subproject commit 1c67b78fa43052e8ad730fc6c91a219ce1f3a2af +Subproject commit 230682955a2873ade2273875659a630311982abb diff --git a/tests/package.json b/tests/package.json index 020030e..714ab87 100644 --- a/tests/package.json +++ b/tests/package.json @@ -30,7 +30,8 @@ "3d:review": "tsx tools/screenshots/compare-dirs.ts --old 3d-regression/baseline --new 3d-regression/output/webgl --out 3d-regression/output/diff/parity-review --floors 3d-regression/floors.json --level webgl-vs-native --label 3d-parity --artifacts always", "3d:test:webgl": "playwright test --project=wx-chromium e2e/3d-webgl.spec.ts", "tools:contract": "tsx tools/cli-contract.ts", - "ngspice:worker-batch": "tsx tools/ngspice-worker-batch-unit.ts" + "ngspice:worker-batch": "tsx tools/ngspice-worker-batch-unit.ts", + "findings-e:contract": "tsx tools/findings-e-source-contract.ts" }, "devDependencies": { "@playwright/test": "^1.62.1", diff --git a/tests/tools/findings-e-source-contract.ts b/tests/tools/findings-e-source-contract.ts new file mode 100644 index 0000000..161437a --- /dev/null +++ b/tests/tools/findings-e-source-contract.ts @@ -0,0 +1,85 @@ +/** + * Source-contract tripwire for the findings group E fixes that live in C++ + * (EM_JS bridges and KiCad simulator code) and therefore cannot be + * behaviorally unit-tested without a full wasm build. Same style as the codex + * thread's contract tools: read the sources, assert the load-bearing tokens + * are present (and the reverted shapes absent), fail loudly with the finding + * ID. Run: npm run findings-e:contract + */ +import { strict as assert } from "node:assert"; +import { readFileSync } from "node:fs"; +import * as path from "node:path"; +import { fileURLToPath } from "node:url"; + +const here = path.dirname(fileURLToPath(import.meta.url)); +const repo = path.resolve(here, "../.."); +const read = (rel: string) => readFileSync(path.join(repo, rel), "utf8"); + +const sharedspice = read("wasm/stubs/sharedspice_client.cpp"); +const exporterStub = read("wasm/stubs/exporter_step_stub.cpp"); +const oceStub = read("wasm/stubs/oce_plugin_stub.cpp"); +const simFrame = read("kicad/eeschema/sim/simulator_frame.cpp"); +const ngspiceCpp = read("kicad/eeschema/sim/ngspice.cpp"); +const shim = read("scripts/common/shims/jspi-scheduler.js"); + +// --- E-5: ngspice event handler bound to exact module identity -------------- +assert.ok(sharedspice.includes("const installingModule = Module"), + "E-5: js_ngspice_install_events must capture the installing module"); +assert.ok((sharedspice.match(/__pcbjamNgspiceOwnerModule/g) ?? []).length >= 2, + "E-5: the handler must be stamped AND compared by owner module identity"); +assert.ok(sharedspice.includes("globalThis.__ngspiceOnEvent !== handler"), + "E-5: a superseded handler must disarm itself"); +assert.ok(!/if\(\s*globalThis\.__ngspiceOnEvent\s*\)/.test(sharedspice), + "E-5 REGRESSION: the install-once presence guard is back — presence is not identity"); +assert.ok(sharedspice.includes("canTouchNative"), + "E-5/E-8: event dispatch must check the scheduler liveness gate"); + +// --- E-8: all four completion sites route native work through the gate ------ +for (const [name, src, site] of [ + ["exporter_step_stub.cpp", exporterStub, "'OCC export completion'"], + ["oce_plugin_stub.cpp", oceStub, "'OCC model completion'"], + ["sharedspice_client.cpp", sharedspice, "'ngspice request completion'"], + ["sharedspice_client.cpp", sharedspice, "'ngspice vector completion'"], +] as const) { + assert.ok(src.includes(`runWaitCompletion( ${site}`), + `E-8: ${name} must run its ${site} through runWaitCompletion`); +} +for (const [name, src] of [ + ["exporter_step_stub.cpp", exporterStub], + ["oce_plugin_stub.cpp", oceStub], + ["sharedspice_client.cpp", sharedspice], +] as const) { + assert.ok(!/__wxScheduler\.resolveWait\(/.test(src), + `E-8 REGRESSION: ${name} resolves a wait directly, bypassing the admission gate`); + assert.ok(/if\(\s*token\s*<=\s*0\s*\)/.test(src), + `E-8: ${name} must bail when wxWasmBeginWait refuses the token`); +} +for (const symbol of ["runWaitCompletion", "_terminalizeNativeTrap", + "canTouchNative", "beginWaitRefused"]) { + assert.ok(shim.includes(symbol), + `E-8: jspi-scheduler.js must provide ${symbol}`); +} + +// --- E-7: per-session run generation, behavioral drops wasm-only ------------ +for (const token of ["s_nextSimRunGeneration", "allocateSimRunGeneration", + "SetExtraLong", "m_lastAppliedSimRunGeneration"]) { + assert.ok(simFrame.includes(token), + `E-7: simulator_frame.cpp must carry the run-generation mechanism (${token})`); +} +assert.equal( + (simFrame.match(/Generation acceptance is confined to the wasm build/g) ?? []).length, 2, + "E-7: both handler acceptance guards must be confined to the wasm build"); +assert.ok(simFrame.includes("drop is confined to the wasm build"), + "E-7: the unowned-event drop must be confined to the wasm build"); +assert.ok(simFrame.includes("Wasm-only, findings E-7"), + "E-7: the post-wxYield re-check must be confined to the wasm build"); + +// --- E-9: destructor unregisters the sharedspice callbacks ------------------ +assert.ok(/#ifdef __EMSCRIPTEN__[\s\S]{0,400}pcbjam_ngspice_reset_callbacks\( this \)/ + .test(ngspiceCpp), + "E-9: ~NGSPICE must call pcbjam_ngspice_reset_callbacks(this) under __EMSCRIPTEN__"); +assert.ok(/pcbjam_ngspice_reset_callbacks\( void\* aUser \)[\s\S]{0,200}s_user != aUser/ + .test(sharedspice), + "E-9: the reset must be identity-checked so a stale destructor cannot clear a successor"); + +console.log("findings-e-source-contract: all green"); diff --git a/web/standalone/src/wasm/ngspice-service.test.ts b/web/standalone/src/wasm/ngspice-service.test.ts index f81a1fb..3fe2e0a 100644 --- a/web/standalone/src/wasm/ngspice-service.test.ts +++ b/web/standalone/src/wasm/ngspice-service.test.ts @@ -414,6 +414,46 @@ describe("ngspice service worker lifetime", () => { expect(events).toEqual(["new"]); }); + it("acks a pre-handler queued frame at enqueue so the transport window never starves", async () => { + // Pin for the E-6 refinement: placing a frame in the bounded mirror queue + // IS taking ownership — without the enqueue-time ack, a stream that starts + // before the C++ handler installs pins the worker's 64-frame credit + // window open forever (observed as the ngspice-probe bg_halt starvation). + const first = await readyRequest(0); + expect(globalThis.__ngspiceOnEvent).toBeUndefined(); + + first.worker.emitMessage({ + evt: { kind: "char", lines: ["early"] }, + eventSequence: 1, + eventBytes: 33, + }); + await vi.waitFor(() => + expect(first.worker.postMessage).toHaveBeenCalledWith({ + eventAck: { sequence: 1, bytes: 33 }, + })); + + // Once the handler installs, the queued frame is dispatched WITHOUT a + // second ack; only the new frame earns one. + const events: string[] = []; + globalThis.__ngspiceOnEvent = (event) => events.push(event.lines?.[0] ?? event.kind); + first.worker.emitMessage({ + evt: { kind: "char", lines: ["late"] }, + eventSequence: 2, + eventBytes: 32, + }); + await vi.waitFor(() => expect(events).toEqual(["early", "late"])); + const acks = first.worker.postMessage.mock.calls + .map((call) => (call[0] as { eventAck?: { sequence: number; bytes: number } }).eventAck) + .filter(Boolean); + expect(acks, "exactly one ack per frame, none repeated on drain").toEqual([ + { sequence: 1, bytes: 33 }, + { sequence: 2, bytes: 32 }, + ]); + + first.worker.emitMessage({ id: first.id, res: { ret: 0 } }); + await expect(first.request).resolves.toEqual({ ret: 0 }); + }); + it("retires a worker whose bounded event stream reports a fatal line", async () => { const first = await readyRequest(0, commandRequest("oversize output")); first.worker.emitMessage({