From fe9d58d580cfa5ab90f9e4359d34de2bf82e0abc Mon Sep 17 00:00:00 2001 From: Eric Winger Date: Wed, 19 Aug 2026 13:57:37 -0700 Subject: [PATCH 01/15] SUnit: one runner path, a result store, and gutter-ready test items MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Groundwork for running a test class or test method from the GemStone Explorer and from the editor. No new user-facing entry points yet — this is the engine everything will share. - Every named entry point (browser menus today, Explorer rows and code lenses next) now funnels through the same run path as the Test Explorer's own run profile, so all of them report identically. The second class-run path and the faked CancellationToken are gone. - The controller keeps the last-known outcome per class and per method, written in one place and published through onDidChangeResults, so a UI outside the Test Explorer can show pass/fail without re-running anything. Running state is published before the (blocking) stone call so a row can show a spinner. - Test items now carry the URI the editor actually opens — built with the shared builders, dictionary-scoped — plus a range, which is what makes VS Code draw a run/status icon in the gutter. Test-class discovery reports the SymbolList index so those URIs can be scoped. - Compiling a method or a class definition drops the outcome that described it and marks the rest stale, so a green check never outlives the code it was about. Toward #427. Co-Authored-By: Claude Opus 5 (1M context) --- client/src/__mocks__/vscode.ts | 23 + client/src/__tests__/sunitQueries.test.ts | 22 + .../src/__tests__/sunitTestController.test.ts | 220 ++++++++ client/src/extension.ts | 19 + client/src/queries/discoverTestClasses.ts | 41 +- client/src/sunitTestController.ts | 489 ++++++++++++++---- 6 files changed, 699 insertions(+), 115 deletions(-) diff --git a/client/src/__mocks__/vscode.ts b/client/src/__mocks__/vscode.ts index 95a58ff7..38b5321d 100644 --- a/client/src/__mocks__/vscode.ts +++ b/client/src/__mocks__/vscode.ts @@ -81,6 +81,29 @@ export class ThemeIcon { constructor(public readonly id: string) {} } +// ── CancellationTokenSource mock ─────────────────────────── + +export class CancellationTokenSource { + private listeners: Array<() => void> = []; + + readonly token = { + isCancellationRequested: false, + onCancellationRequested: (listener: () => void) => { + this.listeners.push(listener); + return { dispose: () => {} }; + }, + }; + + cancel(): void { + this.token.isCancellationRequested = true; + for (const listener of this.listeners) listener(); + } + + dispose(): void { + this.listeners = []; + } +} + // ── EventEmitter mock ────────────────────────────────────── export class EventEmitter { diff --git a/client/src/__tests__/sunitQueries.test.ts b/client/src/__tests__/sunitQueries.test.ts index 6f0e6744..e0c99d0c 100644 --- a/client/src/__tests__/sunitQueries.test.ts +++ b/client/src/__tests__/sunitQueries.test.ts @@ -63,6 +63,28 @@ describe('sunitQueries', () => { expect(results.map((r) => r.testCount)).toEqual([null, null, null, null]); }); + it('parses the dictionary index so callers can build ?dict=N URIs', () => { + const session = createMockSession('UserGlobals\tMyTestCase\t7\t3\n'); + expect(sunit.discoverTestClasses(session)[0].dictIndex).toBe(3); + }); + + it('leaves the dictionary index undefined when the stone sends no usable one', () => { + const session = createMockSession( + 'A\tMissing\t1\t\n' + // empty index field + 'B\tNonNumeric\t1\tabc\n' + // not a number + 'C\tZero\t1\t0\n', // 0 — SymbolList indexes are 1-based + ); + const results = sunit.discoverTestClasses(session); + expect(results.map((r) => r.dictIndex)).toEqual([undefined, undefined, undefined]); + }); + + it('reads the dictionary index from the symbol list position', () => { + const session = createMockSession(''); + sunit.discoverTestClasses(session); + const code = (session.gci.executeAndFetchString as ReturnType).mock.calls[0][1]; + expect(code).toContain('1 to: sl size do:'); + }); + it('returns empty array when no test classes exist', () => { const session = createMockSession(''); expect(sunit.discoverTestClasses(session)).toEqual([]); diff --git a/client/src/__tests__/sunitTestController.test.ts b/client/src/__tests__/sunitTestController.test.ts index 0167f54a..1cf41d25 100644 --- a/client/src/__tests__/sunitTestController.test.ts +++ b/client/src/__tests__/sunitTestController.test.ts @@ -498,6 +498,226 @@ describe('SunitTestController', () => { }); }); + describe('result store', () => { + // The tree rows, the code lenses and the Test Explorer all read the same + // store, so what it holds after a run is what the user sees everywhere. + async function runClass(ctrl: SunitTestController) { + await ctrl.runClassByName('UserGlobals', 'MyTestCase'); + } + + it('records each method outcome from a class run', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + await runClass(ctrl); + + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testAdd')).toMatchObject({ + outcome: 'passed', + durationMs: 5, + }); + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testRemove')).toMatchObject({ + outcome: 'failed', + message: 'Expected true', + }); + ctrl.dispose(); + }); + + it('records a passed/total roll-up on the class itself', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + await runClass(ctrl); + + expect(ctrl.resultFor('UserGlobals', 'MyTestCase')).toMatchObject({ + outcome: 'failed', + passedCount: 1, + totalCount: 2, + }); + ctrl.dispose(); + }); + + it('records the outcome of a single-method run', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + await ctrl.runTestsByName('UserGlobals', 'MyTestCase', ['testAdd']); + + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testAdd')).toMatchObject({ + outcome: 'passed', + durationMs: 10, + }); + ctrl.dispose(); + }); + + it('publishes a running state before the outcome so the row can show a spinner', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + const seen: (string | undefined)[] = []; + ctrl.onDidChangeResults(() => { + seen.push(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testAdd')?.outcome); + }); + + await ctrl.runTestsByName('UserGlobals', 'MyTestCase', ['testAdd']); + + expect(seen[0]).toBe('running'); + expect(seen.at(-1)).toBe('passed'); + ctrl.dispose(); + }); + + it('batches the change event rather than firing per test', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + let fires = 0; + ctrl.onDidChangeResults(() => { + fires += 1; + }); + + await runClass(ctrl); + + // Once for "all running", once for the finished run — not once per test. + expect(fires).toBe(2); + ctrl.dispose(); + }); + + it('drops every result when the session changes', async () => { + const sm = makeSessionManager(true); + const ctrl = new SunitTestController(sm); + await runClass(ctrl); + + const onSelectionChange = (sm.onDidChangeSelection as ReturnType).mock + .calls[0][0] as () => Promise; + await onSelectionChange(); + + expect(ctrl.resultFor('UserGlobals', 'MyTestCase')).toBeUndefined(); + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testAdd')).toBeUndefined(); + ctrl.dispose(); + }); + }); + + describe('invalidation when code is compiled', () => { + it('drops the recompiled method and its class roll-up', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + await ctrl.runClassByName('UserGlobals', 'MyTestCase'); + + ctrl.invalidateForMethod('UserGlobals', 'MyTestCase', 'testAdd'); + + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testAdd')).toBeUndefined(); + expect(ctrl.resultFor('UserGlobals', 'MyTestCase')).toBeUndefined(); + ctrl.dispose(); + }); + + it('marks the results it keeps stale — they predate the edit', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + await ctrl.runClassByName('UserGlobals', 'MyTestCase'); + + ctrl.invalidateForMethod('UserGlobals', 'MyTestCase', 'testAdd'); + + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testRemove')).toMatchObject({ + outcome: 'failed', + stale: true, + }); + ctrl.dispose(); + }); + + it('drops every result for a class whose definition was recompiled', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + await ctrl.runClassByName('UserGlobals', 'MyTestCase'); + + ctrl.invalidateForClass('UserGlobals', 'MyTestCase'); + + expect(ctrl.resultFor('UserGlobals', 'MyTestCase')).toBeUndefined(); + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testAdd')).toBeUndefined(); + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testRemove')).toBeUndefined(); + ctrl.dispose(); + }); + + it('fires a change event so the rows repaint', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + await ctrl.runClassByName('UserGlobals', 'MyTestCase'); + let fires = 0; + ctrl.onDidChangeResults(() => { + fires += 1; + }); + + ctrl.invalidateForMethod('UserGlobals', 'MyTestCase', 'testAdd'); + + expect(fires).toBe(1); + ctrl.dispose(); + }); + + it('stays quiet when there is nothing to invalidate', () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + let fires = 0; + ctrl.onDidChangeResults(() => { + fires += 1; + }); + + ctrl.invalidateForMethod('UserGlobals', 'MyTestCase', 'testAdd'); + + expect(fires).toBe(0); + ctrl.dispose(); + }); + }); + + describe('editor gutter wiring', () => { + // VS Code matches a test item to an open editor by exact URI and draws the + // run/status icon at the item's range. Both must therefore be the URI the + // editor itself opens — including the ?dict=N scope — not a hand-built one. + // Only this group cares about the dictionary index, and a persistent + // mockReturnValue would leak into the other groups (tests are shuffled). + function discoverWithDictIndex() { + (sunit.discoverTestClasses as ReturnType).mockReturnValueOnce([ + { dictName: 'UserGlobals', className: 'MyTestCase', testCount: 2, dictIndex: 3 }, + ]); + } + + it('points a class item at its class-definition document', async () => { + discoverWithDictIndex(); + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + + await mockController.resolveHandler(undefined); + + const [, , uri] = (mockController.createTestItem as ReturnType).mock.calls[0]; + expect(uri.toString()).toBe( + 'gemstone://1/UserGlobals/MyTestCase/definition/MyTestCase?dict%3D3', + ); + ctrl.dispose(); + }); + + it('points a method item at its method document', async () => { + discoverWithDictIndex(); + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + + await mockController.resolveHandler(undefined); + const classItem = mockController.items.get('sunit/1/UserGlobals/MyTestCase'); + await mockController.resolveHandler(classItem); + + const methodCall = ( + mockController.createTestItem as ReturnType + ).mock.calls.find((call: unknown[]) => call[1] === 'testAdd'); + expect(methodCall).toBeDefined(); + expect(methodCall![2].toString()).toBe( + 'gemstone://1/UserGlobals/MyTestCase/instance/unit%20tests/testAdd?dict%3D3', + ); + ctrl.dispose(); + }); + + it('gives class and method items a range, without which no icon is drawn', async () => { + discoverWithDictIndex(); + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + + await mockController.resolveHandler(undefined); + const classItem = mockController.items.get('sunit/1/UserGlobals/MyTestCase'); + await mockController.resolveHandler(classItem); + + expect(classItem.range.start.line).toBe(0); + const methodItems: { range: { start: { line: number } } }[] = []; + classItem.children.forEach((child: { range: { start: { line: number } } }) => + methodItems.push(child), + ); + expect(methodItems[0].range.start.line).toBe(0); + ctrl.dispose(); + }); + }); + describe('dispose', () => { it('disposes the controller', () => { const sm = makeSessionManager(true); diff --git a/client/src/extension.ts b/client/src/extension.ts index 521ce2b4..92a362b9 100644 --- a/client/src/extension.ts +++ b/client/src/extension.ts @@ -96,6 +96,7 @@ import { closeGemstoneTabsForSession, installStaleGemstoneTabReaper, buildMethodUri, + parseMethodUri, parseUri, } from './gemstoneFileSystemProvider'; import { openWorkspace } from './workspace'; @@ -946,6 +947,24 @@ export function activate(context: vscode.ExtensionContext) { const sunitTestController = new SunitTestController(sessionManager); context.subscriptions.push(sunitTestController); + // Keep the pass/fail indicators honest. A compiled method or class definition + // means the outcome shown beside it predates the code now in the stone: the + // recompiled thing's own result is dropped, and everything still showing a + // result is marked stale (see SunitTestController.invalidateForMethod). + context.subscriptions.push( + gemstoneFs.onMethodCompiled((e) => { + const method = parseMethodUri(e.uri); + if (method) { + sunitTestController.invalidateForMethod(method.dictName, method.className, method.selector); + } + }), + gemstoneFs.onClassDefinitionCompiled((e) => { + // parts: ['', dictName, className, 'definition', …] + const parts = e.uri.path.split('/').map(decodeURIComponent); + if (parts.length >= 3) sunitTestController.invalidateForClass(parts[1], parts[2]); + }), + ); + // ── Jupyter Notebook Kernels (Grail Python + Smalltalk) ─ const grailNotebookController = new GrailNotebookController(sessionManager); context.subscriptions.push(grailNotebookController); diff --git a/client/src/queries/discoverTestClasses.ts b/client/src/queries/discoverTestClasses.ts index 072f684c..a96d08a9 100644 --- a/client/src/queries/discoverTestClasses.ts +++ b/client/src/queries/discoverTestClasses.ts @@ -4,6 +4,12 @@ import { splitLines } from './util'; export interface TestClassInfo { dictName: string; className: string; + // 1-based SymbolList index of the dictionary the class was found in. Carried + // so callers can build the same `?dict=N`-scoped gemstone:// URIs the editor + // opens — a test item whose URI differs from the opened document's by so much + // as the query string is not recognised as that document's test. undefined + // when the stone returned an unparseable index. + dictIndex?: number; // Number of test methods (testSelectors) — shown in the Test Explorer and // used to sanity-check counts without expanding the class. A non-negative // integer, or null when the stone returned an unparseable/invalid value @@ -22,26 +28,45 @@ function parseTestCount(raw: string | undefined): number | null { return Number.isInteger(n) && n >= 0 ? n : null; } +// SymbolList indexes are 1-based, so 0 and negatives are as invalid as NaN. +// undefined means "unknown" — callers then fall back to dictionary-name lookup. +function parseDictIndex(raw: string | undefined): number | undefined { + if (raw === undefined || raw.trim() === '') return undefined; + const n = Number(raw); + return Number.isInteger(n) && n >= 1 ? n : undefined; +} + export function discoverTestClasses(execute: QueryExecutor): TestClassInfo[] { + // Walk the symbol list by index rather than with `do:` so each class carries + // the 1-based index of the dictionary it was found in. `(classDict includesKey:)` + // keeps the first dictionary that defines a class — the same one bare-name + // lookup would resolve to — so the recorded index always matches the class we + // report. const code = `| ws sl classDict | sl := System myUserProfile symbolList. classDict := IdentityDictionary new. -sl do: [:dict | - dict keysAndValuesDo: [:k :v | +1 to: sl size do: [:i | + (sl at: i) keysAndValuesDo: [:k :v | (v isBehavior and: [(v isSubclassOf: TestCase) and: [v ~~ TestCase and: [(classDict includesKey: v) not]]]) - ifTrue: [classDict at: v put: dict name]]]. + ifTrue: [classDict at: v put: (Array with: (sl at: i) name with: i)]]]. ws := WriteStream on: Unicode7 new. -classDict keysAndValuesDo: [:cls :dictName | - ws nextPutAll: dictName; tab; +classDict keysAndValuesDo: [:cls :dictInfo | + ws nextPutAll: (dictInfo at: 1); tab; nextPutAll: cls name; tab; - nextPutAll: cls testSelectors size printString; lf]. + nextPutAll: cls testSelectors size printString; tab; + nextPutAll: (dictInfo at: 2) printString; lf]. ws contents`; const data = execute(code); return splitLines(data).map((line) => { - const [dictName, className, count] = line.split('\t'); - return { dictName, className, testCount: parseTestCount(count) }; + const [dictName, className, count, dictIndex] = line.split('\t'); + return { + dictName, + className, + testCount: parseTestCount(count), + dictIndex: parseDictIndex(dictIndex), + }; }); } diff --git a/client/src/sunitTestController.ts b/client/src/sunitTestController.ts index a5815703..f6c79c61 100644 --- a/client/src/sunitTestController.ts +++ b/client/src/sunitTestController.ts @@ -1,5 +1,6 @@ -import type { CancellationToken, TestItem } from 'vscode'; +import type { TestItem } from 'vscode'; import * as vscode from 'vscode'; +import { buildClassDefinitionUri, buildMethodUri } from './gemstoneFileSystemProvider'; import { ActiveSession, SessionManager } from './sessionManager'; import * as sunit from './sunitQueries'; @@ -40,6 +41,56 @@ function parseTestId(id: string): ParsedTestId { return { dictName, className, selector }; } +/** + * How a test was launched. Everything above the innermost "execute one test" + * step is shared between the two — discovery, id parsing, reporting, and the + * result store all behave identically, so a debugged test lights up the same + * places a run one does. + */ +export type SunitRunKind = 'run' | 'debug'; + +/** State of the most recent run of one test class or test method. */ +export type SunitOutcome = 'running' | 'passed' | 'failed' | 'error'; + +/** + * The last-known outcome of a class or method, kept so the tree rows, code + * lenses, and anything else outside the Test Explorer can show it. Written in + * exactly one place (`setResult`, reached only from `reportOutcome`), so every + * entry point — Explorer button, gutter, code lens, Test Explorer — produces + * the same indicator. + */ +export interface SunitResult { + outcome: SunitOutcome; + /** One-line failure/error text. Empty for a pass. */ + message?: string; + /** Elapsed time as measured on the stone. Absent when it wasn't measured. */ + durationMs?: number; + /** Class rows only: how many of the class's tests passed, out of how many. */ + passedCount?: number; + totalCount?: number; + /** + * True once code has been compiled since this result was produced, so the + * outcome may no longer describe the code in the stone. Stale results are + * shown dimmed rather than dropped — "this was green before your edit" is + * more useful than a blank row, as long as it doesn't masquerade as current. + */ + stale?: boolean; +} + +/** Key into the result store. Session-scoped implicitly: the store is cleared + * whenever the selected session changes, so ids need no session segment. */ +function resultKey(dictName: string, className: string, selector?: string): string { + return selector === undefined + ? `${dictName}/${className}` + : `${dictName}/${className}/${selector}`; +} + +/** Test items are one-method (or one class-definition) documents, so the run + * icon always belongs on the first line. */ +function topOfDocument(): vscode.Range { + return new vscode.Range(new vscode.Position(0, 0), new vscode.Position(0, 0)); +} + /** * Integrates GemStone SUnit tests with VS Code's Test Explorer. */ @@ -50,6 +101,22 @@ export class SunitTestController implements vscode.Disposable { /** category cache populated during method discovery, keyed by dictName/className/selector */ private methodCategory = new Map(); + /** SymbolList index per class item id, captured at discovery — needed to + * build `?dict=N` URIs that match the documents the editor actually opens. */ + private classDictIndex = new Map(); + + /** Last-known outcome per class/method. See SunitResult. */ + private results = new Map(); + private resultsDirty = false; + + private _onDidChangeResults = new vscode.EventEmitter(); + /** + * Fires when one or more stored results changed. Batched: a class run of + * fourteen tests fires twice (all-running, then all-done), not twenty-eight + * times, so a listening tree view refreshes twice too. + */ + readonly onDidChangeResults = this._onDidChangeResults.event; + constructor(private sessionManager: SessionManager) { this.controller = vscode.tests.createTestController('gemstone-sunit', 'GemStone SUnit Tests'); @@ -69,15 +136,16 @@ export class SunitTestController implements vscode.Disposable { ); this.controller.refreshHandler = async () => { - this.methodCategory.clear(); - this.controller.items.replace([]); + this.resetDiscovery(); await this.discoverTests(); }; this.disposables.push( sessionManager.onDidChangeSelection(async () => { - this.methodCategory.clear(); - this.controller.items.replace([]); + // Results belong to the stone they ran against, so a different session + // means every stored outcome is meaningless, not merely stale. + this.clearResults(); + this.resetDiscovery(); await this.discoverTests(); }), ); @@ -85,60 +153,127 @@ export class SunitTestController implements vscode.Disposable { dispose(): void { this.controller.dispose(); + this._onDidChangeResults.dispose(); for (const d of this.disposables) d.dispose(); } /** Clear items and let resolveHandler re-discover on next view. */ refresh(): void { + this.resetDiscovery(); + } + + private resetDiscovery(): void { this.methodCategory.clear(); + this.classDictIndex.clear(); this.controller.items.replace([]); } - /** Run all tests in a named class (bridge for browser tree context menu). */ - async runClassByName(dictName: string, className: string): Promise { - const session = this.sessionManager.getSelectedSession(); - if (!session) { - vscode.window.showErrorMessage('No active GemStone session.'); - return; - } + // ── Result store ─────────────────────────────────────────── - // Ensure discovery has run so the item exists - let classItem = this.findClassItem(dictName, className); - if (!classItem) { - await this.discoverTests(); - classItem = this.findClassItem(dictName, className); + /** + * The last-known outcome for a class (omit `selector`) or one of its test + * methods, or undefined if it hasn't run since the last invalidation. + */ + resultFor(dictName: string, className: string, selector?: string): SunitResult | undefined { + return this.results.get(resultKey(dictName, className, selector)); + } + + private setResult( + dictName: string, + className: string, + selector: string | undefined, + result: SunitResult, + ): void { + this.results.set(resultKey(dictName, className, selector), result); + this.resultsDirty = true; + } + + private flushResultChanges(): void { + if (!this.resultsDirty) return; + this.resultsDirty = false; + this._onDidChangeResults.fire(); + } + + /** Drop every stored result (e.g. the session changed underneath them). */ + clearResults(): void { + if (this.results.size > 0) { + this.results.clear(); + this.resultsDirty = true; } + this.flushResultChanges(); + } - if (!classItem) { - vscode.window.showWarningMessage(`${className} is not a TestCase subclass in ${dictName}.`); - return; + /** + * A method was recompiled: its own result and its class's roll-up no longer + * describe what is in the stone, so they go. Everything else is marked stale + * rather than dropped — recompiling any method can change what any test does, + * but only the edited one is known to be wrong. + */ + invalidateForMethod(dictName: string, className: string, selector: string): void { + this.deleteResult(dictName, className, selector); + this.deleteResult(dictName, className); + this.markRemainingStale(); + this.flushResultChanges(); + } + + /** + * A class definition was recompiled: every result for that class goes (its + * methods may not even exist any more), and the rest go stale. + */ + invalidateForClass(dictName: string, className: string): void { + const classPrefix = `${resultKey(dictName, className)}/`; + const classOwn = resultKey(dictName, className); + for (const key of [...this.results.keys()]) { + if (key === classOwn || key.startsWith(classPrefix)) { + this.results.delete(key); + this.resultsDirty = true; + } } + this.markRemainingStale(); + this.flushResultChanges(); + } - // Ensure children are resolved - if (classItem.children.size === 0) { - await this.resolveTestMethods(classItem); + private deleteResult(dictName: string, className: string, selector?: string): void { + if (this.results.delete(resultKey(dictName, className, selector))) this.resultsDirty = true; + } + + /** Mark every settled result stale. A running test is left alone — it is + * about to be overwritten with a fresh outcome anyway. */ + private markRemainingStale(): void { + for (const [key, result] of this.results) { + if (result.stale || result.outcome === 'running') continue; + this.results.set(key, { ...result, stale: true }); + this.resultsDirty = true; } + } - // Run directly via a TestRun - const run = this.controller.createTestRun({ - include: [classItem], - exclude: [], - profile: undefined, - preserveFocus: false, - }); - await this.runClassTests(session, run, classItem, className, dictName); - run.end(); + // ── Named entry points (Explorer rows, browser menus, code lenses) ── + + /** Run all tests in a named class. */ + async runClassByName( + dictName: string, + className: string, + kind: SunitRunKind = 'run', + ): Promise { + const classItem = await this.ensureClassItem(dictName, className); + if (!classItem) return; + await this.runTestItems([classItem], kind); } /** * Run all tests in the provided class names, all within one dictionary, * using a single TestRun. */ - async runClassesByName(dictName: string, classNames: string[]): Promise { + async runClassesByName( + dictName: string, + classNames: string[], + kind: SunitRunKind = 'run', + ): Promise { + if (!this.requireSession()) return; await this.discoverTests(); const classItems: TestItem[] = this.itemsForClasses(dictName, classNames); - await this.runTestItems(classItems); + await this.runTestItems(classItems, kind); } /** Run all test methods in a method category from browser context menus. */ @@ -146,18 +281,10 @@ export class SunitTestController implements vscode.Disposable { dictName: string, className: string, category: string, + kind: SunitRunKind = 'run', ): Promise { - await this.discoverTests(); - - const classItem = this.findClassItem(dictName, className); - if (!classItem) { - vscode.window.showWarningMessage(this.notATestClassErrorMessage(className)); - return; - } - - if (classItem.children.size === 0) { - await this.resolveTestMethods(classItem); - } + const classItem = await this.ensureClassItem(dictName, className); + if (!classItem) return; const methodItems: TestItem[] = []; classItem.children.forEach((child) => { @@ -166,28 +293,69 @@ export class SunitTestController implements vscode.Disposable { } }); - await this.runTestItems(methodItems); + await this.runTestItems(methodItems, kind); } - /** Run test methods by class/selector from browser context menus. */ - async runTestsByName(dictName: string, className: string, selectors: string[]): Promise { - await this.discoverTests(); + /** Run named test methods of one class. */ + async runTestsByName( + dictName: string, + className: string, + selectors: string[], + kind: SunitRunKind = 'run', + ): Promise { + const classItem = await this.ensureClassItem(dictName, className); + if (!classItem) return; + + const methodItems = selectors + .map((selector) => this.itemForMethodNamed(classItem, selector)) + .filter((result) => result !== undefined); + + await this.runTestItems(methodItems, kind); + } + + /** + * The class item for a named class, with its methods resolved — discovering + * first if it isn't known yet. Reports to the user and answers undefined when + * the class isn't a test class, so every named entry point above fails the + * same way. + */ + private async ensureClassItem( + dictName: string, + className: string, + ): Promise { + if (!this.requireSession()) return undefined; + + let classItem = this.findClassItem(dictName, className); + if (!classItem) { + await this.discoverTests(); + classItem = this.findClassItem(dictName, className); + } - const classItem = this.findClassItem(dictName, className); if (!classItem) { vscode.window.showWarningMessage(this.notATestClassErrorMessage(className)); - return; + return undefined; } if (classItem.children.size === 0) { await this.resolveTestMethods(classItem); } - const methodItems = selectors - .map((selector) => this.itemForMethodNamed(classItem, selector)) - .filter((result) => result !== undefined); + return classItem; + } - await this.runTestItems(methodItems); + /** + * The selected session, or undefined after telling the user there isn't one. + * Checked before anything else so a logged-out user is told that, rather than + * that their class "is not a test class" — which is what an empty discovery + * would otherwise look like. + */ + private requireSession(): ActiveSession | undefined { + const session = this.sessionManager.getSelectedSession(); + if (!session) { + vscode.window.showErrorMessage('No active GemStone session.'); + return undefined; + } + return session; } public notATestClassErrorMessage(className: string) { @@ -222,23 +390,23 @@ export class SunitTestController implements vscode.Disposable { const ambiguous = (nameCounts.get(cls.className) ?? 0) > 1; const label = ambiguous ? `${cls.className} {${cls.dictName}}` : cls.className; - const uri = vscode.Uri.parse( - `gemstone://${session.id}` + - `/${encodeURIComponent(cls.dictName)}` + - `/${encodeURIComponent(cls.className)}` + - `/definition`, - ); + const id = makeClassId(session.id, cls.dictName, cls.className); const classItem = this.controller.createTestItem( - makeClassId(session.id, cls.dictName, cls.className), + id, label, - uri, + this.classDefinitionUri(session.id, cls.dictName, cls.className, cls.dictIndex), ); classItem.canResolveChildren = true; + // A range is what puts the run/status icon in the editor gutter. The + // class definition is its own document, so line 1 is the definition + // itself. + classItem.range = topOfDocument(); // Dimmed qualifier (sidebar only): test count. The dictionary never // goes here — it lives in the label, and only when the name is // ambiguous. A null count means the stone returned an unparseable // value; show "(?)" rather than a misleading "(0)". classItem.description = cls.testCount === null ? '(?)' : `(${cls.testCount})`; + this.classDictIndex.set(id, cls.dictIndex); items.push(classItem); } @@ -258,6 +426,7 @@ export class SunitTestController implements vscode.Disposable { // a same-named class in another dictionary). The label is NOT the class // name — for ambiguous names it carries a " {Dict}" suffix. const { dictName, className } = parseTestId(classItem.id); + const dictIndex = this.classDictIndex.get(classItem.id); try { const methods = sunit.discoverTestMethods(session, className, dictName); @@ -266,19 +435,13 @@ export class SunitTestController implements vscode.Disposable { for (const { selector, category } of methods) { this.methodCategory.set(`${dictName}/${className}/${selector}`, category); - const uri = vscode.Uri.parse( - `gemstone://${session.id}` + - `/${encodeURIComponent(dictName)}` + - `/${encodeURIComponent(className)}` + - `/instance` + - `/${encodeURIComponent(category || 'as yet unclassified')}` + - `/${encodeURIComponent(selector)}`, - ); const methodItem = this.controller.createTestItem( makeMethodId(session.id, dictName, className, selector), selector, - uri, + this.methodUri(session.id, dictName, className, category, selector, dictIndex), ); + // Gutter icon on line 1 of the method's own document (see above). + methodItem.range = topOfDocument(); children.push(methodItem); } @@ -289,11 +452,58 @@ export class SunitTestController implements vscode.Disposable { } } + /** + * The URIs below must be built with the same builders the editor uses, not + * assembled by hand: VS Code matches a test item to an open document by exact + * URI, so a differing query string or a hand-encoded segment silently costs + * the gutter icon. A malformed name (a category containing '/', say) makes the + * builder throw — answer undefined rather than lose the whole discovery pass; + * the item still works everywhere except the gutter. + */ + private classDefinitionUri( + sessionId: number, + dictName: string, + className: string, + dictIndex: number | undefined, + ): vscode.Uri | undefined { + try { + return buildClassDefinitionUri(sessionId, dictName, className, dictIndex); + } catch { + return undefined; + } + } + + private methodUri( + sessionId: number, + dictName: string, + className: string, + category: string, + selector: string, + dictIndex: number | undefined, + ): vscode.Uri | undefined { + try { + return buildMethodUri({ + kind: 'method', + sessionId, + dictName, + className, + isMeta: false, + category: category || 'as yet unclassified', + selector, + environmentId: 0, + dictIndex, + }); + } catch { + return undefined; + } + } + // ── Test Execution ───────────────────────────────────────── private async runTests( request: vscode.TestRunRequest, token: vscode.CancellationToken, + kind: SunitRunKind = 'run', ): Promise { const session = this.sessionManager.getSelectedSession(); if (!session) { @@ -304,23 +514,25 @@ export class SunitTestController implements vscode.Disposable { const run = this.controller.createTestRun(request); const queue = this.getTestsToRun(request); - for (const item of queue) { - if (token.isCancellationRequested) { - run.skipped(item); - continue; - } + try { + for (const item of queue) { + if (token.isCancellationRequested) { + run.skipped(item); + continue; + } - const { dictName, className, selector } = parseTestId(item.id); + const { dictName, className, selector } = parseTestId(item.id); - if (selector === undefined) { - await this.runClassTests(session, run, item, className, dictName); - } else { - run.started(item); - this.runSingleTest(session, run, item, className, selector, dictName); + if (selector === undefined) { + await this.runClassTests(session, run, item, className, dictName, kind); + } else { + await this.runSingleTest(session, run, item, className, selector, dictName, kind); + } } + } finally { + run.end(); + this.flushResultChanges(); } - - run.end(); } private getTestsToRun(request: vscode.TestRunRequest): vscode.TestItem[] { @@ -338,20 +550,24 @@ export class SunitTestController implements vscode.Disposable { return queue.filter((i) => !excluded.has(i.id)); } - private runSingleTest( + private async runSingleTest( session: ActiveSession, run: vscode.TestRun, item: vscode.TestItem, className: string, selector: string, dictName: string, - ): void { + _kind: SunitRunKind, + ): Promise { + run.started(item); + await this.markRunning([item]); + try { const result = sunit.runTestMethod(session, className, selector, dictName); this.reportResult(run, item, result); } catch (e: unknown) { const msg = e instanceof Error ? e.message : String(e); - run.errored(item, new vscode.TestMessage(`Execution error: ${msg}`)); + this.reportError(run, item, `Execution error: ${msg}`); } } @@ -361,6 +577,7 @@ export class SunitTestController implements vscode.Disposable { classItem: vscode.TestItem, className: string, dictName: string, + _kind: SunitRunKind, ): Promise { // Ensure children are resolved if (classItem.children.size === 0) { @@ -370,41 +587,65 @@ export class SunitTestController implements vscode.Disposable { // Mark all children as started run.started(classItem); classItem.children.forEach((child) => run.started(child)); + const running: vscode.TestItem[] = [classItem]; + classItem.children.forEach((child) => running.push(child)); + await this.markRunning(running); try { const results = sunit.runTestClass(session, className, dictName); const resultMap = new Map(results.map((r) => [r.selector, r])); - let allPassed = true; + let passedCount = 0; + let totalCount = 0; classItem.children.forEach((child) => { // Children are always method ids, so selector is present. const selector = parseTestId(child.id).selector!; const result = resultMap.get(selector); if (!result) { - run.skipped(child); + this.reportSkipped(run, child); return; } + totalCount += 1; this.reportResult(run, child, result); - if (result.status !== 'passed') allPassed = false; + if (result.status === 'passed') passedCount += 1; }); + const allPassed = passedCount === totalCount; if (allPassed) { run.passed(classItem); } else { run.failed(classItem, new vscode.TestMessage('Some tests failed.')); } + this.setResult(dictName, className, undefined, { + outcome: allPassed ? 'passed' : 'failed', + passedCount, + totalCount, + }); } catch (e: unknown) { const msg = e instanceof Error ? e.message : String(e); - const errMsg = new vscode.TestMessage(`Execution error: ${msg}`); - run.errored(classItem, errMsg); + this.reportError(run, classItem, `Execution error: ${msg}`); classItem.children.forEach((child) => { - run.errored(child, new vscode.TestMessage(`Class execution error: ${msg}`)); + this.reportError(run, child, `Class execution error: ${msg}`); }); } } + /** + * Show the tests as running before the (blocking) stone call starts. The + * yield matters: the queries are synchronous, so without handing the event + * loop back the spinner would only ever appear after the answer arrived. + */ + private async markRunning(items: vscode.TestItem[]): Promise { + for (const item of items) { + const { dictName, className, selector } = parseTestId(item.id); + this.setResult(dictName, className, selector, { outcome: 'running' }); + } + this.flushResultChanges(); + await new Promise((resolve) => setImmediate(resolve)); + } + private reportResult( run: vscode.TestRun, item: vscode.TestItem, @@ -421,6 +662,30 @@ export class SunitTestController implements vscode.Disposable { run.errored(item, new vscode.TestMessage(result.message), result.durationMs); break; } + this.reportOutcome(item, { + outcome: result.status, + message: result.message, + durationMs: result.durationMs, + }); + } + + private reportError(run: vscode.TestRun, item: vscode.TestItem, message: string): void { + run.errored(item, new vscode.TestMessage(message)); + this.reportOutcome(item, { outcome: 'error', message }); + } + + /** A test the class run didn't report on — it has no current outcome at all, + * so drop any older one rather than leave a result the run didn't produce. */ + private reportSkipped(run: vscode.TestRun, item: vscode.TestItem): void { + run.skipped(item); + const { dictName, className, selector } = parseTestId(item.id); + this.deleteResult(dictName, className, selector); + } + + /** The one place a settled outcome enters the store. */ + private reportOutcome(item: vscode.TestItem, result: SunitResult): void { + const { dictName, className, selector } = parseTestId(item.id); + this.setResult(dictName, className, selector, result); } /** @@ -470,23 +735,33 @@ export class SunitTestController implements vscode.Disposable { return methodItem; } - private async runTestItems(testItems: TestItem[]) { + /** + * The single funnel every named entry point runs through — so a test started + * from an Explorer button, a gutter icon, a code lens, or the browser's + * context menu takes exactly the same path (and gets the same reporting and + * cancellation) as one started from the Test Explorer itself. + */ + private async runTestItems(testItems: TestItem[], kind: SunitRunKind = 'run'): Promise { if (testItems.length === 0) { vscode.window.showWarningMessage(this.noTestsFoundErrorMessage()); return; } - await this.runTests( - { - include: testItems, - exclude: undefined, - preserveFocus: false, - profile: undefined, - continuous: false, - }, - { - isCancellationRequested: false, - } as CancellationToken, - ); + const tokenSource = new vscode.CancellationTokenSource(); + try { + await this.runTests( + { + include: testItems, + exclude: undefined, + preserveFocus: false, + profile: undefined, + continuous: false, + }, + tokenSource.token, + kind, + ); + } finally { + tokenSource.dispose(); + } } } From 66504811bd955d9f82fc1c4fbce383a232c61065 Mon Sep 17 00:00:00 2001 From: Eric Winger Date: Wed, 19 Aug 2026 15:30:11 -0700 Subject: [PATCH 02/15] SUnit: debug a test class or test method through the shared debugger path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second run profile, so a test can be debugged wherever it can be run, and both leave the same mark. The debugger needs the test to run WITHOUT SUnit's exception handler — that handler is what records "this failed" and throws the exception away, and it is why a failing test has been undebuggable. A debug run therefore executes setUp / the test / tearDown directly, so a raise suspends the GemStone process and the debugger gets the live stack. - CodeExecutor gains executeWithDebugger: the same debug-enabled execution Execute It uses (interpreted, same poll loop, same debugger prompt) for a caller with no editor behind it. The SUnit controller reaches it through a narrow interface rather than depending on the executor. - Debugging a class runs its tests one at a time and stops at the first that raises: from that moment a debugger owns the suspended process. Tests after it are left with no result rather than an invented one. - A debugged test records its outcome in the same store an ordinary run writes to, minus a duration — elapsed time under a debugger is the user's stepping time. A raise is reported as raised, not classified as failure-vs-error: SUnit makes that distinction inside the handler a debug run deliberately omits. Toward #427. Co-Authored-By: Claude Opus 5 (1M context) --- client/src/__tests__/codeExecutor.test.ts | 75 +++++++++ client/src/__tests__/debugTestMethod.test.ts | 36 +++++ .../src/__tests__/sunitTestController.test.ts | 94 ++++++++++- client/src/codeExecutor.ts | 59 +++++++ client/src/extension.ts | 12 +- client/src/queries/debugTestMethod.ts | 31 ++++ client/src/sunitTestController.ts | 148 +++++++++++++++++- 7 files changed, 445 insertions(+), 10 deletions(-) create mode 100644 client/src/__tests__/debugTestMethod.test.ts create mode 100644 client/src/queries/debugTestMethod.ts diff --git a/client/src/__tests__/codeExecutor.test.ts b/client/src/__tests__/codeExecutor.test.ts index 4c0b8b4d..08b22e68 100644 --- a/client/src/__tests__/codeExecutor.test.ts +++ b/client/src/__tests__/codeExecutor.test.ts @@ -1619,4 +1619,79 @@ describe('CodeExecutor', () => { expect(diags[0].message).toContain('no socket'); }); }); + describe('executeWithDebugger', () => { + // The debug-execution primitive a SUnit test run borrows: no editor, no + // result rendering — just "run this with the debugger enabled and tell me + // whether it raised". + it('runs the code with the debugger enabled and answers that it did not raise', async () => { + const gci = makeGci(); + const session = makeSession(gci); + const executor = new CodeExecutor(makeSessionManager(session)); + + const outcome = await executor.executeWithDebugger(session, '3 + 4', 'MyTest>>testAdd'); + + expect(outcome).toEqual({ raised: false }); + const flags = (gci.GciTsNbExecute as Mock).mock.calls[0][5] as number; + expect(flags & GCI_PERFORM_FLAG_ENABLE_DEBUG).toBe(GCI_PERFORM_FLAG_ENABLE_DEBUG); + expect((gci.GciTsNbExecute as Mock).mock.calls[0][1]).toBe('3 + 4'); + }); + + it('needs no active editor — a test is debugged from a row, not from text', async () => { + (vscode.window as unknown as Record).activeTextEditor = undefined; + const session = makeSession(); + const executor = new CodeExecutor(makeSessionManager(session)); + + await expect( + executor.executeWithDebugger(session, '3 + 4', 'MyTest>>testAdd'), + ).resolves.toEqual({ raised: false }); + }); + + it('offers the suspended process to a debugger and reports that it raised', async () => { + const gci = makeGci({ + GciTsNbResult: vi.fn(() => ({ + result: 0n, + err: { number: 2010, message: 'doesNotUnderstand: #foo', context: 999n }, + })), + }); + const session = makeSession(gci); + const executor = new CodeExecutor(makeSessionManager(session)); + + const outcome = await executor.executeWithDebugger(session, '3 + 4', 'MyTest>>testAdd'); + + expect(outcome).toEqual({ raised: true, message: 'doesNotUnderstand: #foo' }); + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + expect.stringContaining('doesNotUnderstand: #foo'), + { modal: true }, + 'Enhanced Debug', + 'Debug', + ); + }); + + it('releases the session lock so the next test can run', async () => { + const session = makeSession(); + const executor = new CodeExecutor(makeSessionManager(session)); + + await executor.executeWithDebugger(session, '3 + 4', 'MyTest>>testAdd'); + await executor.executeWithDebugger(session, '3 + 4', 'MyTest>>testRemove'); + + expect((session.gci.GciTsNbExecute as Mock).mock.calls).toHaveLength(2); + }); + + it('refuses to start on a session that is already executing', async () => { + const session = makeSession(); + const executor = new CodeExecutor(makeSessionManager(session)); + // Hold the first execution open so the session is genuinely busy. + (session.gci.GciTsNbPoll as Mock).mockReturnValue({ result: 0, err: { number: 0 } }); + const pending = executor.executeWithDebugger(session, '3 + 4', 'MyTest>>testAdd'); + + await expect( + executor.executeWithDebugger(session, '3 + 4', 'MyTest>>testRemove'), + ).rejects.toThrow(/already in progress/); + + // Let it finish: a poll loop still spinning at the end of this test would + // be drained by whichever fake-timer test runs next. + (session.gci.GciTsNbPoll as Mock).mockReturnValue({ result: 1, err: { number: 0 } }); + await pending; + }); + }); }); diff --git a/client/src/__tests__/debugTestMethod.test.ts b/client/src/__tests__/debugTestMethod.test.ts new file mode 100644 index 00000000..c5913f3d --- /dev/null +++ b/client/src/__tests__/debugTestMethod.test.ts @@ -0,0 +1,36 @@ +import { describe, it, expect } from 'vitest'; + +import { debugTestMethodCode } from '../queries/debugTestMethod'; + +describe('debugTestMethodCode', () => { + // The whole reason a debug run can't reuse the ordinary run queries is that + // those install an exception handler. If one ever appears here, a failing + // test silently stops being debuggable — hence the explicit assertions. + it('installs no exception handler around the test', () => { + const code = debugTestMethodCode('MyTestCase', 'testAdd', 'UserGlobals'); + expect(code).not.toContain('on: AbstractException'); + expect(code).not.toContain('TestFailure'); + }); + + it('runs setUp, the test, and tearDown', () => { + const code = debugTestMethodCode('MyTestCase', 'testAdd', 'UserGlobals'); + expect(code).toContain('tc setUp'); + expect(code).toContain("tc perform: #'testAdd'"); + expect(code).toContain('ensure: [tc tearDown]'); + }); + + it('resolves the class in the dictionary it was found in', () => { + const code = debugTestMethodCode('MyTestCase', 'testAdd', 'UserGlobals'); + expect(code).toContain('UserGlobals'); + }); + + it('answers a value when nothing raised, so a pass is distinguishable', () => { + const code = debugTestMethodCode('MyTestCase', 'testAdd', 'UserGlobals'); + expect(code.trimEnd().endsWith("'passed'")).toBe(true); + }); + + it('escapes a quote in the selector rather than breaking the literal', () => { + const code = debugTestMethodCode('MyTestCase', "test'Odd", 'UserGlobals'); + expect(code).toContain("#'test''Odd'"); + }); +}); diff --git a/client/src/__tests__/sunitTestController.test.ts b/client/src/__tests__/sunitTestController.test.ts index 1cf41d25..07323211 100644 --- a/client/src/__tests__/sunitTestController.test.ts +++ b/client/src/__tests__/sunitTestController.test.ts @@ -37,8 +37,8 @@ vi.mock('../sunitQueries', () => ({ }, })); -import { tests, window } from '../__mocks__/vscode'; -import { SunitTestController } from '../sunitTestController'; +import { tests, window, TestRunProfileKind } from '../__mocks__/vscode'; +import { SunitTestController, SunitDebugOutcome } from '../sunitTestController'; import { SessionManager } from '../sessionManager'; import * as sunit from '../sunitQueries'; @@ -718,6 +718,96 @@ describe('SunitTestController', () => { }); }); + describe('debugging a test', () => { + // The debugger needs the test to run WITHOUT SUnit's exception handler, so a + // debug run can't go through runTestClass/runTestMethod. Everything else — + // which items run, and where the outcome is recorded — must stay identical. + function makeDebugExecutor(...outcomes: SunitDebugOutcome[]) { + const queued = [...outcomes]; + const executeWithDebugger = vi.fn( + async (_session: unknown, _code: string, _label: string): Promise => + queued.shift() ?? { raised: false }, + ); + return { executeWithDebugger }; + } + + it('offers a Debug profile alongside the Run profile', () => { + const ctrl = new SunitTestController(makeSessionManager(true), makeDebugExecutor()); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + + const kinds = (mockController.createRunProfile as ReturnType).mock.calls.map( + (call: unknown[]) => call[1], + ); + + expect(kinds).toEqual([TestRunProfileKind.Run, TestRunProfileKind.Debug]); + ctrl.dispose(); + }); + + it('runs the test through the debug executor, not the ordinary run query', async () => { + const debugExecutor = makeDebugExecutor(); + const ctrl = new SunitTestController(makeSessionManager(true), debugExecutor); + + await ctrl.runTestsByName('UserGlobals', 'MyTestCase', ['testAdd'], 'debug'); + + expect(sunit.runTestMethod).not.toHaveBeenCalled(); + expect(debugExecutor.executeWithDebugger).toHaveBeenCalledOnce(); + const code = debugExecutor.executeWithDebugger.mock.calls[0][1]; + expect(code).toContain('MyTestCase'); + expect(code).toContain('testAdd'); + ctrl.dispose(); + }); + + it('debugs a class one test at a time, not through the suite run', async () => { + const debugExecutor = makeDebugExecutor(); + const ctrl = new SunitTestController(makeSessionManager(true), debugExecutor); + + await ctrl.runClassByName('UserGlobals', 'MyTestCase', 'debug'); + + // runTestClass installs the handler that makes a failure undebuggable. + expect(sunit.runTestClass).not.toHaveBeenCalled(); + expect(debugExecutor.executeWithDebugger).toHaveBeenCalledTimes(2); + ctrl.dispose(); + }); + + it('stops a class debug at the first test that raises', async () => { + const debugExecutor = makeDebugExecutor({ raised: true, message: 'boom' }); + const ctrl = new SunitTestController(makeSessionManager(true), debugExecutor); + + await ctrl.runClassByName('UserGlobals', 'MyTestCase', 'debug'); + + // A debugger now owns the suspended process; running the next test on top + // of it would be nonsense. + expect(debugExecutor.executeWithDebugger).toHaveBeenCalledOnce(); + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testAdd')).toMatchObject({ + outcome: 'error', + message: 'boom', + }); + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testRemove')).toBeUndefined(); + ctrl.dispose(); + }); + + it('records a debugged pass in the same store an ordinary run writes to', async () => { + const ctrl = new SunitTestController(makeSessionManager(true), makeDebugExecutor()); + + await ctrl.runTestsByName('UserGlobals', 'MyTestCase', ['testAdd'], 'debug'); + + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testAdd')).toMatchObject({ + outcome: 'passed', + }); + ctrl.dispose(); + }); + + it('leaves no duration on a debugged test — the elapsed time is stepping time', async () => { + const ctrl = new SunitTestController(makeSessionManager(true), makeDebugExecutor()); + + await ctrl.runTestsByName('UserGlobals', 'MyTestCase', ['testAdd'], 'debug'); + + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testAdd')?.durationMs).toBeUndefined(); + ctrl.dispose(); + }); + }); + describe('dispose', () => { it('disposes the controller', () => { const sm = makeSessionManager(true); diff --git a/client/src/codeExecutor.ts b/client/src/codeExecutor.ts index e35a0f8d..de270f27 100644 --- a/client/src/codeExecutor.ts +++ b/client/src/codeExecutor.ts @@ -468,6 +468,65 @@ export class CodeExecutor { vscode.window.setStatusBarMessage('GemStone: Result copied to clipboard.', 2000); } + /** + * Execute `code` with the GemStone debugger enabled, for a caller that has no + * editor behind it — a SUnit test started from a tree row, a gutter icon or a + * code lens. Deliberately the same execution path as Execute It (interpreted, + * debug-enabled, same non-blocking poll, same debugger prompt), so a test that + * halts behaves exactly like halted workspace code. + * + * Answers whether the code raised — and no more than that. Once the process + * is suspended it belongs to whichever debugger the user picked, so its + * eventual fate isn't ours to report. Throws when the execution could not be + * started at all. + */ + async executeWithDebugger( + session: ActiveSession, + code: string, + label: string, + ): Promise<{ raised: boolean; message?: string }> { + if (this.executing.has(session.id)) { + throw new Error('A GemStone execution is already in progress on this session.'); + } + + const oopClassString = this.resolveUtf8ClassOopUsing(session); + this.setExecuting(session.id, true); + appendTranscriptOutput(setTranscriptLive(session, true)); + try { + const { success, err: startErr } = session.gci.GciTsNbExecute( + session.handle, + code, + oopClassString, + OOP_ILLEGAL, + OOP_NIL, + GCI_PERFORM_FLAG_ENABLE_DEBUG | GCI_PERFORM_FLAG_INTERPRETED, + 0, + ); + if (!success) { + throw new Error(startErr.message || `GemStone error ${startErr.number}`); + } + + await this.pollForResultOop(session); + return { raised: false }; + } catch (e: unknown) { + // A cancelled (hard-break) run tells us nothing about the test, so it is + // reported as "raised" rather than passed off as a pass. + if (e instanceof NbCancelledError) return { raised: true, message: 'Execution cancelled.' }; + + const msg = e instanceof Error ? e.message : String(e); + logError(session.id, `${label}: ${msg}`); + + if (e instanceof DebuggableError) { + await this.promptDebuggableError(session, e.context, `${label} — ${msg}`); + return { raised: true, message: msg }; + } + throw e instanceof Error ? e : new Error(msg); + } finally { + appendTranscriptOutput(setTranscriptLive(session, false)); + this.setExecuting(session.id, false); + } + } + /** Show the full most recent Display It result in the Output panel. */ outputLastResult(): void { if (this.lastResult === null) { diff --git a/client/src/extension.ts b/client/src/extension.ts index 92a362b9..21936fb8 100644 --- a/client/src/extension.ts +++ b/client/src/extension.ts @@ -943,8 +943,14 @@ export function activate(context: vscode.ExtensionContext) { }), ); + // ── Code Execution ───────────────────────────────────── + // Constructed before the SUnit controller, which borrows its debug-enabled + // execution path to run a single test under the debugger. + const codeExecutor = new CodeExecutor(sessionManager); + context.subscriptions.push(codeExecutor); + // ── SUnit Test Controller ──────────────────────────────── - const sunitTestController = new SunitTestController(sessionManager); + const sunitTestController = new SunitTestController(sessionManager, codeExecutor); context.subscriptions.push(sunitTestController); // Keep the pass/fail indicators honest. A compiled method or class definition @@ -971,10 +977,6 @@ export function activate(context: vscode.ExtensionContext) { const smalltalkNotebookController = new SmalltalkNotebookController(sessionManager); context.subscriptions.push(smalltalkNotebookController); - // ── Code Execution ───────────────────────────────────── - const codeExecutor = new CodeExecutor(sessionManager); - context.subscriptions.push(codeExecutor); - // ── Status Bar: Active Session ───────────────────────── const statusBarItem = vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Right, 100); statusBarItem.command = 'gemstone.selectSession'; diff --git a/client/src/queries/debugTestMethod.ts b/client/src/queries/debugTestMethod.ts new file mode 100644 index 00000000..f259cf77 --- /dev/null +++ b/client/src/queries/debugTestMethod.ts @@ -0,0 +1,31 @@ +import { classLookupOrRaiseExpr, escapeString } from './util'; + +/** + * Smalltalk that runs ONE test the way a debugger needs it run: setUp, the test + * itself, then tearDown — with no exception handler anywhere. + * + * This is why a debug run cannot reuse runTestMethod / runTestClass. Both of + * those (like SUnit's own TestCase>>run) wrap the test in a handler that + * records "this failed" and discards the exception, which is exactly what makes + * a failing test undebuggable. Without a handler, a raise suspends the GemStone + * process instead, and the debugger gets the live stack. + * + * tearDown runs through `ensure:` so a suspended — then terminated — test still + * tears its fixture down. + * + * Answers 'passed' when nothing raised: the caller otherwise has no way to tell + * "the test finished" from "a debugger took the process". + */ +export function debugTestMethodCode( + className: string, + selector: string, + dictName?: string, +): string { + const sel = escapeString(selector); + return `| cls tc | +${classLookupOrRaiseExpr(className, dictName)} +tc := cls selector: #'${sel}'. +tc setUp. +[tc perform: #'${sel}'] ensure: [tc tearDown]. +'passed'`; +} diff --git a/client/src/sunitTestController.ts b/client/src/sunitTestController.ts index f6c79c61..a63f3bfa 100644 --- a/client/src/sunitTestController.ts +++ b/client/src/sunitTestController.ts @@ -1,6 +1,7 @@ import type { TestItem } from 'vscode'; import * as vscode from 'vscode'; import { buildClassDefinitionUri, buildMethodUri } from './gemstoneFileSystemProvider'; +import { debugTestMethodCode } from './queries/debugTestMethod'; import { ActiveSession, SessionManager } from './sessionManager'; import * as sunit from './sunitQueries'; @@ -49,6 +50,28 @@ function parseTestId(id: string): ParsedTestId { */ export type SunitRunKind = 'run' | 'debug'; +/** + * What the controller needs in order to run one test under the GemStone + * debugger. Narrower than the whole code executor on purpose: the two share the + * one debug-enabled execution path (suspend on error, hand the process to a + * debugger) without the controller depending on the rest of it. + */ +export interface SunitDebugExecutor { + executeWithDebugger( + session: ActiveSession, + code: string, + label: string, + ): Promise; +} + +/** What became of one test run under the debugger. */ +export interface SunitDebugOutcome { + /** True when the test raised and the suspended process went to a debugger. */ + raised: boolean; + /** The GemStone error message, when it raised. */ + message?: string; +} + /** State of the most recent run of one test class or test method. */ export type SunitOutcome = 'running' | 'passed' | 'failed' | 'error'; @@ -117,7 +140,15 @@ export class SunitTestController implements vscode.Disposable { */ readonly onDidChangeResults = this._onDidChangeResults.event; - constructor(private sessionManager: SessionManager) { + constructor( + private sessionManager: SessionManager, + /** + * Absent only where nothing can be debugged anyway (tests of the run path). + * The extension always supplies one, so the Debug profile is always there + * for a user. + */ + private debugExecutor?: SunitDebugExecutor, + ) { this.controller = vscode.tests.createTestController('gemstone-sunit', 'GemStone SUnit Tests'); this.controller.resolveHandler = async (item) => { @@ -135,6 +166,15 @@ export class SunitTestController implements vscode.Disposable { true, ); + if (this.debugExecutor) { + this.controller.createRunProfile( + 'Debug Tests', + vscode.TestRunProfileKind.Debug, + (request, token) => this.runTests(request, token, 'debug'), + true, + ); + } + this.controller.refreshHandler = async () => { this.resetDiscovery(); await this.discoverTests(); @@ -557,11 +597,16 @@ export class SunitTestController implements vscode.Disposable { className: string, selector: string, dictName: string, - _kind: SunitRunKind, + kind: SunitRunKind, ): Promise { run.started(item); await this.markRunning([item]); + if (kind === 'debug' && this.debugExecutor) { + await this.debugSingleTest(session, run, item, className, selector, dictName); + return; + } + try { const result = sunit.runTestMethod(session, className, selector, dictName); this.reportResult(run, item, result); @@ -571,19 +616,62 @@ export class SunitTestController implements vscode.Disposable { } } + /** + * Run one test under the debugger. Reports into the same store as an ordinary + * run — the whole point is that a test debugged from a row or a gutter icon + * leaves the same mark as one that was merely run. + */ + private async debugSingleTest( + session: ActiveSession, + run: vscode.TestRun, + item: vscode.TestItem, + className: string, + selector: string, + dictName: string, + ): Promise { + try { + const outcome = await this.debugExecutor!.executeWithDebugger( + session, + debugTestMethodCode(className, selector, dictName), + `${className}>>${selector}`, + ); + + if (!outcome.raised) { + this.reportPassed(run, item); + return false; + } + + // Once the process is suspended and a debugger owns it, we no longer know + // whether an assertion failed or something else was raised — SUnit does + // that classification inside the handler a debug run deliberately omits. + // Report the honest "it raised", with the message, rather than guessing. + this.reportError(run, item, outcome.message ?? 'Test raised an exception.'); + return true; + } catch (e: unknown) { + const msg = e instanceof Error ? e.message : String(e); + this.reportError(run, item, `Execution error: ${msg}`); + return true; + } + } + private async runClassTests( session: ActiveSession, run: vscode.TestRun, classItem: vscode.TestItem, className: string, dictName: string, - _kind: SunitRunKind, + kind: SunitRunKind, ): Promise { // Ensure children are resolved if (classItem.children.size === 0) { await this.resolveTestMethods(classItem); } + if (kind === 'debug' && this.debugExecutor) { + await this.debugClassTests(session, run, classItem, className, dictName); + return; + } + // Mark all children as started run.started(classItem); classItem.children.forEach((child) => run.started(child)); @@ -632,6 +720,53 @@ export class SunitTestController implements vscode.Disposable { } } + /** + * Debug every test in a class, one at a time — a suite run would install the + * handler that makes a failure undebuggable — and stop at the first test that + * raises, because from that moment a debugger owns the suspended process and + * running the next test on top of it would be nonsense. Tests after that one + * are left with no result rather than a made-up one. + */ + private async debugClassTests( + session: ActiveSession, + run: vscode.TestRun, + classItem: vscode.TestItem, + className: string, + dictName: string, + ): Promise { + const children: vscode.TestItem[] = []; + classItem.children.forEach((child) => children.push(child)); + + run.started(classItem); + await this.markRunning([classItem]); + + let passedCount = 0; + for (const child of children) { + const selector = parseTestId(child.id).selector!; + run.started(child); + await this.markRunning([child]); + + const raised = await this.debugSingleTest(session, run, child, className, selector, dictName); + if (raised) { + run.errored(classItem, new vscode.TestMessage(`${selector} raised.`)); + this.setResult(dictName, className, undefined, { + outcome: 'error', + passedCount, + totalCount: children.length, + }); + return; + } + passedCount += 1; + } + + run.passed(classItem); + this.setResult(dictName, className, undefined, { + outcome: 'passed', + passedCount, + totalCount: children.length, + }); + } + /** * Show the tests as running before the (blocking) stone call starts. The * yield matters: the queries are synchronous, so without handing the event @@ -669,6 +804,13 @@ export class SunitTestController implements vscode.Disposable { }); } + /** A pass with no measured duration — a debugged test's elapsed time is the + * user's stepping time, which would be a lie on the row. */ + private reportPassed(run: vscode.TestRun, item: vscode.TestItem): void { + run.passed(item); + this.reportOutcome(item, { outcome: 'passed' }); + } + private reportError(run: vscode.TestRun, item: vscode.TestItem, message: string): void { run.errored(item, new vscode.TestMessage(message)); this.reportOutcome(item, { outcome: 'error', message }); From 2720083f134fcf7a96ed5cd80d4fc1e91c37a13d Mon Sep 17 00:00:00 2001 From: Eric Winger Date: Thu, 20 Aug 2026 15:48:10 -0700 Subject: [PATCH 03/15] URI: let a method category carry a slash, like a selector already can MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `initialize/release` is a stock GemStone method category, and buildMethodUri asserted the category was slash-free. Building a row for any method in such a category therefore threw — surfacing as a "Method category name must not contain '/'" toast and taking the whole Methods pane's render down with it, and leaving those methods with no URI to open at all. The category now rides in the path through the same FRACTION SLASH sentinel the selector has always used, and parseUri reverses it. The category stays one path segment, so the selector still begins where the parser expects. Dictionary and class names keep the assertion: those genuinely cannot contain a slash, so one there is a caller's bug rather than a name to carry. Co-Authored-By: Claude Opus 5 (1M context) --- .../gemstoneFileSystemProvider.test.ts | 43 +++++++++++-------- client/src/gemstoneFileSystemProvider.ts | 12 ++++-- 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/client/src/__tests__/gemstoneFileSystemProvider.test.ts b/client/src/__tests__/gemstoneFileSystemProvider.test.ts index 121d4525..0713c47c 100644 --- a/client/src/__tests__/gemstoneFileSystemProvider.test.ts +++ b/client/src/__tests__/gemstoneFileSystemProvider.test.ts @@ -1403,19 +1403,28 @@ describe('buildMethodUri', () => { ).toThrow("Class name must not contain '/': My/Class"); }); - it('throws when category contains a slash', () => { - expect(() => - buildMethodUri({ - kind: 'method', - sessionId: 1, - dictName: 'Globals', - className: 'Array', - isMeta: false, - category: 'accessing/stuff', - selector: 'at', - environmentId: 0, - }), - ).toThrow("Method category name must not contain '/': accessing/stuff"); + it('carries a slash-bearing category through the path and back', () => { + // `initialize/release` is a stock GemStone category. Rejecting it threw from + // anywhere a row for such a method was built, taking the Methods pane with it. + const uri = buildMethodUri({ + kind: 'method', + sessionId: 1, + dictName: 'Globals', + className: 'Array', + isMeta: false, + category: 'initialize/release', + selector: 'at', + environmentId: 0, + }); + + // Escaped in the path, so the category stays one segment and the selector + // still starts at the segment after it. + expect(uri.path).not.toContain('initialize/release'); + expect(parseUri(uri)).toMatchObject({ + kind: 'method', + category: 'initialize/release', + selector: 'at', + }); }); it('does not throw for a raw binary selector containing a slash', () => { @@ -1506,10 +1515,10 @@ describe('buildNewMethodUri', () => { ); }); - it('throws when category contains a slash', () => { - expect(() => buildNewMethodUri(1, 'Globals', 'Array', false, 'accessing/stuff', 0)).toThrow( - "Method category name must not contain '/': accessing/stuff", - ); + it('carries a slash-bearing category through the path and back', () => { + expect( + parseUri(buildNewMethodUri(1, 'Globals', 'Array', false, 'initialize/release', 0)), + ).toMatchObject({ category: 'initialize/release' }); }); }); diff --git a/client/src/gemstoneFileSystemProvider.ts b/client/src/gemstoneFileSystemProvider.ts index bcee9264..6415ad86 100644 --- a/client/src/gemstoneFileSystemProvider.ts +++ b/client/src/gemstoneFileSystemProvider.ts @@ -131,7 +131,7 @@ export function parseUri(uri: vscode.Uri): ParsedUri { dictName: parts[1], className: parts[2], isMeta: parts[3] === 'class', - category: parts[4], + category: unescapeSelectorSlashes(parts[4]), environmentId, dictIndex, }; @@ -154,7 +154,7 @@ export function parseUri(uri: vscode.Uri): ParsedUri { dictName: parts[1], className: parts[2], isMeta: parts[3] === 'class', - category: parts[4], + category: unescapeSelectorSlashes(parts[4]), selector: labelled ? labelled[1] : rawSelector, environmentId, base, @@ -274,7 +274,11 @@ export function buildClassCommentUri( export function buildMethodUri(parsedUri: ParsedMethodUri): vscode.Uri { assertIsValidUriPath('Dictionary name', parsedUri.dictName); assertIsValidUriPath('Class name', parsedUri.className); - assertIsValidUriPath('Method category name', parsedUri.category); + // A method category legitimately contains '/' — `initialize/release` is a stock + // GemStone one — so it rides in the path through the same slash sentinel the + // selector uses, rather than being rejected. Asserting instead threw from + // anywhere a row for such a method was built, which surfaced as a toast and + // took the whole Methods pane down with it. // The selector is NOT asserted slash-free: '/' and '//' are ordinary binary // selectors. Escape any slashes to the sentinel so they survive the path // (parseUri reverses it). Idempotent, so callers that pre-escape stay correct. @@ -287,7 +291,7 @@ export function buildMethodUri(parsedUri: ParsedMethodUri): vscode.Uri { return vscode.Uri.from({ scheme: 'gemstone', authority: String(parsedUri.sessionId), - path: `/${parsedUri.dictName}/${parsedUri.className}/${side}/${parsedUri.category}/${escapeSelectorSlashes(parsedUri.selector)}`, + path: `/${parsedUri.dictName}/${parsedUri.className}/${side}/${escapeSelectorSlashes(parsedUri.category)}/${escapeSelectorSlashes(parsedUri.selector)}`, query: params.join('&'), }); } From 4c338e7f66bc11c305f5e55865624e4c71f78493 Mon Sep 17 00:00:00 2001 From: Eric Winger Date: Thu, 20 Aug 2026 15:48:28 -0700 Subject: [PATCH 04/15] Non-blocking calls: survive a cancel, and a session that goes away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in the shared poll runner, all of which made a long call look broken rather than stopped. They affect every cancellable call — Execute It and a Rowan load as much as a test run. - A hard break abandons the call but does not end it: until something collects the result, the session still reports a call in progress and refuses the next one. So the FOLLOWING call failed with "session is busy", which reads as the next run silently doing nothing. The abandoned result is now drained in the background. - A logout (or a lost connection) while a call was outstanding left the poll reporting "not ready" for good — the progress notification sat there claiming work was in flight and the awaiting caller never heard back. The poll now asks GciTsCallInProgress whether the session is still there to answer, and settles when it isn't. - executeFetchStringNb gains an `onStart` pass-through, so a caller can drive the break from its own UI instead of only the ~2s notification. Each break is now logged with what GciTsBreak answered. A break the gem ignores and a break that was never sent are indistinguishable from outside, and that difference is the whole diagnosis when a stop button appears to do nothing. Co-Authored-By: Claude Opus 5 (1M context) --- client/src/__tests__/codeExecutor.test.ts | 2 + client/src/__tests__/nbRunner.test.ts | 26 +++++++++ client/src/browserQueries.ts | 6 +- client/src/nbRunner.ts | 71 ++++++++++++++++++++++- 4 files changed, 101 insertions(+), 4 deletions(-) diff --git a/client/src/__tests__/codeExecutor.test.ts b/client/src/__tests__/codeExecutor.test.ts index 08b22e68..863dd8bf 100644 --- a/client/src/__tests__/codeExecutor.test.ts +++ b/client/src/__tests__/codeExecutor.test.ts @@ -56,6 +56,8 @@ function makeGci(overrides: Record = {}) { err: { number: 0, message: '' }, })), GciTsNbPoll: vi.fn(() => ({ result: 1, err: { number: 0 } })), + // The poll checks the session is still there to answer; -1 would mean it is gone. + GciTsCallInProgress: vi.fn(() => ({ result: 0, err: { number: 0 } })), isAvailable: vi.fn(() => true), GciTsSocket: vi.fn(() => ({ fd: 7, err: { number: 0 } })), GciTsNbResult: vi.fn((): Record => ({ diff --git a/client/src/__tests__/nbRunner.test.ts b/client/src/__tests__/nbRunner.test.ts index d34fde2a..618305a8 100644 --- a/client/src/__tests__/nbRunner.test.ts +++ b/client/src/__tests__/nbRunner.test.ts @@ -23,6 +23,10 @@ function makeSession(pollResults: { result: number; err?: unknown }[]): ActiveSe }), GciTsBreak: vi.fn(() => ({ result: 0, err: noErr })), GciTsSocket: vi.fn(() => ({ fd: 3, err: noErr })), + // 0 = the session is there and idle-ish; -1 would mean it has gone away, which + // the poll treats as "nobody is coming to answer this call". + GciTsCallInProgress: vi.fn(() => ({ result: 0, err: noErr })), + GciTsNbResult: vi.fn(() => ({ result: 0, err: noErr })), }; return { id: 1, handle: { h: 1 }, gci } as unknown as ActiveSession; } @@ -233,3 +237,25 @@ describe('runNbCall — notification suppression', () => { } }); }); + +describe('a session that goes away mid-call', () => { + it('settles instead of polling forever', async () => { + // A logout (or a lost connection) while a call is outstanding used to leave the + // poll reporting "not ready" for good: the progress notification sat there + // claiming work was in flight and the awaiting caller never heard back. + const session = makeSession([{ result: 0 }, { result: 0 }]); + (session.gci.GciTsCallInProgress as ReturnType).mockReturnValue({ + result: -1, + err: { number: 4100, message: 'session not logged in' }, + }); + + await expect( + runNbCall( + session, + () => ({ success: true, err: noErr as never }), + () => 'never', + { suppressNotification: true }, + ), + ).rejects.toThrow(/session not logged in/); + }); +}); diff --git a/client/src/browserQueries.ts b/client/src/browserQueries.ts index c52ab273..ee527fc7 100644 --- a/client/src/browserQueries.ts +++ b/client/src/browserQueries.ts @@ -323,6 +323,10 @@ export async function executeFetchStringNb( code: string, progressTitle?: string, suppressNotification = false, + // Handed a `cancel` fn once polling starts — soft break on the first call, hard + // on the second. Lets a caller drive the break from its own UI (the Testing + // view's stop button, an Explorer row's ■) rather than only the ~2s toast. + onStart?: (cancel: () => void) => void, ): Promise { const { result: inProgress } = session.gci.GciTsCallInProgress(session.handle); if (inProgress !== 0) { @@ -352,7 +356,7 @@ export async function executeFetchStringNb( } return fetched.data; }, - { title: progressTitle ?? `GemStone: ${label}…`, suppressNotification }, + { title: progressTitle ?? `GemStone: ${label}…`, suppressNotification, onStart }, ); return data; diff --git a/client/src/nbRunner.ts b/client/src/nbRunner.ts index 20339d36..0e068343 100644 --- a/client/src/nbRunner.ts +++ b/client/src/nbRunner.ts @@ -2,6 +2,7 @@ import * as vscode from 'vscode'; import { ActiveSession } from './sessionManager'; import { GciError } from './gciLibrary'; import { pollReadable } from './socketPoll'; +import { logInfo } from './gciLog'; /** * Shared non-blocking GCI call runner. @@ -97,6 +98,34 @@ export interface NbRunOptions { * notification so the user can see it registered; a second sends a hard break * and rejects with `NbCancelledError`. */ +/** How long to keep collecting the result of a hard-broken call, and how often. */ +const DRAIN_ATTEMPTS = 40; +const DRAIN_INTERVAL_MS = 50; + +/** + * Collect and discard the result of a call we gave up on, so the session goes + * back to idle. + * + * A hard break stops the gem but does not, by itself, end the GCI call: until + * something reads its result the session reports a call in progress and refuses + * the next one. Nothing here cares what the result was — only that it has been + * taken. Gives up after a bounded number of attempts rather than polling a + * session that is never going to answer. + */ +function drainAbandonedCall(session: ActiveSession, attempt = 0): void { + try { + const { result } = pollNbResultReady(session); + if (result === 1) { + session.gci.GciTsNbResult(session.handle); + return; + } + if (result === -1 || attempt >= DRAIN_ATTEMPTS) return; + } catch { + return; + } + setTimeout(() => drainAbandonedCall(session, attempt + 1), DRAIN_INTERVAL_MS); +} + export function pollNbToCompletion( session: ActiveSession, onReady: () => T | Promise, @@ -131,13 +160,32 @@ export function pollNbToCompletion( // canceller handed out via opts.onStart. First call asks the gem to stop at a // safe point; a second interrupts now and gives up on the call. const requestCancel = (): void => { - if (settled) return; + if (settled) { + logInfo(`[Session ${session.id}] Break requested, but the call had already settled.`); + return; + } if (!softBreakSent) { - session.gci.GciTsBreak(session.handle, false); + // Logged because a break that the gem ignores is indistinguishable, from + // the outside, from a break that was never sent — and the difference is + // the whole diagnosis when a stop button appears to do nothing. + const { success, err } = session.gci.GciTsBreak(session.handle, false); + logInfo( + `[Session ${session.id}] Soft break sent: success=${success}` + + (err?.number ? ` err=${err.number} ${err.message ?? ''}` : ''), + ); softBreakSent = true; progressReport?.({ message: 'Soft break sent — waiting for the gem to stop…' }); } else { - session.gci.GciTsBreak(session.handle, true); + const { success, err } = session.gci.GciTsBreak(session.handle, true); + logInfo( + `[Session ${session.id}] Hard break sent: success=${success}` + + (err?.number ? ` err=${err.number} ${err.message ?? ''}` : ''), + ); + // A hard break abandons the call, but the session still counts it as in + // progress until its (aborted) result is collected. Drain it, or the very + // next call on this session is refused with "session is busy" — which + // reads as the next run silently doing nothing. + drainAbandonedCall(session); settle(() => reject(new NbCancelledError())); } }; @@ -169,6 +217,23 @@ export function pollNbToCompletion( return; } + // The call has not answered — but check the session is still there to answer. + // A logout (or a lost connection) while a call is outstanding leaves the poll + // reporting "not ready" forever: the progress notification would sit there + // claiming work is in flight, and whatever awaited this promise would never + // hear back. GciTsCallInProgress answers -1 for a session that is gone. + const { result: alive, err: aliveErr } = session.gci.GciTsCallInProgress(session.handle); + if (alive === -1) { + settle(() => + reject( + new Error( + aliveErr?.message || 'The GemStone session ended while this call was still running.', + ), + ), + ); + return; + } + const interval = pollIndex < BACKOFF_INTERVALS.length ? BACKOFF_INTERVALS[pollIndex] : MAX_INTERVAL; pollIndex++; From 5a90050085cace76ae31fab387da6e90bb0f962a Mon Sep 17 00:00:00 2001 From: Eric Winger Date: Thu, 20 Aug 2026 15:48:49 -0700 Subject: [PATCH 05/15] SUnit: run tests without freezing the editor, and let a run be stopped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A test can run for minutes, and the blocking GCI call held the extension host for its whole duration. That is why nothing could interrupt one — not even VS Code's own stop button, whose cancellation event could not be delivered until the run had already finished. - The two run queries are split into a code builder and a parser, so the same Smalltalk serves both the blocking path (unchanged, for its other callers) and a new non-blocking one that polls instead. - A run holds its canceller for as long as it is in flight, and the run profile's cancellation token is wired to it. One press is enough: a soft break goes first, because it lets the gem stop at a safe point and leaves the session healthy, and it escalates to a hard break 1.5s later on its own rather than making the user press again to discover the test was in a tight loop. - A stopped test is reported skipped, with no outcome left behind. Calling it an error would blame the test for the user's decision, and leave a red mark to clear. This covers both ways a stop ends a run: the hard break's cancellation, and the Break error a soft break usually raises instead. - A debug run is marked not stoppable. The gem is deliberately suspended and belongs to the debugger, whose own Terminate ends it; a stop button of ours would be one that does nothing. Also, so a row can say something honest between runs: a run now blanks the previous outcome before it starts, since re-running a failing test that fails again would otherwise repaint the mark it already showed, leaving no sign the run happened. And a run is named after what ran, rather than VS Code's "Test run at ", which says nothing about which tests these were. Toward #427. Co-Authored-By: Claude Opus 5 (1M context) --- client/src/__mocks__/vscode.ts | 7 +- .../src/__tests__/sunitTestController.test.ts | 456 +++++++++++++++++- client/src/queries/runTestClass.ts | 26 +- client/src/queries/runTestMethod.ts | 31 +- client/src/sunitQueries.ts | 58 ++- client/src/sunitTestController.ts | 352 ++++++++++++-- 6 files changed, 862 insertions(+), 68 deletions(-) diff --git a/client/src/__mocks__/vscode.ts b/client/src/__mocks__/vscode.ts index 38b5321d..1e628b4a 100644 --- a/client/src/__mocks__/vscode.ts +++ b/client/src/__mocks__/vscode.ts @@ -78,7 +78,12 @@ export const TreeItemCollapsibleState = { // ── ThemeIcon mock ───────────────────────────────────────── export class ThemeIcon { - constructor(public readonly id: string) {} + constructor( + public readonly id: string, + // Real ThemeIcon takes an optional ThemeColor; kept here so a test can assert + // which colour a row was painted (pass vs. failed vs. stale). + public readonly color?: { id: string }, + ) {} } // ── CancellationTokenSource mock ─────────────────────────── diff --git a/client/src/__tests__/sunitTestController.test.ts b/client/src/__tests__/sunitTestController.test.ts index 07323211..2a1aa750 100644 --- a/client/src/__tests__/sunitTestController.test.ts +++ b/client/src/__tests__/sunitTestController.test.ts @@ -18,6 +18,33 @@ vi.mock('../sunitQueries', () => ({ message: '', durationMs: 10, })), + runTestMethodNb: vi.fn(() => + Promise.resolve({ + className: 'MyTestCase', + selector: 'testAdd', + status: 'passed', + message: '', + durationMs: 10, + }), + ), + runTestClassNb: vi.fn(() => + Promise.resolve([ + { + className: 'MyTestCase', + selector: 'testAdd', + status: 'passed', + message: '', + durationMs: 5, + }, + { + className: 'MyTestCase', + selector: 'testRemove', + status: 'failed', + message: 'Expected true', + durationMs: 3, + }, + ]), + ), runTestClass: vi.fn(() => [ { className: 'MyTestCase', selector: 'testAdd', status: 'passed', message: '', durationMs: 5 }, { @@ -37,7 +64,9 @@ vi.mock('../sunitQueries', () => ({ }, })); -import { tests, window, TestRunProfileKind } from '../__mocks__/vscode'; +import { tests, window, commands, TestRunProfileKind } from '../__mocks__/vscode'; +import { buildClassDefinitionUri, buildMethodUri } from '../gemstoneFileSystemProvider'; +import { NbCancelledError } from '../nbRunner'; import { SunitTestController, SunitDebugOutcome } from '../sunitTestController'; import { SessionManager } from '../sessionManager'; import * as sunit from '../sunitQueries'; @@ -272,10 +301,11 @@ describe('SunitTestController', () => { await ctrl.runClassByName('UserGlobals', 'MyTestCase'); - expect(sunit.runTestClass).toHaveBeenCalledWith( + expect(sunit.runTestClassNb).toHaveBeenCalledWith( expect.objectContaining({ id: 1 }), 'MyTestCase', 'UserGlobals', + expect.any(Function), ); ctrl.dispose(); }); @@ -316,18 +346,20 @@ describe('SunitTestController', () => { await ctrl.runClassesByName('UserGlobals', ['MyTestCase', 'OtherTest']); - expect(sunit.runTestClass).toHaveBeenCalledTimes(2); - expect(sunit.runTestClass).toHaveBeenNthCalledWith( + expect(sunit.runTestClassNb).toHaveBeenCalledTimes(2); + expect(sunit.runTestClassNb).toHaveBeenNthCalledWith( 1, expect.objectContaining({ id: 1 }), 'MyTestCase', 'UserGlobals', + expect.any(Function), ); - expect(sunit.runTestClass).toHaveBeenNthCalledWith( + expect(sunit.runTestClassNb).toHaveBeenNthCalledWith( 2, expect.objectContaining({ id: 1 }), 'OtherTest', 'UserGlobals', + expect.any(Function), ); ctrl.dispose(); }); @@ -340,11 +372,12 @@ describe('SunitTestController', () => { // Ask for both names but scoped to UserGlobals — only MyTestCase matches. await ctrl.runClassesByName('UserGlobals', ['MyTestCase', 'OtherTest']); - expect(sunit.runTestClass).toHaveBeenCalledTimes(1); - expect(sunit.runTestClass).toHaveBeenCalledWith( + expect(sunit.runTestClassNb).toHaveBeenCalledTimes(1); + expect(sunit.runTestClassNb).toHaveBeenCalledWith( expect.objectContaining({ id: 1 }), 'MyTestCase', 'UserGlobals', + expect.any(Function), ); ctrl.dispose(); }); @@ -355,7 +388,7 @@ describe('SunitTestController', () => { await ctrl.runClassesByName('UserGlobals', ['NoSuchTest']); - expect(sunit.runTestClass).not.toHaveBeenCalled(); + expect(sunit.runTestClassNb).not.toHaveBeenCalled(); ctrl.dispose(); }); }); @@ -375,12 +408,13 @@ describe('SunitTestController', () => { it('runs a single test', async () => { await sunitTestController.runTestsByName('UserGlobals', 'MyTestCase', ['testAdd']); - expect(sunit.runTestMethod).toHaveBeenCalledTimes(1); - expect(sunit.runTestMethod).toHaveBeenCalledWith( + expect(sunit.runTestMethodNb).toHaveBeenCalledTimes(1); + expect(sunit.runTestMethodNb).toHaveBeenCalledWith( expect.objectContaining({ id: 1 }), 'MyTestCase', 'testAdd', 'UserGlobals', + expect.any(Function), ); }); @@ -390,7 +424,7 @@ describe('SunitTestController', () => { expect(window.showWarningMessage).toHaveBeenCalledWith( sunitTestController.notATestClassErrorMessage('NoSuchClass'), ); - expect(sunit.runTestMethod).not.toHaveBeenCalled(); + expect(sunit.runTestMethodNb).not.toHaveBeenCalled(); }); it('does not run tests when no tests methods were found', async () => { @@ -399,7 +433,7 @@ describe('SunitTestController', () => { expect(window.showWarningMessage).toHaveBeenCalledWith( sunitTestController.noTestsFoundErrorMessage(), ); - expect(sunit.runTestMethod).not.toHaveBeenCalled(); + expect(sunit.runTestMethodNb).not.toHaveBeenCalled(); }); }); @@ -419,18 +453,20 @@ describe('SunitTestController', () => { // 'testAdd' and 'testRemove' are both in 'unit tests' per the mock await ctrl.runMethodCategoryByName('UserGlobals', 'MyTestCase', 'unit tests'); - expect(sunit.runTestMethod).toHaveBeenCalledTimes(2); - expect(sunit.runTestMethod).toHaveBeenCalledWith( + expect(sunit.runTestMethodNb).toHaveBeenCalledTimes(2); + expect(sunit.runTestMethodNb).toHaveBeenCalledWith( expect.objectContaining({ id: 1 }), 'MyTestCase', 'testAdd', 'UserGlobals', + expect.any(Function), ); - expect(sunit.runTestMethod).toHaveBeenCalledWith( + expect(sunit.runTestMethodNb).toHaveBeenCalledWith( expect.objectContaining({ id: 1 }), 'MyTestCase', 'testRemove', 'UserGlobals', + expect.any(Function), ); }); @@ -440,7 +476,7 @@ describe('SunitTestController', () => { expect(window.showWarningMessage).toHaveBeenCalledWith( expect.stringContaining('NoSuchClass'), ); - expect(sunit.runTestMethod).not.toHaveBeenCalled(); + expect(sunit.runTestMethodNb).not.toHaveBeenCalled(); ctrl.dispose(); }); @@ -448,7 +484,7 @@ describe('SunitTestController', () => { await ctrl.runMethodCategoryByName('UserGlobals', 'MyTestCase', 'non-existent category'); expect(window.showWarningMessage).toHaveBeenCalledWith(ctrl.noTestsFoundErrorMessage()); - expect(sunit.runTestMethod).not.toHaveBeenCalled(); + expect(sunit.runTestMethodNb).not.toHaveBeenCalled(); ctrl.dispose(); }); }); @@ -474,26 +510,31 @@ describe('SunitTestController', () => { // grab the handler the Test Explorer invokes when you click "Run". const runHandler = (mockController.createRunProfile as ReturnType).mock .calls[0][2]; - const cancellationToken = { isCancellationRequested: false }; + const cancellationToken = { + isCancellationRequested: false, + onCancellationRequested: () => ({ dispose: () => {} }), + }; // Run the UserGlobals copy — must resolve against UserGlobals, not the // symbol-list winner. await runHandler({ include: [userGlobals], exclude: undefined }, cancellationToken); - expect(sunit.runTestClass).toHaveBeenLastCalledWith( + expect(sunit.runTestClassNb).toHaveBeenLastCalledWith( expect.objectContaining({ id: 1 }), 'AnnouncerTest', 'UserGlobals', + expect.any(Function), ); // Run the Globals copy — must resolve against Globals. await runHandler({ include: [globals], exclude: undefined }, cancellationToken); - expect(sunit.runTestClass).toHaveBeenLastCalledWith( + expect(sunit.runTestClassNb).toHaveBeenLastCalledWith( expect.objectContaining({ id: 1 }), 'AnnouncerTest', 'Globals', + expect.any(Function), ); - expect(sunit.runTestClass).toHaveBeenCalledTimes(2); + expect(sunit.runTestClassNb).toHaveBeenCalledTimes(2); ctrl.dispose(); }); }); @@ -557,6 +598,24 @@ describe('SunitTestController', () => { ctrl.dispose(); }); + it('blanks a previous outcome before re-running, so a repeat run is visible', async () => { + // Re-running a failing test that fails again would otherwise repaint the ✗ + // it was already showing, leaving no sign the run happened. + const ctrl = new SunitTestController(makeSessionManager(true)); + await ctrl.runTestsByName('UserGlobals', 'MyTestCase', ['testAdd']); + const seen: (string | undefined)[] = []; + ctrl.onDidChangeResults(() => { + seen.push(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testAdd')?.outcome); + }); + + await ctrl.runTestsByName('UserGlobals', 'MyTestCase', ['testAdd']); + + expect(seen[0]).toBeUndefined(); + expect(seen).toContain('running'); + expect(seen.at(-1)).toBe('passed'); + ctrl.dispose(); + }); + it('batches the change event rather than firing per test', async () => { const ctrl = new SunitTestController(makeSessionManager(true)); let fires = 0; @@ -750,7 +809,7 @@ describe('SunitTestController', () => { await ctrl.runTestsByName('UserGlobals', 'MyTestCase', ['testAdd'], 'debug'); - expect(sunit.runTestMethod).not.toHaveBeenCalled(); + expect(sunit.runTestMethodNb).not.toHaveBeenCalled(); expect(debugExecutor.executeWithDebugger).toHaveBeenCalledOnce(); const code = debugExecutor.executeWithDebugger.mock.calls[0][1]; expect(code).toContain('MyTestCase'); @@ -765,7 +824,7 @@ describe('SunitTestController', () => { await ctrl.runClassByName('UserGlobals', 'MyTestCase', 'debug'); // runTestClass installs the handler that makes a failure undebuggable. - expect(sunit.runTestClass).not.toHaveBeenCalled(); + expect(sunit.runTestClassNb).not.toHaveBeenCalled(); expect(debugExecutor.executeWithDebugger).toHaveBeenCalledTimes(2); ctrl.dispose(); }); @@ -808,6 +867,357 @@ describe('SunitTestController', () => { }); }); + describe('resolving tests for an open document (gutter icons)', () => { + // The URI is built with the same builder the editor opens with — a test that + // hand-assembled it could pass while the real gutter stayed empty. + const methodUri = ( + selector: string, + className = 'MyTestCase', + dictName = 'UserGlobals', + sessionId = 1, + ) => + buildMethodUri({ + kind: 'method', + sessionId, + dictName, + className, + isMeta: false, + category: 'unit tests', + selector, + environmentId: 0, + }); + + async function discovered() { + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + await mockController.resolveHandler(undefined); + return { ctrl, mockController }; + } + + it("lists a test class's methods when one of them is opened", async () => { + const { ctrl, mockController } = await discovered(); + const classItem = mockController.items.get('sunit/1/UserGlobals/MyTestCase'); + expect(classItem.children.size).toBe(0); + + await ctrl.ensureTestsForDocument(methodUri('testAdd')); + + // Without this the Testing view knows the class but not the method, and + // VS Code has no item to hang a gutter icon on. + expect(sunit.discoverTestMethods).toHaveBeenCalled(); + expect(classItem.children.size).toBe(2); + ctrl.dispose(); + }); + + it('does not re-resolve a class whose methods are already listed', async () => { + const { ctrl, mockController } = await discovered(); + const classItem = mockController.items.get('sunit/1/UserGlobals/MyTestCase'); + await mockController.resolveHandler(classItem); + (sunit.discoverTestMethods as ReturnType).mockClear(); + + await ctrl.ensureTestsForDocument(methodUri('testAdd')); + + expect(sunit.discoverTestMethods).not.toHaveBeenCalled(); + ctrl.dispose(); + }); + + it('ignores a method of a class that is not a test class', async () => { + const { ctrl } = await discovered(); + (sunit.discoverTestMethods as ReturnType).mockClear(); + + await ctrl.ensureTestsForDocument(methodUri('doSomething', 'NotATest')); + + expect(sunit.discoverTestMethods).not.toHaveBeenCalled(); + ctrl.dispose(); + }); + + it('ignores a document belonging to another session', async () => { + // Items are keyed by session; resolving this one would list the selected + // stone's methods under a document from a different stone. + const { ctrl } = await discovered(); + (sunit.discoverTestMethods as ReturnType).mockClear(); + + await ctrl.ensureTestsForDocument(methodUri('testAdd', 'MyTestCase', 'UserGlobals', 2)); + + expect(sunit.discoverTestMethods).not.toHaveBeenCalled(); + ctrl.dispose(); + }); + + it('ignores a document that is not a method — a class definition has its own item', async () => { + const { ctrl } = await discovered(); + (sunit.discoverTestMethods as ReturnType).mockClear(); + + await ctrl.ensureTestsForDocument(buildClassDefinitionUri(1, 'UserGlobals', 'MyTestCase')); + await ctrl.ensureTestsForDocument(undefined); + + expect(sunit.discoverTestMethods).not.toHaveBeenCalled(); + ctrl.dispose(); + }); + }); + + describe('naming a run', () => { + // VS Code labels an unnamed run 'Test run at ', which does not + // say which tests ran. + const runNameOf = (mockController: { createTestRun: ReturnType }) => + mockController.createTestRun.mock.calls.at(-1)?.[1]; + + async function run(include?: unknown[]) { + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + await mockController.resolveHandler(undefined); + const runHandler = mockController.createRunProfile.mock.calls[0][2]; + await runHandler( + { include, exclude: undefined }, + { isCancellationRequested: false, onCancellationRequested: () => ({ dispose: () => {} }) }, + ); + return { ctrl, mockController }; + } + + it('names a single-class run after the class', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + await mockController.resolveHandler(undefined); + const classItem = mockController.items.get('sunit/1/UserGlobals/MyTestCase'); + + const runHandler = mockController.createRunProfile.mock.calls[0][2]; + await runHandler( + { include: [classItem], exclude: undefined }, + { + isCancellationRequested: false, + onCancellationRequested: () => ({ dispose: () => {} }), + }, + ); + + expect(runNameOf(mockController)).toBe('MyTestCase'); + ctrl.dispose(); + }); + + it('names a single-method run Class>>selector', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + await mockController.resolveHandler(undefined); + const classItem = mockController.items.get('sunit/1/UserGlobals/MyTestCase'); + await mockController.resolveHandler(classItem); + const methodItem = classItem.children.get('sunit/1/UserGlobals/MyTestCase/testAdd'); + + const runHandler = mockController.createRunProfile.mock.calls[0][2]; + await runHandler( + { include: [methodItem], exclude: undefined }, + { + isCancellationRequested: false, + onCancellationRequested: () => ({ dispose: () => {} }), + }, + ); + + // The mock has no parent link, so the class name comes from the id. + expect(runNameOf(mockController)).toBe('MyTestCase>>testAdd'); + ctrl.dispose(); + }); + + it('names a run-everything after the number of classes', async () => { + const { ctrl, mockController } = await run(undefined); + + expect(runNameOf(mockController)).toBe('2 test classes'); + ctrl.dispose(); + }); + }); + + describe('stopping a run', () => { + it('has nothing to stop when no run is in flight', () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + + expect(ctrl.cancelActiveRun()).toBe(false); + ctrl.dispose(); + }); + + it('hands the run canceller out so a stop button can break the gem', async () => { + // The query offers its canceller through onStart; the controller holds it for + // as long as the call is in flight, and drops it when the call settles. + let cancelled = false; + const ctrl = new SunitTestController(makeSessionManager(true)); + const during: boolean[] = []; + (sunit.runTestMethodNb as ReturnType).mockImplementationOnce( + (_s, _c, _sel, _d, onStart?: (cancel: () => void) => void) => { + onStart?.(() => { + cancelled = true; + }); + during.push(ctrl.cancelActiveRun()); + return Promise.resolve({ + className: 'MyTestCase', + selector: 'testAdd', + status: 'passed', + message: '', + durationMs: 1, + }); + }, + ); + + await ctrl.runTestsByName('UserGlobals', 'MyTestCase', ['testAdd']); + + expect(during).toEqual([true]); + expect(cancelled).toBe(true); + // Settled, so there is nothing left to stop. + expect(ctrl.cancelActiveRun()).toBe(false); + ctrl.dispose(); + }); + + it("breaks the gem when VS Code's own stop button cancels the run", async () => { + // The Testing view's stop cancels the run profile's token. Nothing else + // connects that to the gem, so if this wiring goes, the button goes quiet. + let broke = false; + let release: (() => void) | undefined; + (sunit.runTestClassNb as ReturnType).mockImplementationOnce( + (_s, _c, _d, onStart?: (cancel: () => void) => void) => { + onStart?.(() => { + broke = true; + }); + return new Promise((resolve) => { + release = () => resolve([]); + }); + }, + ); + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + await mockController.resolveHandler(undefined); + const classItem = mockController.items.get('sunit/1/UserGlobals/MyTestCase'); + + let requestCancel: (() => void) | undefined; + const runHandler = mockController.createRunProfile.mock.calls[0][2]; + const running = runHandler( + { include: [classItem], exclude: undefined }, + { + isCancellationRequested: false, + onCancellationRequested: (listener: () => void) => { + requestCancel = listener; + return { dispose: () => {} }; + }, + }, + ); + // Let the run reach the (pending) stone call before stopping it: markRunning + // yields twice on its own (blank frame, then spinner) before the call starts. + for (let i = 0; i < 8; i++) await new Promise((r) => setImmediate(r)); + + expect(requestCancel).toBeDefined(); + requestCancel!(); + + expect(broke).toBe(true); + release?.(); + await running; + ctrl.dispose(); + }); + + it('leaves no verdict on a test whose run was stopped', async () => { + // Reporting "error" would blame the test for the user's decision to stop. + (sunit.runTestMethodNb as ReturnType).mockRejectedValueOnce( + new NbCancelledError('Execution cancelled.'), + ); + const ctrl = new SunitTestController(makeSessionManager(true)); + + await ctrl.runTestsByName('UserGlobals', 'MyTestCase', ['testAdd']); + + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testAdd')).toBeUndefined(); + ctrl.dispose(); + }); + + it('leaves no verdict on any test of a class run that was stopped', async () => { + (sunit.runTestClassNb as ReturnType).mockRejectedValueOnce( + new NbCancelledError('Execution cancelled.'), + ); + const ctrl = new SunitTestController(makeSessionManager(true)); + + await ctrl.runClassByName('UserGlobals', 'MyTestCase'); + + expect(ctrl.resultFor('UserGlobals', 'MyTestCase')).toBeUndefined(); + expect(ctrl.resultFor('UserGlobals', 'MyTestCase', 'testAdd')).toBeUndefined(); + ctrl.dispose(); + }); + }); + + describe('a test class with no tests', () => { + it('is not offered as runnable — a run would do nothing', async () => { + (sunit.discoverTestClasses as ReturnType).mockReturnValueOnce([ + { dictName: 'UserGlobals', className: 'AbstractBase', testCount: 0 }, + { dictName: 'UserGlobals', className: 'MyTestCase', testCount: 2 }, + ]); + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + await mockController.resolveHandler(undefined); + + expect(ctrl.isTestClass('UserGlobals', 'AbstractBase')).toBe(false); + expect(ctrl.isTestClass('UserGlobals', 'MyTestCase')).toBe(true); + ctrl.dispose(); + }); + + it('stays runnable when the stone gave no usable count', async () => { + // Better a button that reports "no tests found" than one silently missing. + (sunit.discoverTestClasses as ReturnType).mockReturnValueOnce([ + { dictName: 'UserGlobals', className: 'MyTestCase', testCount: null }, + ]); + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + await mockController.resolveHandler(undefined); + + expect(ctrl.isTestClass('UserGlobals', 'MyTestCase')).toBe(true); + ctrl.dispose(); + }); + }); + + describe('revealInTestExplorer', () => { + it('reveals a discovered test class', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + await mockController.resolveHandler(undefined); + + await expect(ctrl.revealInTestExplorer('UserGlobals', 'MyTestCase')).resolves.toBe(true); + // The view has to be showing before the reveal, or it scrolls something + // nobody can see. + expect(commands.executeCommand).toHaveBeenNthCalledWith(1, 'workbench.view.testing.focus'); + expect(commands.executeCommand).toHaveBeenNthCalledWith( + 2, + 'vscode.revealTestInExplorer', + expect.objectContaining({ id: 'sunit/1/UserGlobals/MyTestCase' }), + ); + ctrl.dispose(); + }); + + it("lists a class's methods on demand so one can be revealed", async () => { + // Methods are discovered lazily; wanting to reveal one is a reason to have it. + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + await mockController.resolveHandler(undefined); + + await expect(ctrl.revealInTestExplorer('UserGlobals', 'MyTestCase', 'testAdd')).resolves.toBe( + true, + ); + expect(commands.executeCommand).toHaveBeenCalledWith( + 'vscode.revealTestInExplorer', + expect.objectContaining({ id: 'sunit/1/UserGlobals/MyTestCase/testAdd' }), + ); + ctrl.dispose(); + }); + + it('answers false when there is nothing to reveal', async () => { + const ctrl = new SunitTestController(makeSessionManager(true)); + const mockController = (tests.createTestController as ReturnType).mock + .results[0].value; + await mockController.resolveHandler(undefined); + + await expect(ctrl.revealInTestExplorer('UserGlobals', 'NotATest')).resolves.toBe(false); + await expect( + ctrl.revealInTestExplorer('UserGlobals', 'MyTestCase', 'notATest'), + ).resolves.toBe(false); + ctrl.dispose(); + }); + }); + describe('dispose', () => { it('disposes the controller', () => { const sm = makeSessionManager(true); diff --git a/client/src/queries/runTestClass.ts b/client/src/queries/runTestClass.ts index 6eef9b24..b46cfd4e 100644 --- a/client/src/queries/runTestClass.ts +++ b/client/src/queries/runTestClass.ts @@ -2,11 +2,14 @@ import { QueryExecutor } from './types'; import { classLookupOrRaiseExpr, splitLines } from './util'; import { TestRunResult } from './runTestMethod'; -export function runTestClass( - execute: QueryExecutor, - className: string, - dictName?: string, -): TestRunResult[] { +/** + * The Smalltalk for running a whole test class, and the parse of what it + * answers, are exported separately from the blocking `runTestClass` below so a + * caller can run the same code non-blocking (see sunitQueries.runTestClassNb). + * A test can run for minutes, and the synchronous GCI call freezes the whole + * extension host for the duration — which is also why nothing could interrupt it. + */ +export function runTestClassCode(className: string, dictName?: string): string { // Resolve the class dictionary-scoped (when dictName is given) rather than // by bare name. Two distinct TestCase subclasses can share a name across // dictionaries; bare-name `objectNamed:` would silently run only the @@ -61,7 +64,10 @@ result errors do: [:each | captureMessage value: each. ws lf]. ws contents encodeAsUTF8`; - const data = execute(code); + return code; +} + +export function parseTestClassResults(data: string, className: string): TestRunResult[] { return splitLines(data).map((line) => { const parts = line.split('\t'); return { @@ -73,3 +79,11 @@ ws contents encodeAsUTF8`; }; }); } + +export function runTestClass( + execute: QueryExecutor, + className: string, + dictName?: string, +): TestRunResult[] { + return parseTestClassResults(execute(runTestClassCode(className, dictName)), className); +} diff --git a/client/src/queries/runTestMethod.ts b/client/src/queries/runTestMethod.ts index 84a4a040..c94e7c5f 100644 --- a/client/src/queries/runTestMethod.ts +++ b/client/src/queries/runTestMethod.ts @@ -9,12 +9,9 @@ export interface TestRunResult { durationMs: number; } -export function runTestMethod( - execute: QueryExecutor, - className: string, - selector: string, - dictName?: string, -): TestRunResult { +/** See runTestClassCode: code and parse are split so one test can also be run + * non-blocking, and therefore interrupted. */ +export function runTestMethodCode(className: string, selector: string, dictName?: string): string { const sel = escapeString(selector); // Resolve dictionary-scoped (see runTestClass for the rationale): a bare // name can resolve to the wrong same-named class in another dictionary. @@ -54,7 +51,14 @@ captured isNil ws tab]. ws nextPutAll: (endMs - startMs) printString. ws contents encodeAsUTF8`; - const data = execute(code); + return code; +} + +export function parseTestMethodResult( + data: string, + className: string, + selector: string, +): TestRunResult { const parts = data.split('\t'); return { className, @@ -64,3 +68,16 @@ ws contents encodeAsUTF8`; durationMs: parseInt(parts[2] || '0', 10) || 0, }; } + +export function runTestMethod( + execute: QueryExecutor, + className: string, + selector: string, + dictName?: string, +): TestRunResult { + return parseTestMethodResult( + execute(runTestMethodCode(className, selector, dictName)), + className, + selector, + ); +} diff --git a/client/src/sunitQueries.ts b/client/src/sunitQueries.ts index 016e4f59..9e56aae8 100644 --- a/client/src/sunitQueries.ts +++ b/client/src/sunitQueries.ts @@ -1,10 +1,22 @@ import { ActiveSession } from './sessionManager'; -import { BrowserQueryError, defaultQueryExecutorUsing } from './browserQueries'; +import { + BrowserQueryError, + defaultQueryExecutorUsing, + executeFetchStringNb, +} from './browserQueries'; import { discoverTestClasses as sharedDiscoverTestClasses } from './queries/discoverTestClasses'; import { discoverTestMethods as sharedDiscoverTestMethods } from './queries/discoverTestMethods'; -import { runTestMethod as sharedRunTestMethod } from './queries/runTestMethod'; -import { runTestClass as sharedRunTestClass } from './queries/runTestClass'; +import { + runTestMethod as sharedRunTestMethod, + runTestMethodCode, + parseTestMethodResult, +} from './queries/runTestMethod'; +import { + runTestClass as sharedRunTestClass, + runTestClassCode, + parseTestClassResults, +} from './queries/runTestClass'; import { runFailingTests as sharedRunFailingTests } from './queries/runFailingTests'; import { describeTestFailure as sharedDescribeTestFailure } from './queries/describeTestFailure'; @@ -38,6 +50,46 @@ export function runTestClass(session: ActiveSession, className: string, dictName return sharedRunTestClass(defaultQueryExecutorUsing(session), className, dictName); } +/** + * Non-blocking counterparts of the two run queries. A test can run for minutes, + * and the blocking call freezes the extension host for its whole duration — + * which is why nothing, not even VS Code's own stop button, could interrupt a + * run. These poll instead, so the UI stays live and `onStart`'s canceller can + * break the gem (soft first, hard on a second call). + */ +export function runTestClassNb( + session: ActiveSession, + className: string, + dictName?: string, + onStart?: (cancel: () => void) => void, +) { + return executeFetchStringNb( + session, + `Run tests: ${className}`, + runTestClassCode(className, dictName), + `GemStone: running ${className} tests…`, + false, + onStart, + ).then((data) => parseTestClassResults(data, className)); +} + +export function runTestMethodNb( + session: ActiveSession, + className: string, + selector: string, + dictName?: string, + onStart?: (cancel: () => void) => void, +) { + return executeFetchStringNb( + session, + `Run test: ${className}>>${selector}`, + runTestMethodCode(className, selector, dictName), + `GemStone: running ${className}>>${selector}…`, + false, + onStart, + ).then((data) => parseTestMethodResult(data, className, selector)); +} + export function runFailingTests( session: ActiveSession, classNames?: string[], diff --git a/client/src/sunitTestController.ts b/client/src/sunitTestController.ts index a63f3bfa..d0f5eba1 100644 --- a/client/src/sunitTestController.ts +++ b/client/src/sunitTestController.ts @@ -1,7 +1,13 @@ import type { TestItem } from 'vscode'; import * as vscode from 'vscode'; -import { buildClassDefinitionUri, buildMethodUri } from './gemstoneFileSystemProvider'; +import { + buildClassDefinitionUri, + buildMethodUri, + parseMethodUri, +} from './gemstoneFileSystemProvider'; import { debugTestMethodCode } from './queries/debugTestMethod'; +import { logInfo } from './gciLog'; +import { NbCancelledError } from './nbRunner'; import { ActiveSession, SessionManager } from './sessionManager'; import * as sunit from './sunitQueries'; @@ -42,6 +48,20 @@ function parseTestId(id: string): ParsedTestId { return { dictName, className, selector }; } +/** + * Publish whether a test run is in flight, for the `when` clause of the ■ on the + * Testing view's rows. A context key is global rather than per-row — VS Code has + * no per-test-item key — so the button appears on every test row during a run, + * not only the one executing. That is the honest reading anyway: one session runs + * one thing at a time, and stopping is stopping THE run. + */ +async function setTestRunningContext(running: boolean): Promise { + await vscode.commands.executeCommand('setContext', 'gemstone.testRunning', running); +} + +/** How long to wait for a soft break to land before escalating to a hard one. */ +const HARD_BREAK_AFTER_MS = 1500; + /** * How a test was launched. Everything above the innermost "execute one test" * step is shared between the two — discovery, id parsing, reporting, and the @@ -91,6 +111,14 @@ export interface SunitResult { /** Class rows only: how many of the class's tests passed, out of how many. */ passedCount?: number; totalCount?: number; + /** + * Running results only: whether this run can be broken. True for an ordinary + * run, which goes through the interruptible path; false under the debugger, + * where the gem is deliberately suspended and belongs to the debugger — its + * own Terminate is what ends it, and a stop button of ours would be a button + * that does nothing. + */ + stoppable?: boolean; /** * True once code has been compiled since this result was produced, so the * outcome may no longer describe the code in the stone. Stale results are @@ -114,6 +142,43 @@ function topOfDocument(): vscode.Range { return new vscode.Range(new vscode.Position(0, 0), new vscode.Position(0, 0)); } +/** + * What to call one run in the Test Results panel. VS Code names an unnamed run + * `Test run at `, which says nothing about WHICH tests ran — with + * several classes in the tree every entry in the history reads alike. + * + * Names come from the items' own labels, not from the raw class name, so a + * class whose name exists in two dictionaries keeps the `{Dictionary}` + * qualifier discovery gave it. + */ +function runLabel(queue: vscode.TestItem[]): string | undefined { + if (queue.length === 0) return undefined; + + // A method item's class label lives on its parent; fall back to the id's + // class name for an item that has no parent (nothing in the tree does, but + // `parent` is optional in the API). + const classLabelOf = (item: vscode.TestItem): string => + parseTestId(item.id).selector === undefined + ? item.label + : (item.parent?.label ?? parseTestId(item.id).className); + + if (queue.length === 1) { + const [item] = queue; + return parseTestId(item.id).selector === undefined + ? classLabelOf(item) + : `${classLabelOf(item)}>>${item.label}`; + } + + const methodCount = queue.filter((i) => parseTestId(i.id).selector !== undefined).length; + if (methodCount === 0) return `${queue.length} test classes`; + + const classLabels = new Set(queue.map(classLabelOf)); + if (methodCount < queue.length) return `${queue.length} test selections`; + return classLabels.size === 1 + ? `${[...classLabels][0]} (${queue.length} tests)` + : `${queue.length} tests in ${classLabels.size} classes`; +} + /** * Integrates GemStone SUnit tests with VS Code's Test Explorer. */ @@ -197,6 +262,116 @@ export class SunitTestController implements vscode.Disposable { for (const d of this.disposables) d.dispose(); } + /** + * Every URI a test item currently points at. Clicking a row in the Testing + * view opens one, and that open is indistinguishable from any other — no API + * says where it came from. The Explorer consults this to leave its panes alone + * for a click it did not cause. + */ + private readonly itemUris = new Set(); + /** Test-method count per class id, as discovery reported it. null when the stone + * answered something unparseable. See isTestClass. */ + private readonly classTestCount = new Map(); + + /** True when `uri` is the document some test item points at. */ + isTestItemUri(uri: vscode.Uri): boolean { + return this.itemUris.has(uri.toString()); + } + + /** + * Show a class, or one of its test methods, in the Testing view — selected and + * scrolled to, the mirror of Reveal in GemStone Explorer. + * + * Answers false when there is nothing to reveal (not a test class, or a + * selector SUnit doesn't run), so the caller can say so instead of appearing + * to do nothing. A method's row is listed on demand: the class's methods are + * discovered lazily, and revealing one is exactly a reason to have it. + */ + async revealInTestExplorer( + dictName: string, + className: string, + selector?: string, + ): Promise { + const session = this.sessionManager.getSelectedSession(); + if (!session) return false; + const classItem = this.controller.items.get(makeClassId(session.id, dictName, className)); + if (!classItem) return false; + + let target: vscode.TestItem = classItem; + if (selector !== undefined) { + if (classItem.children.size === 0) await this.resolveTestMethods(classItem); + const child = classItem.children.get(makeMethodId(session.id, dictName, className, selector)); + if (!child) return false; + target = child; + } + // Bring the Testing view up FIRST. Revealing into a view that isn't showing + // scrolls something nobody can see, which reads as the reveal having done + // nothing. (Where in the list it lands is VS Code's to decide — there is no + // API to centre a revealed test item.) + await vscode.commands.executeCommand('workbench.view.testing.focus'); + await vscode.commands.executeCommand('vscode.revealTestInExplorer', target); + return true; + } + + /** + * True when this class was discovered as a TestCase subclass. Known as soon as + * discovery has run — unlike its test methods, which are listed lazily — so a + * view that builds rows synchronously can still ask. + */ + isTestClass(dictName: string, className: string): boolean { + const session = this.sessionManager.getSelectedSession(); + if (!session) return false; + const id = makeClassId(session.id, dictName, className); + if (!this.controller.items.get(id)) return false; + // A TestCase subclass with no tests of its own — an abstract base, or one + // whose tests all live in subclasses — has nothing to run. Offering a ▶ on it + // promises a run that would do nothing. An unknown count (the stone answered + // something unparseable) is treated as runnable: better a button that reports + // "no tests found" than one silently missing. + const count = this.classTestCount.get(id); + return count === undefined || count === null || count > 0; + } + + /** + * Breaks the run currently executing in the stone — soft on the first call, + * hard on a second, the same escalation the progress notification's Cancel + * does. Set while a run is in flight and cleared when it settles. + */ + private activeCancel: (() => void) | undefined; + /** True once a stop was asked for, until the next run starts. Lets a run that + * ends in a Break error be reported as stopped rather than as a test error. */ + private cancelRequested = false; + + /** True when a run is in flight and can be stopped. */ + get isRunning(): boolean { + return this.activeCancel !== undefined; + } + + /** + * Stop the run in progress. Answers false when there was nothing to stop, so a + * caller can say so rather than pretending it did something. + */ + cancelActiveRun(): boolean { + const cancel = this.activeCancel; + if (!cancel) { + logInfo('SUnit: stop requested, but no run is in flight to stop.'); + return false; + } + logInfo('SUnit: stop requested — sending a soft break.'); + this.cancelRequested = true; + cancel(); + // A soft break is the right first move — it lets the gem stop at a safe point + // and leaves the session healthy. But a test spinning in a tight loop never + // reaches one, and making the user press stop a second time to find that out + // is a poor trade. Escalate on their behalf, unless the run settled first. + setTimeout(() => { + if (this.activeCancel !== cancel) return; + logInfo('SUnit: the run is still going — escalating to a hard break.'); + cancel(); + }, HARD_BREAK_AFTER_MS); + return true; + } + /** Clear items and let resolveHandler re-discover on next view. */ refresh(): void { this.resetDiscovery(); @@ -205,6 +380,8 @@ export class SunitTestController implements vscode.Disposable { private resetDiscovery(): void { this.methodCategory.clear(); this.classDictIndex.clear(); + this.classTestCount.clear(); + this.itemUris.clear(); this.controller.items.replace([]); } @@ -415,6 +592,10 @@ export class SunitTestController implements vscode.Disposable { try { const classes = sunit.discoverTestClasses(session); const items: vscode.TestItem[] = []; + // items.replace below drops every existing item, methods included, so the + // URIs they were reachable at stop being test URIs at the same moment. + this.itemUris.clear(); + this.classTestCount.clear(); // A class name is ambiguous when it exists in more than one dictionary. // Only then do we qualify the label with the dictionary — the Test @@ -437,9 +618,10 @@ export class SunitTestController implements vscode.Disposable { this.classDefinitionUri(session.id, cls.dictName, cls.className, cls.dictIndex), ); classItem.canResolveChildren = true; - // A range is what puts the run/status icon in the editor gutter. The - // class definition is its own document, so line 1 is the definition - // itself. + // A range is what puts the run/status icon in the editor gutter — but + // only for an item that exists when the document is open, which is what + // ensureTestsForDocument is for. The class definition is its own + // document, so line 1 is the definition itself. classItem.range = topOfDocument(); // Dimmed qualifier (sidebar only): test count. The dictionary never // goes here — it lives in the label, and only when the name is @@ -447,10 +629,16 @@ export class SunitTestController implements vscode.Disposable { // value; show "(?)" rather than a misleading "(0)". classItem.description = cls.testCount === null ? '(?)' : `(${cls.testCount})`; this.classDictIndex.set(id, cls.dictIndex); + this.classTestCount.set(id, cls.testCount); + if (classItem.uri) this.itemUris.add(classItem.uri.toString()); items.push(classItem); } this.controller.items.replace(items); + // A test method may already be open — a session switch or a window + // reload discovers with the editor sitting there — and replacing the + // items just dropped whatever item its gutter icon was drawn from. + await this.ensureTestsForDocument(vscode.window.activeTextEditor?.document.uri); } catch (e: unknown) { const msg = e instanceof Error ? e.message : String(e); vscode.window.showErrorMessage(`SUnit discovery failed: ${msg}`); @@ -482,6 +670,7 @@ export class SunitTestController implements vscode.Disposable { ); // Gutter icon on line 1 of the method's own document (see above). methodItem.range = topOfDocument(); + if (methodItem.uri) this.itemUris.add(methodItem.uri.toString()); children.push(methodItem); } @@ -492,13 +681,47 @@ export class SunitTestController implements vscode.Disposable { } } + /** + * Make sure the test item a just-opened document could carry a gutter icon + * for actually exists. + * + * Test methods are discovered lazily — normally when someone expands the + * class row in the Testing view — but VS Code can only draw the icon for a + * test item it already knows about. Opening a test method straight from the + * Explorer never expands that row, so without this the gutter stays empty + * for the one workflow most likely to want it. + * + * Idempotent and cheap: it does nothing unless the document is a method of a + * class already discovered as a TestCase subclass, and nothing again once + * that class's methods are listed. + */ + async ensureTestsForDocument(uri: vscode.Uri | undefined): Promise { + if (!uri) return; + const method = parseMethodUri(uri); + if (!method) return; + + // Items are per-session. A document left open from a session that is no + // longer selected has no item here, and must not resolve one against + // whichever stone happens to be selected now. + const session = this.sessionManager.getSelectedSession(); + if (!session || session.id !== method.sessionId) return; + + const classItem = this.controller.items.get( + makeClassId(session.id, method.dictName, method.className), + ); + // Not a test class, or its methods are already listed. + if (!classItem || classItem.children.size > 0) return; + await this.resolveTestMethods(classItem); + } + /** * The URIs below must be built with the same builders the editor uses, not * assembled by hand: VS Code matches a test item to an open document by exact * URI, so a differing query string or a hand-encoded segment silently costs - * the gutter icon. A malformed name (a category containing '/', say) makes the - * builder throw — answer undefined rather than lose the whole discovery pass; - * the item still works everywhere except the gutter. + * the gutter icon. A name the builder rejects (a dictionary or class name + * containing '/') makes it throw — answer undefined rather than lose the whole + * discovery pass; the item still works everywhere except the gutter. A category + * with a slash is fine: it is escaped in the path, same as a selector. */ private classDefinitionUri( sessionId: number, @@ -551,8 +774,17 @@ export class SunitTestController implements vscode.Disposable { return; } - const run = this.controller.createTestRun(request); const queue = this.getTestsToRun(request); + const run = this.controller.createTestRun(request, runLabel(queue)); + // The Testing view's stop button. It could do nothing before: the run held the + // extension host inside a synchronous GCI call, so this event could not even + // be delivered until the run had already finished. + const stopped = token.onCancellationRequested(() => this.cancelActiveRun()); + // Drives the ■ on the Testing view's rows. VS Code draws its own ▶ there and + // gives no way to replace it, but an extension may contribute inline actions — + // so the stop appears BESIDE the run icon, on class and method rows alike, and + // only while there is a run to stop. + await setTestRunningContext(true); try { for (const item of queue) { @@ -570,6 +802,8 @@ export class SunitTestController implements vscode.Disposable { } } } finally { + stopped.dispose(); + await setTestRunningContext(false); run.end(); this.flushResultChanges(); } @@ -600,7 +834,8 @@ export class SunitTestController implements vscode.Disposable { kind: SunitRunKind, ): Promise { run.started(item); - await this.markRunning([item]); + // A debug run is not stoppable — see SunitResult.stoppable. + await this.markRunning([item], kind !== 'debug'); if (kind === 'debug' && this.debugExecutor) { await this.debugSingleTest(session, run, item, className, selector, dictName); @@ -608,14 +843,50 @@ export class SunitTestController implements vscode.Disposable { } try { - const result = sunit.runTestMethod(session, className, selector, dictName); + const result = await this.whileCancellable((onStart) => + sunit.runTestMethodNb(session, className, selector, dictName, onStart), + ); this.reportResult(run, item, result); } catch (e: unknown) { + if (this.wasStopped(e)) { + // Stopped on purpose. A test whose run was interrupted has no outcome — + // saying "error" would blame the test for the user's decision. + this.reportSkipped(run, item); + return; + } const msg = e instanceof Error ? e.message : String(e); this.reportError(run, item, `Execution error: ${msg}`); } } + /** + * Whether a failed run failed because it was stopped. A hard break rejects with + * NbCancelledError, but a soft break usually lets the gem raise a Break error + * instead, which arrives as an ordinary failure — indistinguishable from a real + * one except that we asked for it. + */ + private wasStopped(e: unknown): boolean { + return e instanceof NbCancelledError || this.cancelRequested; + } + + /** + * Run one interruptible stone call, holding onto its canceller for as long as + * it is in flight. Only one runs at a time — the session is single-threaded, + * and a second would be refused as "session is busy" anyway. + */ + private async whileCancellable( + call: (onStart: (cancel: () => void) => void) => Promise, + ): Promise { + this.cancelRequested = false; + try { + return await call((cancel) => { + this.activeCancel = cancel; + }); + } finally { + this.activeCancel = undefined; + } + } + /** * Run one test under the debugger. Reports into the same store as an ordinary * run — the whole point is that a test debugged from a row or a gutter icon @@ -672,15 +943,17 @@ export class SunitTestController implements vscode.Disposable { return; } - // Mark all children as started - run.started(classItem); + // Children only: a class the run never settles would sit in the Test + // Results list spinning forever, and its state is rolled up from these. classItem.children.forEach((child) => run.started(child)); const running: vscode.TestItem[] = [classItem]; classItem.children.forEach((child) => running.push(child)); await this.markRunning(running); try { - const results = sunit.runTestClass(session, className, dictName); + const results = await this.whileCancellable((onStart) => + sunit.runTestClassNb(session, className, dictName, onStart), + ); const resultMap = new Map(results.map((r) => [r.selector, r])); let passedCount = 0; @@ -700,18 +973,25 @@ export class SunitTestController implements vscode.Disposable { if (result.status === 'passed') passedCount += 1; }); + // Deliberately no run.passed/failed on the class itself. Every method + // already reported, and VS Code rolls a parent's state up from its + // children in the tree — reporting the class too only adds a row to the + // Test Results list that duplicates the run's own name. The class-level + // roll-up below is our own store, which the tree rows read. const allPassed = passedCount === totalCount; - if (allPassed) { - run.passed(classItem); - } else { - run.failed(classItem, new vscode.TestMessage('Some tests failed.')); - } this.setResult(dictName, className, undefined, { outcome: allPassed ? 'passed' : 'failed', passedCount, totalCount, }); } catch (e: unknown) { + if (this.wasStopped(e)) { + // See runSingleTest: an interrupted run leaves no verdict behind. + classItem.children.forEach((child) => this.reportSkipped(run, child)); + this.deleteResult(dictName, className); + this.flushResultChanges(); + return; + } const msg = e instanceof Error ? e.message : String(e); this.reportError(run, classItem, `Execution error: ${msg}`); classItem.children.forEach((child) => { @@ -737,18 +1017,20 @@ export class SunitTestController implements vscode.Disposable { const children: vscode.TestItem[] = []; classItem.children.forEach((child) => children.push(child)); - run.started(classItem); - await this.markRunning([classItem]); + // See runClassTests: the class stays out of the run's own reporting; the + // store below is what a tree row reads. + await this.markRunning([classItem], false); let passedCount = 0; for (const child of children) { const selector = parseTestId(child.id).selector!; run.started(child); - await this.markRunning([child]); + await this.markRunning([child], false); const raised = await this.debugSingleTest(session, run, child, className, selector, dictName); if (raised) { - run.errored(classItem, new vscode.TestMessage(`${selector} raised.`)); + // The child already reported the raise; see runClassTests for why the + // class itself stays out of the results list. this.setResult(dictName, className, undefined, { outcome: 'error', passedCount, @@ -759,7 +1041,6 @@ export class SunitTestController implements vscode.Disposable { passedCount += 1; } - run.passed(classItem); this.setResult(dictName, className, undefined, { outcome: 'passed', passedCount, @@ -768,14 +1049,29 @@ export class SunitTestController implements vscode.Disposable { } /** - * Show the tests as running before the (blocking) stone call starts. The - * yield matters: the queries are synchronous, so without handing the event - * loop back the spinner would only ever appear after the answer arrived. + * Show the tests as running before the (blocking) stone call starts, having + * first shown them as never-run. + * + * Both yields matter. The queries are synchronous, so without handing the + * event loop back nothing repaints until the answer has arrived. And the + * blank frame is what makes a re-run visible at all: re-running a test that + * failed and fails again would otherwise repaint the ✗ it was already showing, + * leaving no way to tell whether the run happened. */ - private async markRunning(items: vscode.TestItem[]): Promise { + private async markRunning(items: vscode.TestItem[], stoppable = true): Promise { + for (const item of items) { + const { dictName, className, selector } = parseTestId(item.id); + this.deleteResult(dictName, className, selector); + // A method's outcome is also part of its class's roll-up, which would + // otherwise keep showing the old verdict beside a test being re-run. + if (selector !== undefined) this.deleteResult(dictName, className); + } + this.flushResultChanges(); + await new Promise((resolve) => setImmediate(resolve)); + for (const item of items) { const { dictName, className, selector } = parseTestId(item.id); - this.setResult(dictName, className, selector, { outcome: 'running' }); + this.setResult(dictName, className, selector, { outcome: 'running', stoppable }); } this.flushResultChanges(); await new Promise((resolve) => setImmediate(resolve)); From 43f1a4111ead811f0114a7e5066b137629827ada Mon Sep 17 00:00:00 2001 From: Eric Winger Date: Thu, 20 Aug 2026 15:49:09 -0700 Subject: [PATCH 06/15] Run and watch SUnit tests from the GemStone Explorer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the gap that made the Explorer worth leaving: running tests was browser-only, and the documentation had already stopped advertising the browser. - A test class row — in Classes or Hierarchy — and a test method row carry an inline run action. Both dispatch the same commands the System Browser uses, so a run started anywhere reports in the Testing view. - Each of those rows paints the outcome of its last run, and a result that has gone stale keeps its shape but takes the queued colour, so a green check never quietly outlives the code it described. - While a run is in flight the row's run action is replaced by a stop, and the Testing view's own rows gain one beside VS Code's run icon (which an extension cannot replace). Both drive the same break. - A TestCase subclass with no tests of its own is not offered as runnable: the run would do nothing. One whose count the stone could not report stays runnable, since "no tests found" beats a silently missing button. - Reveal in Testing View on an Explorer row, and Reveal in GemStone Explorer on a Testing view row, so the two navigations can be crossed deliberately. - Clear Test Results wipes both stores — ours, which paints these rows, and VS Code's run history, which paints the Testing view. Clearing only ours left the tester still showing the old verdicts. A method's test items are listed when its document is opened, not only when someone expands the class in the Testing view: VS Code can only draw a gutter icon for a test item it already knows about, and opening a method from the Explorer never expands that row. The `.test` / `.running` / `.debugging` tokens on a row's context value are what gate these actions, so the existing when-clauses that anchored on an exact context value are widened to keep the ordinary class and method actions. Closes #427. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/__tests__/explorerOpenMethod.test.ts | 199 +++++++++++ client/src/extension.ts | 86 ++++- client/src/gemstoneExplorer.ts | 326 +++++++++++++++--- package.json | 173 +++++++++- 4 files changed, 720 insertions(+), 64 deletions(-) diff --git a/client/src/__tests__/explorerOpenMethod.test.ts b/client/src/__tests__/explorerOpenMethod.test.ts index 97c69ffa..46956280 100644 --- a/client/src/__tests__/explorerOpenMethod.test.ts +++ b/client/src/__tests__/explorerOpenMethod.test.ts @@ -7,6 +7,7 @@ vi.mock('../browserQueries', () => ({})); import type * as vscode from 'vscode'; import { ExplorerController, MethodItem } from '../gemstoneExplorer'; +import type { ExplorerTestResult } from '../gemstoneExplorer'; import { Uri, window, commands, workspace, languages } from '../__mocks__/vscode'; import type { SessionManager, ActiveSession } from '../sessionManager'; @@ -271,3 +272,201 @@ describe('ExplorerController.syncToEditor', () => { expect(method.reveal).toHaveBeenCalled(); }); }); + +describe('a click in the Testing view', () => { + const TEST_URI = 'gemstone://1/UserGlobals/Array/instance/accessing/at%3A'; + const ORDINARY_URI = 'gemstone://1/UserGlobals/Array/instance/accessing/size'; + + function ctl(): ExplorerController { + const sessionManager = { getSelectedSession: () => SESSION } as unknown as SessionManager; + const c = new ExplorerController(sessionManager, undefined, undefined, { + isTestClass: () => true, + isTestItemUri: (uri) => uri.toString() === TEST_URI, + resultFor: () => undefined, + onDidChangeResults: () => ({ dispose: () => {} }), + revealInTestExplorer: () => Promise.resolve(true), + }); + c.state.dictName = 'UserGlobals'; + c.state.dictIndex = 1; + c.state.className = 'Array'; + // Both selectors resolve, so each assertion turns on the guard rather than on + // whether a reveal was possible at all. + vi.spyOn(c as unknown as { selectorsFor: typeof info }, 'selectorsFor').mockReturnValue([ + info({ selector: 'at:' }), + info({ selector: 'size' }), + ] as never); + return c; + } + + it('leaves the panes alone — the Testing view navigates on its own', () => { + const c = ctl(); + const method = withViews(c); + + return c.syncToEditor(Uri.parse(TEST_URI)).then(() => { + expect(method.reveal).not.toHaveBeenCalled(); + }); + }); + + it('follows an open someone claimed, even onto a test method', async () => { + // Reveal in GemStone Explorer, and GemStone Search via gemstone.openDocument. + const c = ctl(); + const method = withViews(c); + c.markAttributedOpen(Uri.parse(TEST_URI)); + + await c.syncToEditor(Uri.parse(TEST_URI)); + + expect(method.reveal).toHaveBeenCalled(); + }); + + it('consumes a claim once, so the next unclaimed open is ignored again', async () => { + const c = ctl(); + const method = withViews(c); + c.markAttributedOpen(Uri.parse(TEST_URI)); + await c.syncToEditor(Uri.parse(TEST_URI)); + method.reveal.mockClear(); + + await c.syncToEditor(Uri.parse(TEST_URI)); + + expect(method.reveal).not.toHaveBeenCalled(); + }); + + it('follows an ordinary method that no test item points at', async () => { + const c = ctl(); + const method = withViews(c); + + await c.syncToEditor(Uri.parse(ORDINARY_URI)); + + expect(method.reveal).toHaveBeenCalled(); + }); +}); + +describe('ExplorerController.isTestSelector', () => { + function ctlFor(testClasses: string[]): ExplorerController { + const sessionManager = { getSelectedSession: () => SESSION } as unknown as SessionManager; + const ctl = new ExplorerController(sessionManager, undefined, undefined, { + isTestClass: (_dictName: string, className: string) => testClasses.includes(className), + isTestItemUri: () => false, + resultFor: () => undefined, + onDidChangeResults: () => ({ dispose: () => {} }), + revealInTestExplorer: () => Promise.resolve(true), + }); + ctl.state.dictName = 'UserGlobals'; + ctl.state.className = 'AnnouncerTest'; + return ctl; + } + + it('marks an instance-side unary test* selector of a test class', () => { + expect(ctlFor(['AnnouncerTest']).isTestSelector(false, 'testAnnounceClass')).toBe(true); + }); + + it('leaves the class side alone — SUnit runs instance-side tests', () => { + expect(ctlFor(['AnnouncerTest']).isTestSelector(true, 'testAnnounceClass')).toBe(false); + }); + + it('leaves a keyword selector alone, however it is spelled', () => { + // testSelectors is unary-only; `testFoo:` is a helper, not a test. + expect(ctlFor(['AnnouncerTest']).isTestSelector(false, 'testFoo:')).toBe(false); + }); + + it('leaves setUp and other non-test selectors alone', () => { + const ctl = ctlFor(['AnnouncerTest']); + expect(ctl.isTestSelector(false, 'setUp')).toBe(false); + expect(ctl.isTestSelector(false, 'newAnnouncer')).toBe(false); + }); + + it('marks nothing on a class that is not a TestCase subclass', () => { + expect(ctlFor([]).isTestSelector(false, 'testAnnounceClass')).toBe(false); + }); + + it('carries a .test token only for a test row, so the inline run icon lands there alone', () => { + const ctl = ctlFor(['AnnouncerTest']); + const plain = new MethodItem(false, info({ selector: 'setUp' })); + const test = new MethodItem(false, info({ selector: 'testAnnounceClass' })); + + ctl.decorateTestRow(plain, 'UserGlobals', 'AnnouncerTest', 'setUp'); + ctl.decorateTestRow(test, 'UserGlobals', 'AnnouncerTest', 'testAnnounceClass'); + + expect(plain.contextValue).not.toContain('.test'); + expect(test.contextValue).toContain('.test'); + }); + + it('paints the last-known outcome on the row, and dims it once stale', () => { + const sessionManager = { getSelectedSession: () => SESSION } as unknown as SessionManager; + const results: Record = { + AnnouncerTest: { outcome: 'failed' }, + 'AnnouncerTest/testAnnounceClass': { outcome: 'passed', stale: true }, + }; + const ctl = new ExplorerController(sessionManager, undefined, undefined, { + isTestClass: (_d: string, c: string) => c === 'AnnouncerTest', + isTestItemUri: () => false, + resultFor: (_d: string, c: string, sel?: string) => + results[sel === undefined ? c : `${c}/${sel}`], + onDidChangeResults: () => ({ dispose: () => {} }), + revealInTestExplorer: () => Promise.resolve(true), + }); + ctl.state.dictName = 'UserGlobals'; + ctl.state.className = 'AnnouncerTest'; + + const classRow = new MethodItem(false, info({ selector: 'x' })); + ctl.decorateTestRow(classRow, 'UserGlobals', 'AnnouncerTest'); + const methodRow = new MethodItem(false, info({ selector: 'testAnnounceClass' })); + ctl.decorateTestRow(methodRow, 'UserGlobals', 'AnnouncerTest', 'testAnnounceClass'); + + expect((classRow.iconPath as { id: string }).id).toBe('error'); + // Stale keeps the shape (it did pass) but takes the queued colour. + expect((methodRow.iconPath as { id: string }).id).toBe('pass'); + expect((methodRow.iconPath as { color?: { id: string } }).color?.id).toBe('testing.iconQueued'); + }); + + it('marks a running row so its run button can be swapped for a stop button', () => { + const sessionManager = { getSelectedSession: () => SESSION } as unknown as SessionManager; + const ctl = new ExplorerController(sessionManager, undefined, undefined, { + isTestClass: () => true, + isTestItemUri: () => false, + resultFor: () => ({ outcome: 'running', stoppable: true }), + onDidChangeResults: () => ({ dispose: () => {} }), + revealInTestExplorer: () => Promise.resolve(true), + }); + ctl.state.dictName = 'UserGlobals'; + ctl.state.className = 'AnnouncerTest'; + const row = new MethodItem(false, info({ selector: 'testAnnounceClass' })); + + ctl.decorateTestRow(row, 'UserGlobals', 'AnnouncerTest', 'testAnnounceClass'); + + // The menus anchor on `.test$` for run and `.running$` for stop, so the token + // has to come last. + expect(row.contextValue?.endsWith('.test.running')).toBe(true); + }); + + it('offers no stop button for a test suspended in the debugger', () => { + // The debugger owns the gem; its own Terminate ends the test. A ■ of ours + // would be a button that does nothing. + const sessionManager = { getSelectedSession: () => SESSION } as unknown as SessionManager; + const ctl = new ExplorerController(sessionManager, undefined, undefined, { + isTestClass: () => true, + isTestItemUri: () => false, + resultFor: () => ({ outcome: 'running', stoppable: false }), + onDidChangeResults: () => ({ dispose: () => {} }), + revealInTestExplorer: () => Promise.resolve(true), + }); + ctl.state.dictName = 'UserGlobals'; + ctl.state.className = 'AnnouncerTest'; + const row = new MethodItem(false, info({ selector: 'testAnnounceClass' })); + + ctl.decorateTestRow(row, 'UserGlobals', 'AnnouncerTest', 'testAnnounceClass'); + + expect(row.contextValue).not.toContain('.running'); + // Nor a ▶ mid-run: the row is neither runnable nor stoppable right now. + expect(row.contextValue?.endsWith('.test')).toBe(false); + }); + + it('leaves a row that has never run with the icon it was built with', () => { + const ctl = ctlFor(['AnnouncerTest']); + const row = new MethodItem(false, info({ selector: 'testAnnounceClass' })); + const built = row.iconPath; + + ctl.decorateTestRow(row, 'UserGlobals', 'AnnouncerTest', 'testAnnounceClass'); + + expect(row.iconPath).toBe(built); + }); +}); diff --git a/client/src/extension.ts b/client/src/extension.ts index e8b83956..b8d66009 100644 --- a/client/src/extension.ts +++ b/client/src/extension.ts @@ -562,6 +562,13 @@ export function activate(context: vscode.ExtensionContext) { // Set when GemStone Search registers (below); the class-compile and commit/abort handlers call its // hooks so an open search re-primes/folds in changes instead of going stale. let omniSearch: OmniSearchRegistration | undefined; + // Set when the SUnit controller is built (below, after the Explorer). The Explorer + // asks it which classes are test classes and how each last ran, so its rows can + // offer to run them and show the outcome. + let sunitTests: SunitTestController | undefined; + // Relays the controller's result changes to the Explorer, which is registered first. + const sunitResultsChanged = new vscode.EventEmitter(); + context.subscriptions.push(sunitResultsChanged); // Create every output channel up front — not lazily on first use — so the // full set is discoverable in the Output view's channel dropdown from // activation. (The Class Sync channel is created just after ExportManager is @@ -768,6 +775,17 @@ export function activate(context: vscode.ExtensionContext) { // A removed class has to leave GemStone Search's cached class corpus the same way, but one class at a // time — Remove Class takes the whole subtree with it. (sid, className) => omniSearch?.notifyClassRemoved(sid, className), + { + isTestClass: (dictName, className) => sunitTests?.isTestClass(dictName, className) ?? false, + isTestItemUri: (uri) => sunitTests?.isTestItemUri(uri) ?? false, + resultFor: (dictName, className, selector) => + sunitTests?.resultFor(dictName, className, selector), + // The controller is built after this one, so subscribe through a forwarder + // rather than handing over an event that does not exist yet. + onDidChangeResults: (listener) => sunitResultsChanged.event(listener), + revealInTestExplorer: async (dictName, className, selector) => + (await sunitTests?.revealInTestExplorer(dictName, className, selector)) ?? false, + }, ); // ── GemStone FileSystem Provider ───────────────────────── @@ -950,8 +968,13 @@ export function activate(context: vscode.ExtensionContext) { context.subscriptions.push(codeExecutor); // ── SUnit Test Controller ──────────────────────────────── - const sunitTestController = new SunitTestController(sessionManager, codeExecutor); - context.subscriptions.push(sunitTestController); + // Assigned through `sunitTests` so the Explorer's late-bound predicate (declared + // at the top of activate) can reach it; the Explorer is registered before this. + const sunitTestController = (sunitTests = new SunitTestController(sessionManager, codeExecutor)); + context.subscriptions.push( + sunitTestController, + sunitTestController.onDidChangeResults(() => sunitResultsChanged.fire()), + ); // Keep the pass/fail indicators honest. A compiled method or class definition // means the outcome shown beside it predates the code now in the stone: the @@ -971,6 +994,16 @@ export function activate(context: vscode.ExtensionContext) { }), ); + // A test class lists its methods lazily, but VS Code only draws a gutter run + // icon for a test item it already knows about. Someone who opens a test method + // from the Explorer never expands the Testing view's class row, so resolve on + // open instead of leaving them with no icon. + context.subscriptions.push( + vscode.window.onDidChangeActiveTextEditor((editor) => { + void sunitTestController.ensureTestsForDocument(editor?.document.uri); + }), + ); + // ── Jupyter Notebook Kernels (Grail Python + Smalltalk) ─ const grailNotebookController = new GrailNotebookController(sessionManager); context.subscriptions.push(grailNotebookController); @@ -1281,6 +1314,9 @@ export function activate(context: vscode.ExtensionContext) { uri: vscode.Uri, opts?: { viewColumn?: vscode.ViewColumn; preserveFocus?: boolean; preview?: boolean }, ) => { + // Claim the open: without this the Explorer reads an open of a test method's + // document as a Testing-view row click and leaves its panes where they were. + explorer.markAttributedOpen(uri); const doc = await vscode.workspace.openTextDocument(uri); // `opts` is optional and back-compatible: existing callers pass only the uri and get the // prior behavior (preview in the active group). GemStone Search's Spotter passes a column + @@ -2147,6 +2183,52 @@ export function activate(context: vscode.ExtensionContext) { showTranscript(); }), + // Offered on a row in the Testing view. A plain click there deliberately does + // not move the Explorer (the two navigations are independent), so this is how + // you ask for it. + vscode.commands.registerCommand( + 'gemstone.revealTestInExplorer', + async (item?: { uri?: vscode.Uri }) => { + if (item?.uri) await explorer.revealDocument(item.uri); + }, + ), + + // Offered from menus rather than as a button, so it takes no room from the rows + // it is about. Two stores hold outcomes and both have to go: ours, which paints + // the Explorer rows, and VS Code's own run history, which paints the Testing + // view — clearing only ours leaves the tester still showing the old verdicts. + // Shift+Enter in GemStone Search, and anywhere else that knows a class/selector + // but not a test item. Says so when the Testing view has nothing for it, rather + // than appearing to do nothing. + vscode.commands.registerCommand( + 'gemstone.revealTestInTestingView', + async (dictName: string, className: string, selector?: string) => { + if (!(await sunitTestController.revealInTestExplorer(dictName, className, selector))) { + void vscode.window.showInformationMessage( + `The Testing view has no test for ${className}${selector ? `>>${selector}` : ''}.`, + ); + } + }, + ), + + // The ■ that replaces a row's ▶ while its test is running. Soft break first, + // hard break if pressed again — the same escalation the progress toast offers. + vscode.commands.registerCommand('gemstone.explorer.stopTest', () => { + if (!sunitTestController.cancelActiveRun()) { + void vscode.window.showInformationMessage('No GemStone test run to stop.'); + } + }), + + vscode.commands.registerCommand('gemstone.clearTestResults', async () => { + sunitTestController.clearResults(); + try { + await vscode.commands.executeCommand('testing.clearTestResults'); + } catch { + // A built-in command id we don't own. If a VS Code release renames it, the + // Explorer icons still clear rather than the whole command failing. + } + }), + vscode.commands.registerCommand( 'gemstone.runSunitClass', async (args: { dictName: string; className: string }) => { diff --git a/client/src/gemstoneExplorer.ts b/client/src/gemstoneExplorer.ts index 7c61b803..e7ffdd48 100644 --- a/client/src/gemstoneExplorer.ts +++ b/client/src/gemstoneExplorer.ts @@ -640,6 +640,66 @@ interface ExplorerViews { // ── Controller ─────────────────────────────────────────────────────────────── +/** The last-known outcome of one test class or test method, as the Explorer needs it. */ +export interface ExplorerTestResult { + outcome: 'running' | 'passed' | 'failed' | 'error'; + /** Running results only: whether the run can be broken. False under the debugger, + * which owns the suspended gem and ends it with its own Terminate. */ + stoppable?: boolean; + /** The code changed since this ran, so it describes something no longer in the stone. */ + stale?: boolean; +} + +/** + * What the Explorer needs from the SUnit controller to show test affordances on + * its rows. Narrow on purpose — the Explorer neither runs tests nor knows how + * they run; it marks the rows that can be run and paints the last outcome. + */ +export interface ExplorerSunitHooks { + isTestClass(dictName: string, className: string): boolean; + /** True when a URI is the document a test item points at. Lets the Explorer leave its + * panes alone for a Testing-view row click, which is an open it did not cause. */ + isTestItemUri(uri: vscode.Uri): boolean; + resultFor(dictName: string, className: string, selector?: string): ExplorerTestResult | undefined; + onDidChangeResults: vscode.Event; + /** Select and scroll to this class, or one of its test methods, in the Testing view. + * False when there is nothing there to reveal. */ + revealInTestExplorer(dictName: string, className: string, selector?: string): Promise; +} + +/** + * The row icon for an outcome. A stale result keeps its shape but takes the + * queued colour: what it says was true of code that has since been recompiled, + * so it should read as "was passing" rather than "is passing". + */ +function testResultIcon(result: ExplorerTestResult): vscode.ThemeIcon { + if (result.outcome === 'running') return new vscode.ThemeIcon('loading~spin'); + const [icon, color] = + result.outcome === 'passed' + ? ['pass', 'testing.iconPassed'] + : result.outcome === 'failed' + ? ['error', 'testing.iconFailed'] + : ['warning', 'testing.iconErrored']; + return new vscode.ThemeIcon( + icon, + new vscode.ThemeColor(result.stale ? 'testing.iconQueued' : color), + ); +} + +function testResultTooltip(result: ExplorerTestResult): string { + const said = + result.outcome === 'running' + ? 'Running…' + : result.outcome === 'passed' + ? 'Last run: passed' + : result.outcome === 'failed' + ? 'Last run: failed' + : 'Last run: error'; + return result.stale && result.outcome !== 'running' + ? `${said} — before the code was recompiled` + : said; +} + export class ExplorerController { readonly state: ExplorerState = {}; // className → category for the current dictionary; fetched once per dict. @@ -722,6 +782,19 @@ export class ExplorerController { // letting the earlier open's event slip past the guard and re-reveal (scroll) the // Methods pane. private readonly selfOpenedUris = new Set(); + // URIs someone is about to open deliberately — GemStone Search through + // `gemstone.openDocument`, or Reveal in GemStone Explorer from a test row. + // VS Code gives no way to ask where an open came from, so a Testing-view row + // click is recognised by elimination: an open of a test item's URI that nobody + // claimed. Claiming the deliberate ones keeps them navigating as they always have. + private readonly attributedOpens = new Set(); + + /** Claim the next open of `uri`, so syncToEditor treats it as a deliberate + * navigation rather than a Testing-view row click. */ + markAttributedOpen(uri: vscode.Uri): void { + this.attributedOpens.add(uri.toString()); + } + // Owns where our source editors land. Balances "open to the side" across only // our own groups, so we neither clump nor invade the System Browser's group. readonly placement = new SourceEditorPlacement(); @@ -744,8 +817,77 @@ export class ExplorerController { /** Called once per class removed by Remove Class, so views holding a cached class corpus (GemStone * Search) can drop it. Per class, not per command: the delete takes the whole subtree. */ private readonly onClassRemoved?: (sessionId: number, className: string) => void, + /** Test affordances on class/method rows. Absent in tests that don't exercise them, + * and before the SUnit controller exists. */ + private readonly sunit?: ExplorerSunitHooks, ) {} + /** + * True when SUnit would run this selector of the class the Methods pane is + * showing — i.e. the row should offer to run it. + * + * Matches GemStone's own `TestCase>>testSelectors`: an instance-side unary + * selector beginning with 'test', on a class discovered as a TestCase + * subclass. Decided from the selector rather than by asking the SUnit + * controller for the class's test methods, because those are listed lazily + * and this is answered while building rows, synchronously. A selector that + * slips through runs and reports that it found no such test. + */ + isTestSelector(isMeta: boolean, selector: string): boolean { + if (isMeta) return false; + const { dictName, className } = this.state; + if (dictName === undefined || className === undefined) return false; + if (!this.sunit?.isTestClass(dictName, className)) return false; + return selector.startsWith('test') && !selector.includes(':'); + } + + /** + * Give a rendered row its test affordances: a `.test` token on the context + * value — which is what puts the inline ▶ there and nowhere else — and an icon + * for the last-known outcome. + * + * One helper for all three panes so a class row in Classes, the same class in + * Hierarchy, and its methods all say the same thing about the same run. A row + * that has never been run keeps the icon it was built with; only a result + * replaces it. + * + * Pass `selector` for a method row; omit it for a class row. + */ + decorateTestRow( + item: vscode.TreeItem, + dictName: string | undefined, + className: string, + selector?: string, + isMeta = false, + ): void { + if (dictName === undefined || !this.sunit?.isTestClass(dictName, className)) return; + // A test class's non-test methods (setUp, helpers) are not runnable rows. + if ( + selector !== undefined && + !(!isMeta && selector.startsWith('test') && !selector.includes(':')) + ) + return; + + item.contextValue = `${item.contextValue ?? ''}.test`; + const result = this.sunit.resultFor(dictName, className, selector); + if (!result) return; + // A running row swaps its ▶ for a ■ — see the `.running` when-clauses. The + // token goes last so the menus can anchor on `.test$` vs `.running$`. A test + // suspended in the debugger gets neither: there is no ▶ to offer mid-run, and + // nothing our ■ could break. + if (result.outcome === 'running' && result.stoppable) { + item.contextValue = `${item.contextValue}.running`; + } else if (result.outcome === 'running') { + item.contextValue = `${item.contextValue}.debugging`; + } + item.iconPath = testResultIcon(result); + const note = testResultTooltip(result); + item.tooltip = + typeof item.tooltip === 'string' && item.tooltip.length > 0 + ? `${item.tooltip}\n${note}` + : note; + } + session(): ActiveSession | undefined { return this.sessionManager.getSelectedSession(); } @@ -1632,7 +1774,7 @@ export class ExplorerController { const e = this.hierChain[i]; const isSelf = i === lastIdx; const hasChildren = !isSelf || this.hierSubs.length > 0; - return new HierarchyItem( + const item = new HierarchyItem( e.className, e.dictName, isSelf ? 'self' : 'ancestor', @@ -1640,22 +1782,27 @@ export class ExplorerController { hasChildren, this.classVersion(e.className), ); + // Each row carries its own dictionary — an ancestor often lives in another + // one — so the affordance and the outcome are for the right class. + this.decorateTestRow(item, e.dictName, e.className); + return item; }; if (!element) return [chainItem(0)]; if (element.role === 'subclass') return []; if (element.chainIndex < lastIdx) return [chainItem(element.chainIndex + 1)]; // element is the current class → list its subclasses. - return this.hierSubs.map( - (s) => - new HierarchyItem( - s.className, - s.dictName, - 'subclass', - -1, - false, - this.classVersion(s.className), - ), - ); + return this.hierSubs.map((s) => { + const item = new HierarchyItem( + s.className, + s.dictName, + 'subclass', + -1, + false, + this.classVersion(s.className), + ); + this.decorateTestRow(item, s.dictName, s.className); + return item; + }); } // Select the current class's node in the Hierarchy pane so its selection stays @@ -3199,16 +3346,23 @@ export class ExplorerController { // methodCategoryMatchesFilter answers false for them. this.methodCategoryMatchesFilter(info.category, filter), ) - .map( - (info) => - new MethodItem( - isMeta, - info, - undefined, - this.methodSourceUri(isMeta, info), - this.ivarAccessMark(isMeta, info.selector, filter), - ), - ); + .map((info) => { + const item = new MethodItem( + isMeta, + info, + undefined, + this.methodSourceUri(isMeta, info), + this.ivarAccessMark(isMeta, info.selector, filter), + ); + this.decorateTestRow( + item, + this.state.dictName, + this.state.className ?? '', + info.selector, + isMeta, + ); + return item; + }); } // Lazily load + cache the per-method instance-variable read/write map for the @@ -3819,6 +3973,11 @@ export class ExplorerController { if (this.selfOpenedUris.delete(uri.toString())) { return; } + // Nobody claimed this open and it lands on a test item's document: it is a + // click on a row in the Testing view, whose navigation is its own. + if (!this.attributedOpens.delete(uri.toString()) && this.sunit?.isTestItemUri(uri)) { + return; + } const session = this.session(); if (!session || String(session.id) !== uri.authority) return; @@ -5020,17 +5179,16 @@ class ClassProvider extends RefreshableProvider { getChildren(element?: ClassNode | FilterChipItem): (ClassNode | FilterChipItem)[] { if (this.ctl.state.dictName === undefined || element instanceof FilterChipItem) return []; if (!element) { - const rows = this.ctl - .classNames() - .map( - (n) => - new ClassItem( - n, - this.ctl.classHasDefinedVars(n), - this.ctl.classVersion(n), - this.ctl.classHasComment(n), - ), + const rows = this.ctl.classNames().map((n) => { + const item = new ClassItem( + n, + this.ctl.classHasDefinedVars(n), + this.ctl.classVersion(n), + this.ctl.classHasComment(n), ); + this.ctl.decorateTestRow(item, this.ctl.state.dictName, n); + return item; + }); return withFilterChip(VIEW_CLASSES, this.ctl, rows); } // A class expands to an "instance" and/or "class" variable-side node (like the @@ -5123,16 +5281,23 @@ class MethodProvider extends RefreshableProvider { nameMatched || this.ctl.methodMatchesFilter(element.isMeta, info.selector, filter), ) - .map( - (info) => - new MethodItem( - element.isMeta, - info, - element.category, - this.ctl.methodSourceUri(element.isMeta, info), - this.ctl.ivarAccessMark(element.isMeta, info.selector, filter), - ), - ); + .map((info) => { + const item = new MethodItem( + element.isMeta, + info, + element.category, + this.ctl.methodSourceUri(element.isMeta, info), + this.ctl.ivarAccessMark(element.isMeta, info.selector, filter), + ); + this.ctl.decorateTestRow( + item, + this.ctl.state.dictName, + this.ctl.state.className ?? '', + info.selector, + element.isMeta, + ); + return item; + }); } return []; } @@ -5211,6 +5376,12 @@ export interface ExplorerHandle { onMethodCompiled(sessionId: number, className: string): void; onClassCompiled(sessionId: number, className: string, dictName?: string): void; onSessionAborted(sessionId: number): void; + /** Claim an about-to-happen open so it navigates the panes; see + * ExplorerController.markAttributedOpen. */ + markAttributedOpen(uri: vscode.Uri): void; + /** Navigate the panes to `uri`'s class/method — the explicit Reveal action a + * Testing-view row offers, since a plain click deliberately does not. */ + revealDocument(uri: vscode.Uri): Promise; } export function registerGemStoneExplorer( @@ -5226,8 +5397,27 @@ export function registerGemStoneExplorer( // Called once per class that Remove Class actually deleted, so GemStone Search can drop it from its // cached corpus instead of showing (and offering to open) a class that no longer exists. onClassRemoved?: (sessionId: number, className: string) => void, + // True when a URI is the document a SUnit test item points at; see + // ExplorerController.isTestItemUri. Late-bound, because the SUnit controller is + // built after this one. + // Test affordances on class/method rows. Late-bound, because the SUnit controller is + // built after this one. + sunit?: ExplorerSunitHooks, ): ExplorerHandle { - const ctl = new ExplorerController(sessionManager, onSymbolListChanged, onClassRemoved); + const ctl = new ExplorerController(sessionManager, onSymbolListChanged, onClassRemoved, sunit); + + // A run starting or finishing changes what these rows should say, so repaint the + // three panes that carry test affordances. Cheap — the providers rebuild rows from + // state already fetched, with no trip to the stone. + if (sunit) { + context.subscriptions.push( + sunit.onDidChangeResults(() => { + ctl.classProvider.refresh(); + ctl.hierarchyProvider.refresh(); + ctl.methodProvider.refresh(); + }), + ); + } // The Open Editors pane (last in the container) mirrors the open gemstone:// // source editors; it is session-independent, so it registers on its own. @@ -5347,6 +5537,51 @@ export function registerGemStoneExplorer( vscode.commands.registerCommand('gemstone.explorer.openMethodToSide', (node: MethodItem) => { if (node instanceof MethodItem) void ctl.openMethod(node, 'pin'); }), + // The inline ▶ on a test method row. Runs through the same command the System + // Browser uses, so the result lands in the Testing view like every other run. + vscode.commands.registerCommand('gemstone.explorer.runTestMethod', (node?: MethodItem) => { + if (!(node instanceof MethodItem)) return; + const { dictName, className } = ctl.state; + if (dictName === undefined || className === undefined) return; + void vscode.commands.executeCommand('gemstone.runSunitMethods', dictName, className, [ + node.info.selector, + ]); + }), + // The mirror of Reveal in GemStone Explorer: go from a row here to the same + // test in the Testing view. Offered on the rows that carry a `.test` token, + // so it is never on a row the Testing view has nothing for. + vscode.commands.registerCommand( + 'gemstone.explorer.revealInTestingView', + async (node?: ClassItem | HierarchyItem | MethodItem) => { + if (!node || !sunit) return; + const dictName = node instanceof HierarchyItem ? node.dictName : ctl.state.dictName; + // A method row names its class through the pane's current selection; a class + // or hierarchy row names it directly. + const className = node instanceof MethodItem ? ctl.state.className : node.className; + const selector = node instanceof MethodItem ? node.info.selector : undefined; + if (dictName === undefined || className === undefined) return; + if (!(await sunit.revealInTestExplorer(dictName, className, selector))) { + void vscode.window.showInformationMessage( + `The Testing view has no test for ${className}${selector ? `>>${selector}` : ''}.`, + ); + } + }, + ), + + // The inline ▶ on a test class row, in the Classes pane or the Hierarchy pane. + // A hierarchy row carries its own dictionary — an ancestor test class often + // lives in a different one than the class being browsed. + vscode.commands.registerCommand( + 'gemstone.explorer.runTestClass', + (node?: ClassItem | HierarchyItem) => { + const dictName = node instanceof HierarchyItem ? node.dictName : ctl.state.dictName; + if (!node || dictName === undefined) return; + void vscode.commands.executeCommand('gemstone.runSunitClass', { + dictName, + className: node.className, + }); + }, + ), vscode.commands.registerCommand('gemstone.explorer.removeMethod', (node?: MethodItem) => { if (node instanceof MethodItem) void ctl.removeMethod(node).catch((e: unknown) => { @@ -5809,5 +6044,10 @@ export function registerGemStoneExplorer( onClassCompiled: (sessionId, className, dictName) => ctl.onExternalClassCompiled(sessionId, className, dictName), onSessionAborted: (sessionId) => ctl.onSessionAborted(sessionId), + markAttributedOpen: (uri) => ctl.markAttributedOpen(uri), + revealDocument: async (uri) => { + ctl.markAttributedOpen(uri); + await ctl.syncToEditor(uri); + }, }; } diff --git a/package.json b/package.json index 1a663497..bec7d0a6 100644 --- a/package.json +++ b/package.json @@ -1196,6 +1196,40 @@ "category": "GemStone", "icon": "$(pin)" }, + { + "command": "gemstone.explorer.runTestMethod", + "title": "Run Test", + "category": "GemStone", + "icon": "$(run)" + }, + { + "command": "gemstone.explorer.runTestClass", + "title": "Run Tests", + "category": "GemStone", + "icon": "$(run-all)" + }, + { + "command": "gemstone.revealTestInExplorer", + "title": "Reveal in GemStone Explorer", + "category": "GemStone", + "icon": "$(list-tree)" + }, + { + "command": "gemstone.clearTestResults", + "title": "Clear Test Results", + "category": "GemStone" + }, + { + "command": "gemstone.explorer.stopTest", + "title": "Stop Test Run", + "category": "GemStone", + "icon": "$(debug-stop)" + }, + { + "command": "gemstone.explorer.revealInTestingView", + "title": "Reveal in Testing View", + "category": "GemStone" + }, { "command": "gemstone.explorer.removeMethod", "title": "Remove Method", @@ -2045,7 +2079,43 @@ "when": "gemstone.hasActiveSession" } ], + "testing/item/context": [ + { + "command": "gemstone.explorer.stopTest", + "when": "gemstone.testRunning", + "group": "inline@0" + }, + { + "command": "gemstone.revealTestInExplorer", + "group": "inline@1" + }, + { + "command": "gemstone.explorer.stopTest", + "when": "gemstone.testRunning", + "group": "gemstone@0" + }, + { + "command": "gemstone.revealTestInExplorer", + "group": "gemstone@1" + }, + { + "command": "gemstone.clearTestResults", + "group": "gemstone@2" + } + ], "view/title": [ + { + "command": "gemstone.clearTestResults", + "when": "view == gemstoneExplorerClasses" + }, + { + "command": "gemstone.clearTestResults", + "when": "view == gemstoneExplorerMethods" + }, + { + "command": "gemstone.clearTestResults", + "when": "view == gemstoneExplorerClassHierarchy" + }, { "command": "gemstone.explorer.refresh", "when": "view == gemstoneExplorerDicts", @@ -2253,6 +2323,71 @@ "when": "view == gemstoneExplorerMethods && viewItem =~ /^explorerMethod/", "group": "inline@3" }, + { + "command": "gemstone.explorer.runTestMethod", + "when": "view == gemstoneExplorerMethods && viewItem =~ /\\.test$/", + "group": "inline@0" + }, + { + "command": "gemstone.explorer.stopTest", + "when": "viewItem =~ /\\.running$/", + "group": "inline@0" + }, + { + "command": "gemstone.explorer.runTestClass", + "when": "view == gemstoneExplorerClasses && viewItem =~ /\\.test$/", + "group": "inline@0" + }, + { + "command": "gemstone.explorer.runTestClass", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /\\.test$/", + "group": "inline@0" + }, + { + "command": "gemstone.explorer.runTestClass", + "when": "view == gemstoneExplorerClasses && viewItem =~ /\\.test$/", + "group": "1_browse@0" + }, + { + "command": "gemstone.explorer.runTestClass", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /\\.test$/", + "group": "1_browse@0" + }, + { + "command": "gemstone.explorer.revealInTestingView", + "when": "view == gemstoneExplorerClasses && viewItem =~ /\\.test/", + "group": "1_browse@1" + }, + { + "command": "gemstone.explorer.revealInTestingView", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /\\.test/", + "group": "1_browse@1" + }, + { + "command": "gemstone.explorer.revealInTestingView", + "when": "view == gemstoneExplorerMethods && viewItem =~ /\\.test/", + "group": "1_browse@1" + }, + { + "command": "gemstone.clearTestResults", + "when": "view == gemstoneExplorerClasses && viewItem =~ /\\.test$/", + "group": "1_browse@2" + }, + { + "command": "gemstone.clearTestResults", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /\\.test$/", + "group": "1_browse@2" + }, + { + "command": "gemstone.clearTestResults", + "when": "view == gemstoneExplorerMethods && viewItem =~ /\\.test$/", + "group": "1_browse@2" + }, + { + "command": "gemstone.explorer.runTestMethod", + "when": "view == gemstoneExplorerMethods && viewItem =~ /\\.test$/", + "group": "1_browse@0" + }, { "command": "gemstone.explorer.closeOpenEditor", "when": "view == gemstoneExplorerOpenEditors && viewItem == explorerOpenEditorItem", @@ -2280,7 +2415,7 @@ }, { "command": "gemstone.explorer.openDefinitionToSide", - "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?$/", + "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?(\\.test(\\.running|\\.debugging)?)?$/", "group": "inline@1" }, { @@ -2345,12 +2480,12 @@ }, { "command": "gemstone.explorer.addInstVar", - "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?$/ && gemstone.rbSupportAvailable", + "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?(\\.test(\\.running|\\.debugging)?)?$/ && gemstone.rbSupportAvailable", "group": "2_generate@2" }, { "command": "gemstone.explorer.addClassVar", - "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?$/", + "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?(\\.test(\\.running|\\.debugging)?)?$/", "group": "2_generate@3" }, { @@ -2415,82 +2550,82 @@ }, { "command": "gemstone.explorer.renameClass", - "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?$/ && gemstone.rbSupportAvailable", + "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?(\\.test(\\.running|\\.debugging)?)?$/ && gemstone.rbSupportAvailable", "group": "inline@3" }, { "command": "gemstone.explorer.classHistory", - "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?$/ && gemstone.rbSupportAvailable", + "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?(\\.test(\\.running|\\.debugging)?)?$/ && gemstone.rbSupportAvailable", "group": "3_refactor@1" }, { "command": "gemstone.explorer.insertSuperclass", - "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?$/ && gemstone.rbSupportAvailable", + "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?(\\.test(\\.running|\\.debugging)?)?$/ && gemstone.rbSupportAvailable", "group": "3_refactor@2" }, { "command": "gemstone.explorer.extractSuperclass", - "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?$/ && gemstone.rbSupportAvailable", + "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?(\\.test(\\.running|\\.debugging)?)?$/ && gemstone.rbSupportAvailable", "group": "3_refactor@3" }, { "command": "gemstone.explorer.splitClass", - "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?$/ && gemstone.rbSupportAvailable", + "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?(\\.test(\\.running|\\.debugging)?)?$/ && gemstone.rbSupportAvailable", "group": "3_refactor@4" }, { "command": "gemstone.explorer.openHierarchyDefinition", - "when": "view == gemstoneExplorerClassHierarchy && viewItem == explorerHierClass", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /^explorerHierClass(\\.test(\\.running|\\.debugging)?)?$/", "group": "inline@1" }, { "command": "gemstone.explorer.openHierarchyComment", - "when": "view == gemstoneExplorerClassHierarchy && viewItem == explorerHierClass", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /^explorerHierClass(\\.test(\\.running|\\.debugging)?)?$/", "group": "inline@2" }, { "command": "gemstone.explorer.renameClass", - "when": "view == gemstoneExplorerClassHierarchy && viewItem == explorerHierClass && gemstone.rbSupportAvailable", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /^explorerHierClass(\\.test(\\.running|\\.debugging)?)?$/ && gemstone.rbSupportAvailable", "group": "inline@3" }, { "command": "gemstone.explorer.classHistory", - "when": "view == gemstoneExplorerClassHierarchy && viewItem == explorerHierClass && gemstone.rbSupportAvailable", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /^explorerHierClass(\\.test(\\.running|\\.debugging)?)?$/ && gemstone.rbSupportAvailable", "group": "3_refactor@1" }, { "command": "gemstone.explorer.insertSuperclass", - "when": "view == gemstoneExplorerClassHierarchy && viewItem == explorerHierClass && gemstone.rbSupportAvailable", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /^explorerHierClass(\\.test(\\.running|\\.debugging)?)?$/ && gemstone.rbSupportAvailable", "group": "3_refactor@2" }, { "command": "gemstone.explorer.extractSuperclass", - "when": "view == gemstoneExplorerClassHierarchy && viewItem == explorerHierClass && gemstone.rbSupportAvailable", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /^explorerHierClass(\\.test(\\.running|\\.debugging)?)?$/ && gemstone.rbSupportAvailable", "group": "3_refactor@3" }, { "command": "gemstone.explorer.splitClass", - "when": "view == gemstoneExplorerClassHierarchy && viewItem == explorerHierClass && gemstone.rbSupportAvailable", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /^explorerHierClass(\\.test(\\.running|\\.debugging)?)?$/ && gemstone.rbSupportAvailable", "group": "3_refactor@4" }, { "command": "gemstone.generateGrailStub", - "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?$/", + "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?(\\.test(\\.running|\\.debugging)?)?$/", "group": "2_generate@1" }, { "command": "gemstone.generateGrailStub", - "when": "view == gemstoneExplorerClassHierarchy && viewItem == explorerHierClass", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /^explorerHierClass(\\.test(\\.running|\\.debugging)?)?$/", "group": "2_generate@1" }, { "command": "gemstone.explorer.removeClass", - "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?$/", + "when": "view == gemstoneExplorerClasses && viewItem =~ /^explorerClass(\\.commented)?(\\.test(\\.running|\\.debugging)?)?$/", "group": "inline@9" }, { "command": "gemstone.explorer.removeClass", - "when": "view == gemstoneExplorerClassHierarchy && viewItem == explorerHierClass", + "when": "view == gemstoneExplorerClassHierarchy && viewItem =~ /^explorerHierClass(\\.test(\\.running|\\.debugging)?)?$/", "group": "inline@9" }, { From e046e17facd846658b72ab0e4b681b03b84c90a2 Mon Sep 17 00:00:00 2001 From: Eric Winger Date: Thu, 20 Aug 2026 15:49:25 -0700 Subject: [PATCH 07/15] GemStone Search: Shift+Enter goes to the result's test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enter opens a result, but a result that IS a test is often something you want to run rather than read, and the Testing view was reachable only by finding the class again by hand. Shift+Enter reveals the result there — a class result by its class, a method result down to its selector. It reads the result's own action rather than its label: the label is display text, the action is what the result stands for. The helper lives in omniActions, which neither host imports back. Putting it in omniSearchCommand — which the view provider already imports — made a cycle, and the message handler's catch swallowed the resulting failure, so the gesture looked like it did nothing at all. Co-Authored-By: Claude Opus 5 (1M context) --- .../omniSearch/__tests__/omniActions.test.ts | 65 ++++++++++++++++++- .../__tests__/omniSearchView.test.ts | 15 +++++ client/src/omniSearch/omniActions.ts | 29 ++++++++- client/src/omniSearch/omniSearchPanel.ts | 9 +++ client/src/omniSearch/omniSearchShared.ts | 2 +- client/src/omniSearch/omniSearchView.js | 6 +- .../src/omniSearch/omniSearchViewProvider.ts | 7 ++ 7 files changed, 127 insertions(+), 6 deletions(-) diff --git a/client/src/omniSearch/__tests__/omniActions.test.ts b/client/src/omniSearch/__tests__/omniActions.test.ts index 1e21dc7b..7e4068f5 100644 --- a/client/src/omniSearch/__tests__/omniActions.test.ts +++ b/client/src/omniSearch/__tests__/omniActions.test.ts @@ -1,6 +1,10 @@ import { describe, it, expect, vi } from 'vitest'; -import { runOmniAction, OmniActionHandlers } from '../omniActions'; -import { OmniAction } from '../omniTypes'; + +vi.mock('vscode', () => import('../../__mocks__/vscode.js')); + +import { commands } from '../../__mocks__/vscode'; +import { runOmniAction, OmniActionHandlers, revealTestForResult } from '../omniActions'; +import { OmniAction, OmniResult } from '../omniTypes'; function handlers(): OmniActionHandlers & { calls: Record } { const calls: Record = {}; @@ -66,3 +70,60 @@ describe('runOmniAction', () => { expect(done).toBe(true); }); }); + +describe('revealTestForResult', () => { + function result(action: OmniAction): OmniResult { + return { categoryId: 'classes', label: 'x', score: 1, ranges: [], action }; + } + + it('reveals a class result by its dictionary and class', async () => { + await revealTestForResult( + result({ + kind: 'openClass', + sessionId: 1, + dictName: 'Published', + className: 'RsrStressTest', + dictIndex: 2, + }), + ); + + expect(commands.executeCommand).toHaveBeenCalledWith( + 'gemstone.revealTestInTestingView', + 'Published', + 'RsrStressTest', + ); + }); + + it('reveals a method result down to its selector', async () => { + await revealTestForResult( + result({ + kind: 'openMethod', + sessionId: 1, + dictName: 'Published', + className: 'RsrStressTest', + isMeta: false, + category: 'tests', + selector: 'test1KBytes', + environmentId: 0, + dictIndex: 2, + }), + ); + + expect(commands.executeCommand).toHaveBeenCalledWith( + 'gemstone.revealTestInTestingView', + 'Published', + 'RsrStressTest', + 'test1KBytes', + ); + }); + + it('does nothing for a result that is neither a class nor a method', async () => { + vi.mocked(commands.executeCommand).mockClear(); + + await revealTestForResult( + result({ kind: 'revealDictionary', sessionId: 1, dictName: 'Published' }), + ); + + expect(commands.executeCommand).not.toHaveBeenCalled(); + }); +}); diff --git a/client/src/omniSearch/__tests__/omniSearchView.test.ts b/client/src/omniSearch/__tests__/omniSearchView.test.ts index 364d837e..d12b53ba 100644 --- a/client/src/omniSearch/__tests__/omniSearchView.test.ts +++ b/client/src/omniSearch/__tests__/omniSearchView.test.ts @@ -597,6 +597,21 @@ describe('GemStone Search view — keyboard', () => { expect(vscode.postMessage).toHaveBeenCalledWith({ command: 'activate', id: 0, side: true }); }); + it('Shift+Enter asks the host to reveal the active row in the Testing view', () => { + const { handle, vscode } = mount(); + seed(handle); + + keydown({ key: 'Enter', shiftKey: true }); + + expect(vscode.postMessage).toHaveBeenCalledWith({ command: 'revealTest', id: 0 }); + // and not also opened + expect(vscode.postMessage).not.toHaveBeenCalledWith({ + command: 'activate', + id: 0, + side: false, + }); + }); + it('Alt+Enter pivots to references of the active row', () => { const { handle, vscode } = mount(); seed(handle); diff --git a/client/src/omniSearch/omniActions.ts b/client/src/omniSearch/omniActions.ts index 2b5d57ea..42d02519 100644 --- a/client/src/omniSearch/omniActions.ts +++ b/client/src/omniSearch/omniActions.ts @@ -3,7 +3,8 @@ * the SystemBrowser, and the `gemstone:` uri builders) are injected by `omniSearchCommand.ts`, so * this stays unit-testable and the `switch` is exhaustiveness-checked at compile time. */ -import { OmniAction } from './omniTypes'; +import * as vscode from 'vscode'; +import { OmniAction, OmniResult } from './omniTypes'; type ByKind = Extract; @@ -43,3 +44,29 @@ export async function runOmniAction( assertNever(action); } } + +/** + * Select a result's test in the Testing view, when it has one. Reads the result's + * own action rather than its label — the label is display text, the action is the + * class/selector the result actually stands for. A result that isn't a test class + * or test method is left alone by the command, which says so. + */ +export async function revealTestForResult(result: OmniResult): Promise { + const a = result.action; + if (a.kind === 'openMethod') { + await vscode.commands.executeCommand( + 'gemstone.revealTestInTestingView', + a.dictName, + a.className, + a.selector, + ); + return; + } + if (a.kind === 'openClass') { + await vscode.commands.executeCommand( + 'gemstone.revealTestInTestingView', + a.dictName, + a.className, + ); + } +} diff --git a/client/src/omniSearch/omniSearchPanel.ts b/client/src/omniSearch/omniSearchPanel.ts index fbc10e54..78a8ba49 100644 --- a/client/src/omniSearch/omniSearchPanel.ts +++ b/client/src/omniSearch/omniSearchPanel.ts @@ -15,6 +15,7 @@ import * as vscode from 'vscode'; import { OmniConfig, OmniResult } from './omniTypes'; import { createOmniEngine, OmniEngineDeps, OmniViewData } from './omniEngine'; +import { revealTestForResult } from './omniActions'; import { configMessage, dispatchEngineMessage, @@ -48,6 +49,7 @@ type OmniInbound = | { command: 'loadMore' } | { command: 'loadAll' } | { command: 'activate'; id: number; side: boolean } + | { command: 'revealTest'; id: number } | { command: 'references'; id: number } | { command: 'referencesInline'; id: number } | { command: 'previewReference'; refId: number } @@ -217,6 +219,13 @@ export class OmniSearchPanel { if (!this.pinned) this.panel.dispose(); return; } + case 'revealTest': { + // Shift+Enter: go to the result in the Testing view instead of opening it. + // The Spotter stays put — you are moving to another view, not dismissing this one. + const result = this.engine.resultFor(m.id); + if (result) await revealTestForResult(result); + return; + } case 'preview': { const result = this.engine.resultFor(m.id); if (!result) return; diff --git a/client/src/omniSearch/omniSearchShared.ts b/client/src/omniSearch/omniSearchShared.ts index b6778687..d102942f 100644 --- a/client/src/omniSearch/omniSearchShared.ts +++ b/client/src/omniSearch/omniSearchShared.ts @@ -609,7 +609,7 @@ export function renderOmniHtml(opts: { showPin: boolean }): string {