From 80bb20bced3712f218729877c807672764495394 Mon Sep 17 00:00:00 2001 From: Wei Wang Date: Mon, 19 Oct 2026 04:00:50 -0400 Subject: [PATCH 1/2] Make the live-edit test wait for the table's repaint, not the dirty flag The text-view edit test in liveEdit waited for the shared document to go dirty, which is true as soon as the edit applies, so it no longer waited for the provider's onDidChangeTextDocument listener and its comment said it did. A provider with the listener removed passed it. The test now builds a provider over a stand-in panel on the same document, paints it once, and after the text edit waits for the repaint that listener posts. Removing the listener now fails the test as a timeout naming that repaint. The comments say what the test waits for. --- test-integration/suite/liveEdit.test.ts | 69 +++++++++++++++++-------- 1 file changed, 48 insertions(+), 21 deletions(-) diff --git a/test-integration/suite/liveEdit.test.ts b/test-integration/suite/liveEdit.test.ts index 260bd57..80a0254 100644 --- a/test-integration/suite/liveEdit.test.ts +++ b/test-integration/suite/liveEdit.test.ts @@ -6,9 +6,14 @@ // renders, edits apply, and the extension processes onDidChangeTextDocument // without throwing (a listener error would surface as an unhandled rejection / // deactivated extension). The row-parse itself is covered by the vitest suite. +// Where a test needs to know the provider's change listener ran, it reads the +// repaint that listener posts, from a provider built over a stand-in panel +// (tools/hostHarness.ts fakePanel), since the registered table's webview cannot +// be asked what it was told. import * as assert from 'assert'; import * as vscode from 'vscode'; -import { waitFor } from './tools/hostHarness'; +import { SlddTextEditorProvider } from '../../src/host/SlddTextEditorProvider'; +import { ctx, fakePanel, token, waitFor } from './tools/hostHarness'; // The editable JSON .sldd opens in the tableView, a CustomTextEditorProvider: // the table is a view of the native TextDocument, so there is ONE document and @@ -21,10 +26,12 @@ const BINARY_VIEW = 'dataExplorer.binaryView'; const BINARY_SLDD_VIEW = 'dataExplorer.binarySlddView'; // Locate the `{...}` span of the entries[] element whose "name" equals the given -// name. Kept local (not imported from src/host/entrySplice) so this integration -// bundle stays free of jsonc-parser, which does not `require` cleanly inside the -// Electron test host. The production write-back path uses the real entrySplice; -// its span logic is covered by the vitest suites (entrySplice + editWriteback). +// name. Kept local (not imported from src/host/entrySplice), so the span these +// tests write is found independently of the code under test. (The bundle does +// carry jsonc-parser, through the provider imported above; esbuild.test.mjs's +// mainFields is what makes it load in the Electron test host.) The production +// write-back path uses the real entrySplice; its span logic is covered by the +// vitest suites (entrySplice + editWriteback). function findEntrySpanLocal(text: string, name: string): { offset: number; length: number } | null { const marker = `"name": ${JSON.stringify(name)}`; const nameIdx = text.indexOf(marker); @@ -266,22 +273,42 @@ suite('Data Explorer .sldd live edit', () => { // The document starts clean. assert.strictEqual(doc0.isDirty, false, 'the shared document starts clean'); - // Edit in the TEXT view (valid JSON: prepend whitespace). - const appliedTextEdit = await editor0.edit((eb) => - eb.insert(new vscode.Position(0, 0), ' '), - ); - assert.strictEqual(appliedTextEdit, true, 'the text edit applied'); - - // The provider's onDidChangeTextDocument listener reacts to the same change. - await waitFor('the text edit to dirty the shared document', () => doc0.isDirty); - - // The shared TextDocument is dirty — the single source of truth both views - // observe. - assert.ok(doc0.isDirty, 'the shared document is dirty after a text-view edit'); - assert.ok( - vscode.extensions.getExtension('mathworks.simulink-data-explorer')?.isActive, - 'extension remains active', - ); + // A table over the same document whose messages can be read: the registered + // table's webview cannot be asked what it was told. Painted once, and the + // posts cleared, so anything posted after the edit below is the answer to it. + const view = fakePanel(); + try { + await new SlddTextEditorProvider(ctx()).resolveCustomTextEditor(doc0, view.panel, token()); + view.send({ type: 'ready' }); + await waitFor('the first paint', () => view.posts.some((m) => m.type === 'setRows')); + view.posts.length = 0; + + // Edit in the TEXT view (valid JSON: prepend whitespace). + const appliedTextEdit = await editor0.edit((eb) => + eb.insert(new vscode.Position(0, 0), ' '), + ); + assert.strictEqual(appliedTextEdit, true, 'the text edit applied'); + + // The provider's onDidChangeTextDocument listener answers the change by + // repainting the table: the whole table (setRows) for a change outside every + // entry, as this one is, or one entry's rows (updateEntryRows). Nothing else + // in this test makes it repaint, so a repaint is the listener's own effect; + // a provider with no listener never sends one. + await waitFor('the change listener to repaint the table for the text edit', () => + view.posts.some((m) => m.type === 'setRows' || m.type === 'updateEntryRows'), + ); + await waitFor('the text edit to dirty the shared document', () => doc0.isDirty); + + // The shared TextDocument is dirty — the single source of truth both views + // observe. + assert.ok(doc0.isDirty, 'the shared document is dirty after a text-view edit'); + assert.ok( + vscode.extensions.getExtension('mathworks.simulink-data-explorer')?.isActive, + 'extension remains active', + ); + } finally { + view.close(); + } }); test('an invalid JSON edit is handled gracefully (no crash)', async () => { From bf0d59213a357054c7fd1c274f3780832377dc7d Mon Sep 17 00:00:00 2001 From: Wei Wang Date: Mon, 19 Oct 2026 04:15:11 -0400 Subject: [PATCH 2/2] Make a named wait's timeout say what it saw Three integration waits used to print the state they were watching when they gave up: the names the index held (forgetOnDiskChange), the message types the panel had been sent (twoTabBaseline's lastRows), and how many content changes and empty dirty-state events had arrived (jsonDirtyEventRepaint). Since those loops became named waits, a timeout names only the condition. waitFor now takes an optional detail(), read once at the timeout and added to the message in parentheses; a detail that throws still leaves the timeout naming what it waited for. The three sites pass what they printed before. A new unit test drives the harness's waitFor and checks the message, and that the three sites still pass their detail. --- .../suite/forgetOnDiskChange.test.ts | 1 + .../suite/jsonDirtyEventRepaint.test.ts | 4 + test-integration/suite/tools/hostHarness.ts | 24 +++- test-integration/suite/twoTabBaseline.test.ts | 4 +- test/hostHarnessWaitFor.test.ts | 132 ++++++++++++++++++ 5 files changed, 162 insertions(+), 3 deletions(-) create mode 100644 test/hostHarnessWaitFor.test.ts diff --git a/test-integration/suite/forgetOnDiskChange.test.ts b/test-integration/suite/forgetOnDiskChange.test.ts index f915e06..0348a9a 100644 --- a/test-integration/suite/forgetOnDiskChange.test.ts +++ b/test-integration/suite/forgetOnDiskChange.test.ts @@ -255,6 +255,7 @@ suite('a write no stat can see, with no tab open on it', () => { return names.includes('GainNew'); }, 10000, + () => `the index held ${JSON.stringify(names)}`, ); assert.deepStrictEqual(names, ['GainNew'], 'and the old block is gone from it'); // The forget came before the rebuild was asked for. A rebuild started earlier reads the old diff --git a/test-integration/suite/jsonDirtyEventRepaint.test.ts b/test-integration/suite/jsonDirtyEventRepaint.test.ts index 41f7169..b8aaa3a 100644 --- a/test-integration/suite/jsonDirtyEventRepaint.test.ts +++ b/test-integration/suite/jsonDirtyEventRepaint.test.ts @@ -148,6 +148,10 @@ suite('A JSON .sldd table repaints only for changes that happened', () => { await waitFor( 'both events, and the repaint of the edited entry', () => withChanges >= 1 && empties >= 1 && view.posts.some((m) => m.type === 'updateEntryRows'), + 20000, + () => + `saw ${withChanges} content changes, ${empties} empties, ` + + `and the posts ${JSON.stringify(view.posts.map((m) => m.type))}`, ); // And long enough for a wide repaint to have finished, had either event started one. await quiet(1200, 'a wide repaint started by either event'); diff --git a/test-integration/suite/tools/hostHarness.ts b/test-integration/suite/tools/hostHarness.ts index 4ef2061..f373346 100644 --- a/test-integration/suite/tools/hostHarness.ts +++ b/test-integration/suite/tools/hostHarness.ts @@ -48,15 +48,35 @@ const pause = (ms: number) => new Promise((r) => setTimeout(r, ms)); * * `done` may answer through a promise, for a condition only a query can tell: a usage graph * rebuilt against the open tabs, a file search that lists a new file. + * + * `detail`, when given, says what the wait saw (the names an index held, the message types a + * panel was sent), so a timeout says how it failed and not only that it did. It is read once, + * at the timeout, and added to the message in parentheses; a detail that throws still leaves + * the timeout naming what it waited for. */ -export async function waitFor(what: string, done: () => boolean | Promise, ms = 20000): Promise { +export async function waitFor( + what: string, + done: () => boolean | Promise, + ms = 20000, + detail?: () => string, +): Promise { const deadline = Date.now() + ms; while (!(await done())) { - assert.ok(Date.now() < deadline, `timed out after ${ms}ms waiting for ${what}`); + if (Date.now() >= deadline) assert.fail(`timed out after ${ms}ms waiting for ${what}${seen(detail)}`); await pause(20); } } +/** A wait's `detail`, as the timeout message carries it. */ +function seen(detail: (() => string) | undefined): string { + if (!detail) return ''; + try { + return ` (${detail()})`; + } catch (e) { + return ` (and its detail failed: ${e instanceof Error ? e.message : String(e)})`; + } +} + /** * Let `ms` pass, for what cannot be waited for, and say why. * diff --git a/test-integration/suite/twoTabBaseline.test.ts b/test-integration/suite/twoTabBaseline.test.ts index ceb4bd0..1606723 100644 --- a/test-integration/suite/twoTabBaseline.test.ts +++ b/test-integration/suite/twoTabBaseline.test.ts @@ -123,7 +123,9 @@ function spanOf(text: string, name: string): { offset: number; length: number } */ async function lastRows(posts: any[]): Promise { const setRows = () => posts.filter((m) => m?.type === 'setRows'); - await waitFor('a whole-table paint', () => setRows().length > 0, 30000); + await waitFor('a whole-table paint', () => setRows().length > 0, 30000, () => + `saw ${JSON.stringify(posts.map((m) => m?.type))}`, + ); return setRows()[setRows().length - 1].rows; } diff --git a/test/hostHarnessWaitFor.test.ts b/test/hostHarnessWaitFor.test.ts new file mode 100644 index 0000000..1e3cf22 --- /dev/null +++ b/test/hostHarnessWaitFor.test.ts @@ -0,0 +1,132 @@ +// Copyright 2026 The MathWorks, Inc. +// A named wait that times out says what it saw, not only what it waited for. +// +// The integration suite's waits are `waitFor('', done)` (test-integration/suite/tools/ +// hostHarness.ts), and a timeout fails as "timed out after ms waiting for ". Before +// those waits were named, three of them were loops of their own whose failure also printed the +// state they had been watching: the names the index held (forgetOnDiskChange), the message +// types the panel had been sent (twoTabBaseline's `lastRows`), and how many empty dirty-state +// events had arrived (jsonDirtyEventRepaint). A timeout that names only the condition says that +// it failed, and not how. So `waitFor` takes an optional `detail()`, read once, at the timeout, +// and added to the message; the three sites pass what they printed before. +// +// The harness is driven here as the suite uses it. It imports `vscode` for its other helpers; +// `waitFor` touches none of it, so an empty stand-in is enough. +import { describe, it, expect, vi } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import ts from 'typescript'; +import { waitFor } from '../test-integration/suite/tools/hostHarness'; + +vi.mock('vscode', () => ({})); + +/** The message `waitFor` failed with; the test fails if it did not fail. */ +async function failure(wait: Promise): Promise { + try { + await wait; + } catch (e) { + return (e as Error).message; + } + throw new Error('the wait was expected to time out'); +} + +describe('a named wait that times out says what it saw', () => { + it('carries its detail text in the error', async () => { + const message = await failure(waitFor('the thing', () => false, 60, () => 'saw ["ready","setRows"]')); + expect(message).toBe('timed out after 60ms waiting for the thing (saw ["ready","setRows"])'); + }); + + it('reads the detail at the timeout, so it shows the last state the condition saw', async () => { + let polls = 0; + const message = await failure( + waitFor( + 'the count to reach a million', + async () => { + polls++; + return false; + }, + 60, + () => `saw ${polls} polls`, + ), + ); + expect(message).toMatch(/^timed out after 60ms waiting for the count to reach a million \(saw (\d+) polls\)$/); + expect(Number(/saw (\d+) polls/.exec(message)![1])).toBe(polls); + expect(polls).toBeGreaterThan(1); + }); + + it('asks for no detail when the condition comes true', async () => { + let asked = 0; + let polls = 0; + await waitFor( + 'the third poll', + () => ++polls >= 3, + 5000, + () => { + asked++; + return 'never shown'; + }, + ); + expect(polls).toBe(3); + expect(asked).toBe(0); + }); + + it('still names what it waited for when the detail itself throws', async () => { + const message = await failure( + waitFor('the thing', () => false, 60, () => { + throw new Error('no posts array'); + }), + ); + expect(message).toBe('timed out after 60ms waiting for the thing (and its detail failed: no posts array)'); + }); + + it('without a detail, says what it waited for, as before', async () => { + expect(await failure(waitFor('the thing', () => false, 60))).toBe('timed out after 60ms waiting for the thing'); + }); +}); + +// --- the three sites that printed what they saw before their waits were named ------------ + +const SUITE = join(import.meta.dirname, '..', 'test-integration', 'suite'); + +/** The `waitFor` call in `file` whose name is `what`, with its arguments as source text. */ +function waitNamed(file: string, what: string): string[] { + const sf = ts.createSourceFile(file, readFileSync(join(SUITE, file), 'utf8'), ts.ScriptTarget.Latest, true); + const found: string[][] = []; + const visit = (node: ts.Node): void => { + if ( + ts.isCallExpression(node) && + ts.isIdentifier(node.expression) && + node.expression.text === 'waitFor' && + node.arguments[0] && + (ts.isStringLiteral(node.arguments[0]) || ts.isNoSubstitutionTemplateLiteral(node.arguments[0])) && + node.arguments[0].text === what + ) { + found.push(node.arguments.map((a) => a.getText(sf))); + } + ts.forEachChild(node, visit); + }; + visit(sf); + expect(found, `one waitFor('${what}') in ${file}`).toHaveLength(1); + return found[0]; +} + +describe('the waits that printed what they saw still do', () => { + it("forgetOnDiskChange's index wait shows the names the index held", () => { + const args = waitNamed('forgetOnDiskChange.test.ts', 'the index to follow the file'); + expect(args).toHaveLength(4); + expect(args[3]).toMatch(/^\(\) =>[\s\S]*JSON\.stringify\(names\)/); + }); + + it("twoTabBaseline's lastRows shows the message types the panel was sent", () => { + const args = waitNamed('twoTabBaseline.test.ts', 'a whole-table paint'); + expect(args).toHaveLength(4); + expect(args[3]).toMatch(/^\(\) =>[\s\S]*JSON\.stringify\(posts\.map\(\(m\) => m\?\.type\)\)/); + }); + + it('jsonDirtyEventRepaint shows the events and the posts it counted', () => { + const args = waitNamed('jsonDirtyEventRepaint.test.ts', 'both events, and the repaint of the edited entry'); + expect(args).toHaveLength(4); + expect(args[3]).toMatch(/^\(\) =>/); + for (const seen of ['withChanges', 'empties', 'view.posts']) expect(args[3]).toContain(seen); + }); +});