From 54d5985be83dacb9328528cd4e5a29d2a1b21125 Mon Sep 17 00:00:00 2001 From: Prabhat Sharma Date: Sun, 2 Aug 2026 15:06:52 -0700 Subject: [PATCH] test(editor): require async behaviour, fix model plumbing, retract a claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four from review were right. 1. C4 WAS PERMITTED, NEVER REQUIRED. Awaiting the provider in the older tests allows an async implementation but does not force one — the existing debounce / prop round-trip / re-trigger arrangement passes every one of them. Added the test that makes the difference observable: a fieldValueResolver that settles after 60ms, and the values must appear in the FIRST result the provider returns, with no second trigger. Plus a companion asserting the resolver is not called outside value context. 2. SHAPE TESTS USED THE WRONG MODEL. mountAndGet created a fresh editor and model, then the signature and hover tests invoked the provider with the module-level mockModel. A correct per-model singleton would refuse it and return null, so those tests would have failed against a correct implementation. mountAndGet now returns its model and the tests use it. 3. MOCK URI WAS NOT STABLE. It generated a new string per call, so an implementation keying per-editor config by model.uri.toString() would lose the entry on the next lookup. Generated once per model now. 4. THE D5 MECHANISM WAS WRONG, AND I HAD STATED IT AS VERIFIED. I claimed the missing MonacoEnvironment.getWorker disabled word-based completion. _getOrCreateWorker catches worker-creation failure and falls back to SynchronousWorkerClient(EditorSimpleWorker) (editorWorkerService.js:301-327), so it does not. Re-measured instead of re-arguing: with a stream selected, a buffer-only token yields no suggestion under the default AND under both wordBasedSuggestions: 'currentDocument' and 'allDocuments', while the control (host -> host_name) works. The observation stands; the explanation does not. wordBasedSuggestions: 'off' therefore stays in scope — one line of explicit intent, kept precisely because whatever is keeping it quiet is unidentified. 18 passing, 12 red for their intended reasons; 6877 passing elsewhere. --- .../CodeQueryEditor.completion.spec.ts | 157 ++++++++++++++++-- 1 file changed, 145 insertions(+), 12 deletions(-) diff --git a/web/src/components/CodeQueryEditor.completion.spec.ts b/web/src/components/CodeQueryEditor.completion.spec.ts index 05052321c4..61448a4cfa 100644 --- a/web/src/components/CodeQueryEditor.completion.spec.ts +++ b/web/src/components/CodeQueryEditor.completion.spec.ts @@ -18,6 +18,7 @@ import { createStore } from "vuex"; // getModel() that returns a fresh object each call makes the provider // permanently unreachable from tests — which is why the provider specs below // were previously skipped. +let modelSeq = 0; const makeModel = () => ({ getValue: vi.fn(() => ""), setValue: vi.fn(), @@ -30,7 +31,12 @@ const makeModel = () => ({ getValueInRange: vi.fn(() => ""), getWordUntilPosition: vi.fn(() => ({ word: "", startColumn: 1, endColumn: 1 })), getWordAtPosition: vi.fn(() => ({ word: "", startColumn: 1, endColumn: 1 })), - uri: { toString: () => `inmemory://model/${Math.random()}` }, + // Stable per model, as real monaco URIs are. A fresh value on every call + // would break any implementation keying per-editor config by uri. + uri: (() => { + const value = `inmemory://model/${modelSeq++}`; + return { toString: () => value }; + })(), }); // The default model, used by the tests that only ever mount one editor. @@ -541,7 +547,9 @@ describe("Phase 3 — providers return the shapes monaco requires", () => { timeout: 15000, interval: 25, }); - return api; + // Hand back THIS editor's model. Invoking a provider with an unrelated + // model is exactly what a correct per-model implementation should refuse. + return { api, model: createdEditors.at(-1)!.getModel() }; }; const position = { lineNumber: 1, column: 12 }; @@ -550,12 +558,12 @@ describe("Phase 3 — providers return the shapes monaco requires", () => { "D2 — provideSignatureHelp returns { value, dispose }, not a bare SignatureHelp", { timeout: 30000 }, async () => { - const api = await mountAndGet(); + const { api, model } = await mountAndGet(); const provider = vi .mocked((api.languages as any).registerSignatureHelpProvider) .mock.calls.at(-1)![1]; - mockModel.getValueInRange.mockReturnValue("SELECT sum("); - const result = await provider.provideSignatureHelp(mockModel, position, {}, {}); + model.getValueInRange.mockReturnValue("SELECT sum("); + const result = await provider.provideSignatureHelp(model, position, {}, {}); // monaco reads result.value (SignatureHelpResult extends IDisposable). // Returning the SignatureHelp directly yields undefined and NO hint, silently. expect(result).toBeTruthy(); @@ -566,19 +574,19 @@ describe("Phase 3 — providers return the shapes monaco requires", () => { ); it("D2 — returns null when the cursor is not in a call", { timeout: 30000 }, async () => { - const api = await mountAndGet(); + const { api, model } = await mountAndGet(); const provider = vi .mocked((api.languages as any).registerSignatureHelpProvider) .mock.calls.at(-1)![1]; - mockModel.getValueInRange.mockReturnValue("SELECT * FROM logs "); - expect(await provider.provideSignatureHelp(mockModel, position, {}, {})).toBeNull(); + model.getValueInRange.mockReturnValue("SELECT * FROM logs "); + expect(await provider.provideSignatureHelp(model, position, {}, {})).toBeNull(); }); it( "D3 — provideHover returns { contents } as IMarkdownString[]", { timeout: 30000 }, async () => { - const api = await mountAndGet(); + const { api, model } = await mountAndGet(); const provider = vi .mocked((api.languages as any).registerHoverProvider) .mock.calls.at(-1)![1]; @@ -587,7 +595,7 @@ describe("Phase 3 — providers return the shapes monaco requires", () => { startColumn: 1, endColumn: 10, }); - const hover = await provider.provideHover(mockModel, position, {}); + const hover = await provider.provideHover(model, position, {}); expect(hover).toBeTruthy(); expect(Array.isArray(hover.contents)).toBe(true); expect(hover.contents[0].value).toContain("Utf8"); @@ -595,17 +603,142 @@ describe("Phase 3 — providers return the shapes monaco requires", () => { ); it("D3 — returns null for a word it knows nothing about", { timeout: 30000 }, async () => { - const api = await mountAndGet(); + const { api, model } = await mountAndGet(); const provider = vi.mocked((api.languages as any).registerHoverProvider).mock.calls.at(-1)![1]; mockModel.getWordAtPosition.mockReturnValue({ word: "zzz_unknown", startColumn: 1, endColumn: 12, }); - expect(await provider.provideHover(mockModel, position, {})).toBeNull(); + expect(await provider.provideHover(model, position, {})).toBeNull(); }); }); +describe("Phase 3 — C4: the value lookup is awaited inside the provider", () => { + const store5 = createStore({ state: { theme: "light" } }); + let spy: ReturnType; + afterAll(() => spy?.mockRestore()); + + // Awaiting the provider in the tests above only PERMITS an async + // implementation; it does not require one. Nothing so far fails if the + // current arrangement survives untouched: parent debounces, calls + // getSuggestions, pushes contextKeywords down as a prop, then force-reopens + // the widget. This is the test that makes the difference observable — the + // values must be in the FIRST result the provider returns, with no second + // trigger and no prop round trip. + it( + "returns field values on the FIRST call, from a delayed resolver", + { timeout: 30000 }, + async () => { + const api = await import("monaco-editor/esm/vs/editor/editor.api"); + const createFn = vi.mocked(api.editor.create); + const registerFn = vi.mocked(api.languages.registerCompletionItemProvider); + const before = createFn.mock.calls.length; + + let resolverCalls = 0; + const fieldValueResolver = vi.fn(async (field: string) => { + resolverCalls += 1; + // Deliberately slower than a microtask: a provider that forgets to await + // returns before this settles. + await new Promise((r) => setTimeout(r, 60)); + return field === "level" ? ["error", "warn"] : []; + }); + + spy = vi + .spyOn(document, "getElementById") + .mockImplementation(() => document.createElement("div")); + mount(CodeQueryEditor, { + props: { + editorId: "c4-editor", + language: "sql", + query: "", + keywords: [ + { name: "level", label: "level", kind: "Field", insertText: "level", detail: "Utf8" }, + ], + suggestions: [], + fieldValueResolver, + }, + global: { plugins: [store5] }, + }); + await vi.waitFor(() => expect(createFn.mock.calls.length).toBe(before + 1), { + timeout: 15000, + interval: 25, + }); + + const model = createdEditors.at(-1)!.getModel(); + const provider = registerFn.mock.calls.filter((c) => c[0] === "sql").at(-1)![1]; + model.getValueInRange.mockReturnValue("SELECT * FROM logs WHERE level = '"); + model.getWordUntilPosition.mockReturnValue({ word: "", startColumn: 34, endColumn: 34 }); + + const result = await provider.provideCompletionItems( + model, + { lineNumber: 1, column: 34 }, + {}, + {}, + ); + + expect(fieldValueResolver, "the provider never asked for values").toHaveBeenCalledWith( + "level", + ); + const labels = result.suggestions.map((s: any) => s.label); + expect(labels, "values did not make the first result — they were not awaited").toEqual( + expect.arrayContaining(["error", "warn"]), + ); + expect(resolverCalls).toBe(1); + }, + ); + + it( + "offers the normal list when the cursor is not after an operator", + { timeout: 30000 }, + async () => { + const api = await import("monaco-editor/esm/vs/editor/editor.api"); + const createFn = vi.mocked(api.editor.create); + const registerFn = vi.mocked(api.languages.registerCompletionItemProvider); + const before = createFn.mock.calls.length; + const fieldValueResolver = vi.fn(async () => ["error"]); + + spy = vi + .spyOn(document, "getElementById") + .mockImplementation(() => document.createElement("div")); + mount(CodeQueryEditor, { + props: { + editorId: "c4-editor-2", + language: "sql", + query: "", + keywords: [ + { name: "level", label: "level", kind: "Field", insertText: "level", detail: "Utf8" }, + ], + suggestions: [], + fieldValueResolver, + }, + global: { plugins: [store5] }, + }); + await vi.waitFor(() => expect(createFn.mock.calls.length).toBe(before + 1), { + timeout: 15000, + interval: 25, + }); + + const model = createdEditors.at(-1)!.getModel(); + const provider = registerFn.mock.calls.filter((c) => c[0] === "sql").at(-1)![1]; + model.getValueInRange.mockReturnValue("SELECT le"); + model.getWordUntilPosition.mockReturnValue({ word: "le", startColumn: 8, endColumn: 10 }); + + const result = await provider.provideCompletionItems( + model, + { lineNumber: 1, column: 10 }, + {}, + {}, + ); + expect(result.suggestions.map((s: any) => s.label)).toContain("level"); + expect( + fieldValueResolver, + "resolver was called outside value context", + ).not.toHaveBeenCalled(); + }, + ); +}); + describe("Phase 3 — C5: one provider per language, not per editor", () => { const store3 = createStore({ state: { theme: "light" } }); let spy: ReturnType;