diff --git a/web/EDIWorkbench.html b/web/EDIWorkbench.html index c60fddf..5e9b3d6 100644 --- a/web/EDIWorkbench.html +++ b/web/EDIWorkbench.html @@ -2238,8 +2238,11 @@

Using it with real data

const shifted = shiftDate8(value, this.days); if (shifted === null) return; // malformed date; skip rather than fail the file + // Copied before the write, not after: processText builds its change list + // from these marks, so this is the only record of the original date. + const before = { id: seg.id, elements: seg.elements.slice() }; seg.elements[2] = shifted; - marks.push({ segment: i, elements: [2] }); + marks.push({ segment: i, elements: [2], before }); }); return { segments: marks.length, occurrences: marks.length, marks }; @@ -2315,14 +2318,18 @@

Using it with real data

if (this.segmentIds && !this.segmentIds.has(seg.id.toUpperCase())) return; const hits = []; + let before = null; seg.elements.forEach((value, j) => { const [next, count] = this.replaceIn(value); if (!count) return; + // On the first hit only, and before the write: one copy per changed + // segment is what replaced processText's clone of every segment. + if (!before) before = { id: seg.id, elements: seg.elements.slice() }; seg.elements[j] = next; occurrences += count; hits.push(j); }); - if (hits.length) marks.push({ segment: i, elements: hits }); + if (hits.length) marks.push({ segment: i, elements: hits, before }); }); return { segments: marks.length, occurrences, marks }; @@ -2384,10 +2391,12 @@

Using it with real data

/* ---- engine -------------------------------------------------------- */ -function snapshot(segments) { - return segments.map((seg) => ({ id: seg.id, elements: seg.elements.slice() })); -} - +// There was a snapshot(segments) here that cloned every segment in the +// document before the rules ran, so processText could diff against it +// afterwards. It was a second full copy of the working set, built to find +// changes the rules already report: each rule now copies a segment on its +// first write to it and carries that copy in the mark. sameSegment survives +// because the marks say what a rule touched, not what it managed to change. function sameSegment(a, b) { if (a.id !== b.id || a.elements.length !== b.elements.length) return false; return a.elements.every((v, i) => v === b.elements[i]); @@ -2402,10 +2411,19 @@

Using it with real data

if (typeof rule.check === "function") rule.check(doc); } - const before = snapshot(doc.segments); const countBefore = doc.segments.length; const summary = []; const ruleKindsBySegment = new Map(); + // Segment index -> the segment as it was before any rule touched it. Each + // rule copies a segment on its first write to it and carries that copy in + // the mark, so only segments that actually change are ever copied. This + // replaced a clone of the entire document taken up front, which on a large + // file was a second full working set built to find a handful of changes. + // + // First writer wins. Two rules can hit one segment, and the second one's + // copy has the first one's edit already in it -- the change list has to + // show what arrived, not what the second rule happened to find. + const beforeBySegment = new Map(); for (const rule of rules) { const result = rule.apply(doc.segments); @@ -2417,6 +2435,9 @@

Using it with real data

occurrences: result.occurrences, }); for (const mark of result.marks) { + if (mark.before && !beforeBySegment.has(mark.segment)) { + beforeBySegment.set(mark.segment, mark.before); + } let entry = ruleKindsBySegment.get(mark.segment); if (!entry) { entry = { kinds: [], elements: new Map() }; ruleKindsBySegment.set(mark.segment, entry); } if (!entry.kinds.includes(rule.kind)) entry.kinds.push(rule.kind); @@ -2432,25 +2453,44 @@

Using it with real data

// // The change list below pairs segments by index, which only holds while // the count is unchanged; a rule that changes it will need that revisited. + // Note the counts this repairs are not themselves listed as changes: the + // list is built from what the rules marked, and this repair is not a rule. + // That is moot while no rule changes the count, and is the second thing to + // revisit on the day one does. const countsRepaired = doc.segments.length === countBefore ? 0 : repairControlCounts(doc); + // Only the segments a rule marked can appear here, so this walks those + // rather than sweeping the whole document. sameSegment stays as a filter + // over them: a rule may mark a write that turned out to change nothing -- + // StringReplaceRule counts a match even when the replacement equals it -- + // and such a segment must not reach the Changes tab as a change with an + // identical before and after. What the filter no longer catches is the + // opposite case, a rule editing a segment without marking it; that is + // covered directly now, by the mark audits in parity.mjs and deid.mjs. const changes = []; - doc.segments.forEach((seg, i) => { - if (sameSegment(before[i], seg)) return; + for (const i of [...beforeBySegment.keys()].sort((a, b) => a - b)) { + const was = beforeBySegment.get(i); + const seg = doc.segments[i]; + if (sameSegment(was, seg)) continue; const entry = ruleKindsBySegment.get(i) || { kinds: [], elements: new Map() }; changes.push({ index: i, - before: before[i], + before: was, after: { id: seg.id, elements: seg.elements.slice() }, kinds: entry.kinds, elementKinds: entry.elements, }); - }); + } return { output: serialize(doc), + // The mutated document itself, so the UI can browse the result without + // parsing `output` back into a second copy of what is already here. + // Callers must treat it as owned by the result: the segment objects are + // live, not clones, and `changes` holds the only record of what they were. + doc, segmentCount: doc.segments.length, delimiters: { element: doc.elementSep, @@ -4137,6 +4177,7 @@

Using it with real data

apply(segments) { const marks = new Map(); // segment index -> Set of element indices + const befores = new Map(); // segment index -> the segment as it arrived const mark = (i, el) => { let set = marks.get(i); if (!set) { set = new Set(); marks.set(i, set); } @@ -4144,6 +4185,13 @@

Using it with real data

}; const put = (i, el, value) => { if (segments[i].elements[el] === value) return; // no-op: don't claim a change + // Copy the segment before the first write lands on it, and only then -- + // processText no longer clones every segment up front, so this is the + // only chance to record what a rewritten value used to be. It has to + // happen here rather than in mark(), which runs after the assignment. + if (!befores.has(i)) { + befores.set(i, { id: segments[i].id, elements: segments[i].elements.slice() }); + } segments[i].elements[el] = value; mark(i, el); }; @@ -4284,7 +4332,7 @@

Using it with real data

const out = []; for (const [segment, elements] of [...marks].sort((a, b) => a[0] - b[0])) { occurrences += elements.size; - out.push({ segment, elements: [...elements] }); + out.push({ segment, elements: [...elements], before: befores.get(segment) }); } return { segments: out.length, occurrences, marks: out }; } @@ -6086,7 +6134,13 @@

Using it with real data

`
${t.value.toLocaleString()}${t.label}
` ).join(""); - state.doc = parse(result.output); // browse the edited document, not the original + // Browse the edited document, not the original. processText already holds + // the mutated document, so this takes it rather than re-parsing the output + // string back into a second full set of segment objects -- which cost a + // whole extra parse and about a third of peak heap for a document it had + // in hand the entire time. The segments here are the ones the rules + // mutated, which is exactly what the re-parse was reconstructing. + state.doc = result.doc; state.outline = buildOutline(state.doc.segments); state.claims = null; // stale the moment the document changes; rebuilt on demand state.focus = null; diff --git a/web/PERFORMANCE.md b/web/PERFORMANCE.md index 9b67ed3..fde66c7 100644 --- a/web/PERFORMANCE.md +++ b/web/PERFORMANCE.md @@ -1,15 +1,16 @@ # EDI Workbench (browser) — performance findings -**Measured 2026-08-03.** Status: findings recorded, no fixes applied yet. +**First measured 2026-08-03. Re-measured 2026-09-07** after backlog items 2 and 3 +landed. Status: those two are done; the rest of the backlog stands. ## Recommendation | Range | Verdict | |---|---| -| **up to 25 MB** | Comfortable. Sub-second load, ~1.3 s to run rules. | -| **25–50 MB** | Usable. Noticeable freeze (1–3 s) but nothing breaks. | -| **50–100 MB** | Degraded. 3–7 s frozen tab; Chrome may offer to kill the page. | -| **100–125 MB** | Hard edge. 15 s and ~2 GB heap at 125 MB. | +| **up to 25 MB** | Comfortable. Sub-second load, under a second to run rules. | +| **25–50 MB** | Usable. Brief freeze (1–2 s) but nothing breaks. | +| **50–100 MB** | Degraded. 2–4 s frozen tab; Chrome may offer to kill the page. | +| **100–125 MB** | Hard edge. 6 s and ~2.9 GB heap at 125 MB. | | **150 MB+** | Out of memory on a 4 GB tab. | For scale, 25 MB of 837P is roughly 30,000 claims. Most real batches sit well under @@ -21,21 +22,38 @@ about headroom and about not freezing the tab, not about unblocking normal use. Synthetic 837P, ~41k segments per MB, ~42 bytes per segment (close to real professional claims). Two rules active: service date shift + find/replace. -| File | Segments | Load & browse | Heap | Run rules | Peak heap | +| File | Segments | Load & browse | Heap | Run rules | Heap after run | |---:|---:|---:|---:|---:|---:| -| 1 MB | 41k | 17 ms | 8 MB | 57 ms | 15 MB | -| 5 MB | 206k | 83 ms | 39 MB | 230 ms | 74 MB | -| 10 MB | 410k | 144 ms | 79 MB | 446 ms | 148 MB | -| 25 MB | 1.0M | 351 ms | 195 MB | 1.3 s | 367 MB | -| 50 MB | 2.0M | 642 ms | 392 MB | 2.6 s | 736 MB | -| 64 MB | 2.6M | 855 ms | 495 MB | 3.1 s | 934 MB | -| 80 MB | 3.3M | 1.0 s | 624 MB | 5.0 s | 1.17 GB | -| 100 MB | 4.2M | 1.4 s | 883 MB | 6.9 s | 1.58 GB | -| 125 MB | 5.2M | 1.8 s | 1.11 GB | 15.3 s | 1.99 GB | +| 1 MB | 42k | 15 ms | 8 MB | 50 ms | 22 MB | +| 5 MB | 208k | 76 ms | 43 MB | 174 ms | 115 MB | +| 10 MB | 415k | 133 ms | 88 MB | 332 ms | 232 MB | +| 25 MB | 1.0M | 366 ms | 221 MB | 862 ms | 579 MB | +| 50 MB | 2.1M | 769 ms | 446 MB | 1.8 s | 1.17 GB | +| 64 MB | 2.7M | 953 ms | 565 MB | 2.2 s | 1.48 GB | +| 80 MB | 3.3M | 1.2 s | 712 MB | 3.3 s | 1.86 GB | +| 100 MB | 4.2M | 1.4 s | 882 MB | 4.0 s | 2.31 GB | +| 125 MB | 5.2M | 2.0 s | 1.11 GB | 6.2 s | 2.90 GB | | 150 MB | — | — | — | out of memory | — | Re-run with `node web/bench/bench.mjs` (see that file's header for options). +### Two corrections to the first round of numbers + +**The heap column was understating the app, and is now larger.** It is not a +regression. The benchmark used to throw away its copy of the browsable document +the instant it built it, while the app keeps that document in `state.doc` for as +long as the file is open. So the old column measured a state the app is never in. +The 125 MB row reading 1.99 GB before and 2.90 GB now is that correction, not a +change in what the code does. + +**It is also "heap after the run", not peak heap, and used to be labelled peak.** +The measurement is taken after a forced GC, so it reports what is still reachable +when the run finishes — it cannot see a copy that is built and discarded during +the run. That is exactly what made the original predictions for items 2 and 3 +wrong: both removed transient copies, so both moved this column barely at all +while cutting the time substantially. Bear it in mind before predicting a memory +saving from a copy that does not outlive `processText`. + ### Caveats on these numbers - **Measured in Node, not a browser.** Same V8, so the arithmetic transfers, but a @@ -49,28 +67,49 @@ Re-run with `node web/bench/bench.mjs` (see that file's header for options). ## Why memory is the constraint, not CPU -Parsing costs about **8x the file size** in heap; running rules about **16x**. CPU is -comparatively cheap — the memory wall arrives first. The 16x is five full-size copies -of the same data: +Parsing costs about **8x the file size** in heap; running rules about **23x** of what +survives the run. CPU is comparatively cheap — the memory wall arrives first. What is +still reachable when a run finishes is three full-size copies of the same data: | # | Copy | Where | |---|---|---| -| 1 | Parsed segment objects | `parse()` — unavoidable, this is the working set | -| 2 | Before-snapshot clone of every segment | `processText()` → `snapshot(doc.segments)` | -| 3 | Serialized output string | `serialize(doc)` | -| 4 | `changes[]` holding before + after per changed segment | `processText()` | -| 5 | Re-parse of the output so the UI can browse the edited doc | `render()` → `state.doc = parse(result.output)` | +| 1 | Parsed segment objects, as the rules left them | `parse()` — unavoidable, this is the working set, and it is what the UI browses | +| 2 | Serialized output string | `serialize(doc)` | +| 3 | `changes[]` holding before + after per changed segment | `processText()` | -Copies 2 and 5 are pure waste and are the cheapest to remove. +Two more used to be built and thrown away during the run — a clone of every segment +taken before the rules ran, and a re-parse of the output so the UI had something to +browse. Both are gone; see below. Neither was reachable at the end of a run, which is +why removing them cut the time without moving the heap column much. + +Copy 3 is the one left with any slack in it: it holds a before and an after for every +changed segment, so a rule that rewrites most of a file approaches a second working +set. It is also the only record of what a run changed, so it cannot simply be dropped. ## Already handled -Not on the backlog — these were dealt with when the browser was built: +Not on the backlog — these were dealt with when the browser was built, or have since +been done: - **Document list is virtualized.** Only rows near the fold exist in the DOM, so rendering is O(viewport) regardless of file size. Rendering is not a cost factor. - **Changes tab caps at 500 rendered segments**, output preview at 400k characters. Both note the truncation; the download is always complete. +- **The output is no longer re-parsed** so the UI can browse it (was backlog item 2). + `processText()` returns the document it already mutated and `render()` takes it. + Cut run time by 26–37%. +- **Only changed segments are copied** (was backlog item 3). Each rule copies a + segment on its first write to it and carries the copy in the mark it already + reported, replacing a clone of the whole document taken up front. Cut run time by a + further 26% at 100 MB and 36% at 125 MB — the gain grows with the file, because the + extra copy was crowding the heap limit and paying for it in GC. + + This moved the correctness of the change list onto the accuracy of each rule's + `marks`. That is not free, and it is why `web/tests/_marks.mjs` exists: it audits, + for every rule, that nothing changes without being marked. `sameSegment` was kept + as a filter over marked segments — a rule marks what it *touched*, not what it + managed to change, and `StringReplaceRule` counts a match even when the replacement + equals it. ## Improvement backlog @@ -79,8 +118,10 @@ In the order I'd do them. ### 1. Get processing off the main thread, and show progress **Problem:** `run()` calls `processText()` synchronously, so the tab is frozen with no -feedback for the entire run — 2.6 s at 50 MB, 6.9 s at 100 MB. This is the worst part -of the current behaviour and it is a UX problem, not a throughput one. +feedback for the entire run — 1.8 s at 50 MB, 4.0 s at 100 MB. Items 2 and 3 roughly +halved those, which makes this the worst remaining part of the behaviour by a wide +margin. It is a UX problem, not a throughput one, and no further copy-removal fixes +it: a 4 s freeze with no progress indication reads as a hung tab whatever the number. **Fix:** move the engine into a Web Worker. The engine block in `EDIWorkbench.html` (between the `engine:start` and `engine:end` markers) has no DOM access precisely so @@ -93,52 +134,51 @@ even before the worker lands. **Expected gain:** no change to the ceiling; the tab stays responsive throughout. -### 2. Stop re-parsing the output - -**Problem:** `render()` does `state.doc = parse(result.output)` so the document -browser shows edited values. That's a second full parse and a second full set of -segment objects. - -**Fix:** `processText()` already holds the mutated document. Return it, and have -`render()` use it directly. Segment indices already align (no rule adds or removes -segments), which is the assumption `state.touched` relies on today — worth an -explicit comment when this changes. +### ~~2. Stop re-parsing the output~~ — done 2026-09-07 -**Expected gain:** removes copy #5, roughly a third of peak heap. +### ~~3. Snapshot only what changes~~ — done 2026-09-07 -### 3. Snapshot only what changes +Both are described under **Already handled** above, with what they actually bought. -**Problem:** `processText()` clones *every* segment up front to diff against. On a -5M-segment file that's a second full working set to detect ~1M changes. - -**Fix:** rules already return `marks` naming the segments and elements they touched. -Capture the before-values inside the rule at mutation time and carry them in the -mark, then drop `snapshot()` and the full comparison sweep entirely. - -**Watch:** the current diff is what makes the change list correct even if a rule -reports its marks inaccurately. Removing it moves that trust to the rules, so the -parity tests need to cover both rules' mark accuracy directly. - -**Expected gain:** removes copy #2, plus the O(n) comparison sweep. +Worth recording, because it is the lesson rather than the result: **both were +predicted as memory savings and both delivered time savings instead.** Item 2 was +expected to remove "roughly a third of peak heap" and moved the heap column by about +2%. The reasoning error was treating five copies as five things held at once, when +the two being removed were each built and discarded inside a single run — so at no +point did the file exist five times over. Before predicting a memory win from +removing a copy, check whether anything still refers to it when the run ends. ### 4. Guard rail for oversized files -After 1–3, put a soft warning in front of files over ~100 MB: state the expected time +After 1, put a soft warning in front of files over ~100 MB: state the expected time and let the user proceed. Better than an unexplained multi-second freeze or an out-of-memory crash. ### 5. Only if still needed: streaming parse Chunked parsing with progress reporting would push past 125 MB, but it's a large -change to a currently simple and well-tested parser. Don't start here — 1–3 should -move the ceiling far enough that this stays unnecessary. +change to a currently simple and well-tested parser. + +Note that items 2 and 3 **did not move the ceiling at all** — 150 MB was out of +memory before them and still is. They removed transient copies, and the ceiling is +set by what a run has to hold simultaneously at its worst moment, which is dominated +by the working set, the output string and the change list. If the ceiling is the +thing that actually needs to move, this item is what moves it, and nothing cheaper +will. Don't start here anyway: 125 MB is far past any real batch. ## How to verify an improvement -1. `node web/bench/bench.mjs` before and after; compare peak heap and run time. -2. Re-run the correctness suites — the engine must stay byte-for-byte identical to - the Python `edi_engine` output. Items 2 and 3 both touch `processText()`, which is - exactly what those tests pin down. +1. `node web/bench/bench.mjs` before and after; compare run time and heap. **Measure + the same tree with only the change absent** — not against the table above, which + was taken on a different machine and, before 2026-09-07, with a benchmark that + modelled the app's retained memory incorrectly. Stashing the change and re-running + is the cheap way to get a comparison worth quoting. +2. Remember the heap column is measured after a forced GC. It will not show you a + copy that dies before the run returns, however large that copy was. +3. Re-run the correctness suites — the engine must stay byte-for-byte identical to + the Python `edi_engine` output. Anything touching `processText()` is exactly what + those tests pin down, and anything touching how rules report `marks` is now pinned + by the audits in `_marks.mjs`. ## Test harnesses @@ -148,3 +188,6 @@ claim index and CSV export, control counts and claim balance, de-identification, virtual-list windowing math, the load path against a throwaway DOM, and the static self-containment lint); `node web/bench/bench.mjs` runs the benchmark that produced the numbers above. + +`web/tests/_marks.mjs` is not a suite of its own — it is the mark audit, called from +`parity.mjs` and `deid.mjs`, which are where each rule is already wired up. diff --git a/web/bench/bench.mjs b/web/bench/bench.mjs index 40b7aa4..04f3827 100644 --- a/web/bench/bench.mjs +++ b/web/bench/bench.mjs @@ -12,7 +12,7 @@ * actually does and they have very different budgets: * * load -- parse only, i.e. open a file and browse it - * run -- snapshot + rules + diff + serialize + the UI's re-parse + * run -- snapshot + rules + diff + serialize * * Numbers come from Node, not a browser. Same V8, so the arithmetic * transfers; a real tab additionally pays for file.text() and the download @@ -143,7 +143,10 @@ const [runTime] = time(() => { new Dtp472ServiceLineShiftRule(1), new StringReplaceRule({ find: "SPRINGFIELD", replace: "METROPOLIS" }), ]); - parse(result.output); // the UI re-parses so you can browse the edited doc; count it + // The UI used to re-parse result.output here so you could browse the edited + // document, and this benchmark counted that parse because it was real work + // a user paid for. processText now returns the mutated document directly, + // so there is nothing to count -- result.doc is what the UI browses. return result; }); const runHeap = heap() - base; diff --git a/web/tests/_marks.mjs b/web/tests/_marks.mjs new file mode 100644 index 0000000..3a46d05 --- /dev/null +++ b/web/tests/_marks.mjs @@ -0,0 +1,63 @@ +// Audits whether a rule's marks account for every change it actually makes. +// +// Not a suite of its own -- it is called from the suites that already have a +// rule wired up: parity.mjs for the two engine rules, deid.mjs for +// DeidentifyRule. Putting the audit next to each rule's other tests beats a +// tenth suite that would have to re-lift the whole engine to reach them. +// +// Why this exists at all: processText used to build its change list by cloning +// every segment before the rules ran and comparing afterwards. The clone was a +// full second copy of the working set, so it was dropped -- the change list is +// now built from what the rules say they touched. That moves the correctness of +// the Changes tab, and of the Limited Data Set's own record of what it +// rewrote, onto the accuracy of `marks`. Nothing checked that before; this does. +// +// Two directions, and they are not equally serious: +// +// unmarked a rule changed something and did not say so. The change list +// misses it. Nothing downstream can recover it, because the +// before-value is gone the moment the rule overwrites it. This +// must always be empty. +// overclaimed a rule marked something that did not actually change. Harmless +// and expected -- StringReplaceRule counts a match even when the +// replacement equals what it matched. processText filters these +// with sameSegment, which is exactly why that filter was kept +// when the full snapshot was dropped. + +const key = (segment, element) => `${segment}:${element}`; + +export function auditMarks(parse, rule, raw) { + const doc = parse(raw); + if (typeof rule.check === "function") rule.check(doc); + + // The audit's own before-copy. This is the thing processText no longer does; + // a test may pay for it because a fixture is small and correctness is the + // point, which is not true of a 50 MB file in a browser tab. + const before = doc.segments.map((seg) => ({ id: seg.id, elements: seg.elements.slice() })); + const result = rule.apply(doc.segments); + + const changed = new Set(); + doc.segments.forEach((seg, i) => { + const was = before[i].elements; + const now = seg.elements; + // Compare across the longer of the two: a rule that appends or truncates + // elements has changed the segment just as much as one that overwrites. + for (let j = 0; j < Math.max(was.length, now.length); j++) { + if (was[j] !== now[j]) changed.add(key(i, j)); + } + if (before[i].id !== seg.id) changed.add(key(i, "id")); + }); + + const claimed = new Set(); + for (const mark of result.marks) { + for (const element of mark.elements) claimed.add(key(mark.segment, element)); + } + + const sorted = (set) => [...set].sort(); + return { + result, + changed: sorted(changed), + unmarked: sorted([...changed].filter((k) => !claimed.has(k))), + overclaimed: sorted([...claimed].filter((k) => !changed.has(k))), + }; +} diff --git a/web/tests/deid.mjs b/web/tests/deid.mjs index 640ac25..5605a27 100644 --- a/web/tests/deid.mjs +++ b/web/tests/deid.mjs @@ -15,6 +15,7 @@ import { readFileSync, readdirSync } from "node:fs"; import { join } from "node:path"; import { APP, PACDR_FIXTURE, X221_FIXTURE, SAMPLES_DIR } from "./paths.mjs"; +import { auditMarks } from "./_marks.mjs"; const html = readFileSync(APP, "utf8"); @@ -566,5 +567,24 @@ check("the output is byte-identical to the input", m.serialize(x221.doc), x221Ra check("the fixture still validates cleanly after the run", m.validateDocument(x221.doc).map((f) => f.title), []); +console.log("\n[18] every rewrite is marked"); +// processText builds its change list from these marks now, rather than from a +// clone of every segment taken before the rules ran, so a rewrite this rule +// forgets to mark is a rewrite nothing downstream can report. That matters +// more here than for the other two rules: the Changes tab is the only record +// of what a Limited Data Set replaced, and the mapping is thrown away when the +// run ends. See _marks.mjs. +for (const [name, source] of [ + ["the 837P fixture", RAW], + ["the PACDR fixture", readFileSync(PACDR_FIXTURE, "utf8")], +]) { + const audit = auditMarks(m.parse, new m.DeidentifyRule(23), source); + check(`${name}: the rule changes it`, audit.changed.length > 0, true); + check(`${name}: no rewrite goes unmarked`, audit.unmarked, []); + // put() already refuses to mark a write that would not change the value, so + // unlike StringReplaceRule this rule should claim nothing it did not do. + check(`${name}: and none is claimed that did not happen`, audit.overclaimed, []); +} + console.log(`\n${pass} passed, ${fail} failed`); process.exit(fail ? 1 : 0); diff --git a/web/tests/parity.mjs b/web/tests/parity.mjs index 5e49337..0c681fa 100644 --- a/web/tests/parity.mjs +++ b/web/tests/parity.mjs @@ -3,6 +3,7 @@ import { readFileSync, rmSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { APP, FIXTURE, MAKE_REF, REF_DIR } from "./paths.mjs"; +import { auditMarks } from "./_marks.mjs"; const html = readFileSync(APP, "utf8"); @@ -247,5 +248,53 @@ check("4010 file still round trips", serialize(parse(v4010)) === v4010, true); check("5010 with a 'U' placeholder reports none", parse(raw.replace("*^*00501*", "*U*00501*")).repetitionSep, null); +console.log("\n[13] the engine rules mark every change they make"); +// processText builds its change list from these marks now, instead of from a +// clone of every segment taken before the rules ran. A change a rule makes +// without marking it is invisible from here on -- the before-value is gone the +// instant the rule overwrites it. See _marks.mjs. +for (const [name, rule] of [ + ["shift service dates", new Dtp472ServiceLineShiftRule(30)], + ["replace a whole element", new StringReplaceRule({ find: "ANYTOWN", replace: "METROPOLIS" })], + ["replace inside a component", new StringReplaceRule({ find: "99213", replace: "99214", mode: "component" })], +]) { + const audit = auditMarks(parse, rule, raw); + check(`${name}: actually changes the fixture`, audit.changed.length > 0, true); + check(`${name}: no change goes unmarked`, audit.unmarked, []); +} + +// The reason processText still runs sameSegment over the segments a rule +// marked, rather than trusting the marks outright. StringReplaceRule counts a +// match whether or not the replacement differs from it, so asking for +// ANYTOWN -> ANYTOWN marks every hit and changes nothing. Without the +// filter each one would surface in the Changes tab as a change with identical +// before and after. +const noop = auditMarks(parse, new StringReplaceRule({ find: "ANYTOWN", replace: "ANYTOWN" }), raw); +check("a replacement equal to its match changes nothing", noop.changed, []); +check("but the rule marks it anyway", noop.overclaimed.length > 0, true); +check("and processText filters it back out", + processText(raw, [new StringReplaceRule({ find: "ANYTOWN", replace: "ANYTOWN" })]).changes, []); + +console.log("\n[14] two rules on one segment report what arrived, not what the second found"); +// The change list is assembled from per-rule copies now, and a second rule +// touching an already-edited segment copies it with the first rule's edit +// already in place. processText keeps the earliest copy. Get this wrong and +// the Changes tab shows a before that never existed in the input file: here +// it would claim 20230102, a value the shift rule invented moments earlier. +const chained = processText(raw, [ + new Dtp472ServiceLineShiftRule(1), // 20230101 -> 20230102 + new StringReplaceRule({ find: "20230102", replace: "19990101" }), // -> 19990101 +]); +const shiftedThenSwapped = chained.changes.filter((c) => c.before.id === "DTP"); +check("the service-line DTPs are listed once each, not twice", shiftedThenSwapped.length, 2); +check("before is the value from the input file", + shiftedThenSwapped.map((c) => c.before.elements[2]), ["20230101", "20230101"]); +check("after is the value both rules left behind", + shiftedThenSwapped.map((c) => c.after.elements[2]), ["19990101", "19990101"]); +check("both rules are credited on the segment", + [...new Set(shiftedThenSwapped.flatMap((c) => c.kinds))].sort(), ["shift", "swap"]); +check("the output agrees with the change list", + (chained.output.match(/DTP\*472\*D8\*19990101~/g) || []).length, 2); + console.log(`\n${pass} passed, ${fail} failed`); process.exit(fail ? 1 : 0); diff --git a/web/tests/render.mjs b/web/tests/render.mjs index c17cfc7..1c60cf4 100644 --- a/web/tests/render.mjs +++ b/web/tests/render.mjs @@ -173,6 +173,28 @@ check("the caution clears for an 837", byId.get("deidRemit").hidden, true); app.loadFile("broken.edi", "this is not an interchange"); check("the caution clears when nothing is loaded", byId.get("deidRemit").hidden, true); +console.log("\n[6] browsing after a run shows the edited document, not the original"); +// render() used to rebuild the browsable document with parse(result.output) -- +// a second full parse of what processText already held. It now takes +// result.doc directly. The output string was never the thing at risk there +// (parity.mjs pins that); what was at risk is the document you browse, so +// that is what this checks: the same values the download carries. +const subscriber = () => + app.state.doc.segments.find((s) => s.id === "NM1" && s.elements[0] === "IL").elements[2]; + +app.loadFile("sample_837p.edi", app.SAMPLE_837P); +check("the loaded document carries the real name", subscriber(), "DOE"); + +app.deidentify(); +const scrubbed = subscriber(); +check("the browsable document shows the scrubbed name", scrubbed !== "DOE", true); +check("it is the document the run produced, not a re-parse of it", + app.state.doc === app.state.result.doc, true); +check("the download agrees with what is on screen", + app.state.result.output.includes(scrubbed), true); +check("the original is still the one that gets re-run", + app.state.raw.includes("DOE"), true); + console.log(`\n${pass} passed, ${fail} failed`); if (fail) process.exit(1); console.log("clean");