test(editor): fix four defects in the Phase 2 specs

Adversarial review of the specs I had just written.

1. CONTRADICTION — two tests in sqlCompletion.spec.ts made the same call
   mutually unsatisfiable: one required resolveKeywords('sql', []) to EQUAL
   SQL_KEYWORDS, another required it to CONTAIN SELECT. With clauses in a
   separate export no implementation could satisfy both. The Phase 1 test was
   the stale one (written before clause keywords existed); it now expects the
   union.

2. MISLEADING TITLE, WEAK BODY — 'sorts clause keywords after fields' only
   asserted that a sortText existed, never the ordering it claimed. That is the
   data-exists-instead-of-behaviour pattern I have been flagging all along.
   Now compares against an actual field entry's sortText.

3. UNWIRED HELPER — mergeServerFunctions was tested only in isolation, exactly
   the gap external review caught in Phase 1: the helper can be perfect while
   nothing connects it to what the editor consumes. Added six composable-level
   tests driving setServerFunctions through effectiveSuggestions, including
   that a server entry must NOT override local insertion detail (that would
   reintroduce sum('field')).

4. RUST TYPE AMBIGUITY — the catalog test used .clone() then .sort(), which
   only compiles if the accessor returns Vec. Uses .to_vec() so it cannot fail
   for a type reason instead of the behaviour under test.

Also closed two coverage gaps: the global snippet sweep skipped clause keywords
entirely, and predicates carry no sortText in the raw catalog while clauses do
— incoherent for the component fallback that Traces and Dashboards use.

Verified json_get_str/json_length are the real registered names in the vendored
datafusion-functions-json (aliases[0]), and that crate::sql::rewriter is a
reachable path from datafusion::exec (both are pub mod siblings).

Phase 2: 59 failing, 2 suites blocked. Phase 1 suites still green.
This commit is contained in:
Prabhat Sharma 2026-08-02 11:57:38 -07:00
parent be128b48c4
commit 82de244fb1
3 changed files with 98 additions and 6 deletions

View File

@ -871,10 +871,10 @@ mod tests {
"rewriter alias missing from the catalog union"
);
assert!(catalog.iter().any(|n| n == "json_get"), "json function missing");
let mut sorted = catalog.clone();
let mut sorted = catalog.to_vec();
sorted.sort();
sorted.dedup();
assert_eq!(catalog, sorted, "catalog must be sorted and deduplicated");
assert_eq!(catalog.to_vec(), sorted, "catalog must be sorted and deduplicated");
}
#[test]

View File

@ -494,3 +494,68 @@ describe("Phase 2 — SQL clause keywords are offered (B1)", () => {
expect(host.sortText < select.sortText).toBe(true);
});
});
// ─── Phase 2 (tmp/code.md B4) — server functions must actually REACH the editor
// Testing mergeServerFunctions in isolation would repeat the Phase 1 mistake:
// the helper can be perfect while nothing wires it to what the editor consumes.
describe("Phase 2 — server-supplied functions reach the suggestion list (B4)", () => {
beforeEach(() => vi.clearAllMocks());
const serverList = [
{ name: "date_trunc", signature: "(precision, timestamp)", doc: "Truncate a timestamp." },
{ name: "coalesce", signature: "(a, b)", doc: "First non-null argument." },
];
it("adds server-only functions to what the editor receives", async () => {
const c = makeComposable({ storedValues: [] });
c.setServerFunctions(serverList);
await run(c, "SELECT * FROM stream WHERE ");
const names = (c.effectiveSuggestions.value as any[]).map((s) => s.name);
expect(names).toContain("date_trunc");
expect(names).toContain("coalesce");
});
it("keeps the hand-written O2 catalog alongside them", async () => {
const c = makeComposable({ storedValues: [] });
c.setServerFunctions(serverList);
await run(c, "SELECT * FROM stream WHERE ");
const names = (c.effectiveSuggestions.value as any[]).map((s) => s.name);
for (const local of ["match_all", "histogram", "approx_topk"]) {
expect(names, `lost local ${local}`).toContain(local);
}
});
it("does NOT let a server entry override local insertion detail", async () => {
// The server knows arity, not which arguments are columns. Letting it win
// would reintroduce sum('field') — the A3 bug Phase 1 fixed.
const c = makeComposable({ storedValues: [] });
c.setServerFunctions([{ name: "sum", signature: "(expr)", doc: "Sum." }]);
await run(c, "SELECT * FROM stream WHERE ");
const sums = (c.effectiveSuggestions.value as any[]).filter((s) => s.name === "sum");
expect(sums).toHaveLength(1);
expect(sums[0].insertText).toBe("sum(${1:field})");
});
it("still blanks the suggestion list in value context", async () => {
const c = makeComposable({ storedValues: ["error"] });
c.setServerFunctions(serverList);
await run(c, "level = ");
expect(c.effectiveSuggestions.value).toEqual([]);
});
it("is a no-op when the server call returned nothing", async () => {
const c = makeComposable({ storedValues: [] });
c.setServerFunctions([]);
await run(c, "SELECT * FROM stream WHERE ");
const names = (c.effectiveSuggestions.value as any[]).map((s) => s.name);
expect(names).toContain("match_all");
});
it("survives a failed server call without throwing", async () => {
const c = makeComposable({ storedValues: [] });
expect(() => c.setServerFunctions(undefined as any)).not.toThrow();
await run(c, "SELECT * FROM stream WHERE ");
expect((c.effectiveSuggestions.value as any[]).length).toBeGreaterThan(0);
});
});

View File

@ -308,7 +308,7 @@ describe("A5 — snippet rules are mapped from string name to numeric enum", ()
});
it("every catalog entry containing a ${…} tab stop declares InsertAsSnippet", () => {
for (const entry of [...SQL_FUNCTIONS, ...SQL_KEYWORDS]) {
for (const entry of [...SQL_FUNCTIONS, ...SQL_KEYWORDS, ...SQL_CLAUSE_KEYWORDS]) {
if (entry.insertText.includes("${")) {
expect(entry.insertTextRules, `${entry.name} has tab stops but no snippet rule`).toBe(
"InsertAsSnippet",
@ -605,9 +605,12 @@ describe("N2/D7 — resolveSuggestions / resolveKeywords", () => {
expect(resolveSuggestions("promql", null)).toEqual([]);
});
it("falls back to the shared keyword list when the SQL keywords prop is empty", async () => {
it("falls back to predicates AND clause keywords when the SQL prop is empty", async () => {
// Was `toEqual(SQL_KEYWORDS)` — written before clause keywords existed, and
// directly contradicted by the B1 expectations below (which require SELECT
// to come back from this same call). A SQL editor needs both lists.
const { resolveKeywords } = await import("./sqlCompletion");
expect(resolveKeywords("sql", [])).toEqual(SQL_KEYWORDS);
expect(resolveKeywords("sql", [])).toEqual([...SQL_KEYWORDS, ...SQL_CLAUSE_KEYWORDS]);
});
it("prefers caller-supplied keywords over the defaults", async () => {
@ -721,10 +724,34 @@ describe("B1 — the clause keyword catalog", () => {
}
});
it("sorts clause keywords after fields but they remain reachable", () => {
it("sorts every clause keyword behind a field of the same name-space", () => {
// Clause keywords are structural: useful, but never ahead of a column name.
// Assert the ORDERING the title claims, not merely that a sortText exists.
const field = buildFieldEntry({ name: "zzz_last_field", type: "Utf8" });
for (const k of SQL_CLAUSE_KEYWORDS) {
expect(k.sortText, `${k.name} needs an explicit sortText`).toBeTruthy();
expect(
field.sortText! < k.sortText!,
`${k.name} (${k.sortText}) must sort after fields (${field.sortText})`,
).toBe(true);
}
});
});
describe("B1 — keyword ordering is coherent in the raw catalog", () => {
it("gives predicates an explicit sortText too, not just clauses", () => {
// useSuggestions assigns one at runtime, but the component fallback used by
// Traces and Dashboards consumes the catalog directly. Without it there,
// predicates sort by label and interleave with the clause keywords.
for (const k of SQL_KEYWORDS) {
expect(k.sortText, `${k.name} needs an explicit sortText`).toBeTruthy();
}
});
it("orders predicates and clauses in the same name-space", () => {
const field = buildFieldEntry({ name: "zzz", type: "Utf8" });
for (const k of [...SQL_KEYWORDS, ...SQL_CLAUSE_KEYWORDS]) {
expect(field.sortText! < k.sortText!, `${k.name} sorts ahead of fields`).toBe(true);
}
});
});