Skip to content

Run SUnit tests from the GemStone Explorer, and let a run be stopped - #474

Merged
ericwinger merged 17 commits into
mainfrom
eric/issue427-sunit-run
Aug 21, 2026
Merged

Run SUnit tests from the GemStone Explorer, and let a run be stopped#474
ericwinger merged 17 commits into
mainfrom
eric/issue427-sunit-run

Conversation

@ericwinger

@ericwinger ericwinger commented Aug 20, 2026

Copy link
Copy Markdown
Member

Running SUnit tests was the one place where "prefer the Explorer" still cost a real
feature, and the documentation had already stopped advertising the System Browser.
This closes that, and fixes several things found while making it work.

Running tests from the Explorer

  • A test class row — in Classes or Hierarchy — and a test method row in
    Methods carry an inline ▶. Both dispatch the same commands the System Browser
    uses, so a run started anywhere reports in the Testing view.
  • Each of those rows shows the outcome of its last run. A result that has gone stale
    (the code was recompiled since) keeps its shape but takes the queued colour, so a
    green check never quietly outlives the code it described.
  • 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.
  • Navigation between the three places you might be looking from, so none of them is a
    dead end you have to leave by finding the class again by hand:
    • Shift+Enter on a GemStone Search result opens that class or method in the
      Testing view
      — selected and scrolled to, with the view brought up if it wasn't
      showing. A method result reveals down to its own test row, not just its class.
    • Reveal in Testing View does the same from an Explorer class or method row.
    • Reveal in GemStone Explorer goes the other way, from a Testing view row.
  • Clear Test Results wipes both stores: ours, which paints the Explorer rows, and
    VS Code's run history, which paints the Testing view. Clearing only ours would leave
    the tester still showing the old verdicts. It is offered in four places, none of
    which take room from the rows they are about:
    • right-click a test class or test method row in the Explorer (Classes,
      Hierarchy, or Methods), under Run Tests;
    • right-click a row in the Testing view;
    • the overflow on the Classes / Hierarchy / Methods pane headers;
    • GemStone: Clear Test Results in the Command Palette.

Reading a run in the Test Results pane

  • A run is named after what ranAiDemoBankAccountTest,
    AiDemoBankAccountTest>>testOverdrawIsRejected, 2 test classes — instead of VS
    Code's fallback Test run at 8/20/2026, 11:52:18 AM, which says nothing about which
    tests these were. With several entries in the history they all read alike otherwise.
    Names come from the items' own labels, so a class whose name exists in two
    dictionaries keeps its {Dictionary} qualifier there.
  • The class no longer reports into the run. It used to appear in the results list
    as a sibling of its own methods, marked "Some tests failed." and timed at 0.0ms —
    reading like a fourth test that had failed. Every method already reports, and VS Code
    rolls the tree row's state up from them. The class-level roll-up is still kept in our
    own store, which is what paints the Explorer rows.
  • A re-run blanks the previous outcome first. Re-running a failing test that fails
    again would otherwise repaint the mark it was already showing, leaving no sign the
    run happened at all.

The pane's own wording is VS Code's and not ours to change — "Test Results", "1 older
result", "The test run did not record any output" all come from the editor.

Stopping a run

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 finished.

Runs now go through the pollable path. 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 on its own rather than making you press again to discover
the test was in a tight loop. A stopped test is reported skipped, with no verdict left
behind — calling it an error would blame the test for the user's decision.

While a run is in flight the Explorer row's ▶ becomes a ■, and the Testing view's rows
gain one beside VS Code's own run icon (which an extension cannot replace).

Fixes found along the way

These are in nbRunner and gemstoneFileSystemProvider, and affect every cancellable
call in Jasper — Execute It and a Rowan load as much as a test run:

  • A hard break sent immediately after a soft one crashes the client process. Not an
    exception — a native fault in the GCI library. A second break arriving within 300ms
    of the first is now deferred to that mark. Found by an integration test that pressed
    both at once and took the vitest worker down with it.
  • A break left the session unusable for every later call. Abandoning a call without
    collecting its result leaves the session reporting one in progress, so the next call
    was refused outright — which reads as "I pressed stop and now nothing runs". The
    abandoned result is drained, and a call that follows a stop waits for that to finish.
    Both the hard-break and the poll-error path (which is how a soft break usually ends)
    were affected.
  • A logout mid-call left the poll running forever, with the progress notification
    claiming work was in flight and the caller never hearing back.
  • initialize/release broke the Methods pane. buildMethodUri asserted a category
    was slash-free, so building a row for a method in a stock GemStone category threw,
    surfacing as a toast and taking the pane's render down with it. Categories now ride
    in the path through the same sentinel selectors have always used.

Tests

63 tests across the branch, including 7 integration tests against a live stone. The
integration tests earned their place immediately: all three nbRunner bugs above were
found by them and by no unit test — the crash included. Nothing they do is committed;
the probe fixture is installed inside each test's own transaction, which the harness
aborts.

The larger "dedicated SUnit runner view" option in the issue is deliberately not built:
VS Code's Testing view covers it. Gaps we find in that view are worth their own issues.

Closes #427.

ericwinger and others added 13 commits August 19, 2026 13:57
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) <noreply@anthropic.com>
…path

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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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 <timestamp>", which says nothing about which tests these were.

Toward #427.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
`system-browser-and-explorer.md` recorded running SUnit tests as the one place
where "prefer the Explorer" cost a real feature. It doesn't any more, so the
section says what the Explorer offers instead of what it lacks.

README's SUnit section gains the entry points that arrived with it — the
Explorer's run actions and result icons, debugging a test, the gutter icons,
stopping a long run, clearing results, and the two reveal directions.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All three were found by a new integration test against a live stone; none were
visible against a mocked GCI, and one of them is a process crash.

- A hard break sent immediately after a soft one faults inside the GCI library
  and takes the whole client process down — not an exception, a crash. The
  escalation the SUnit stop button uses happens to wait 1.5s, which is the only
  reason it was never hit in the editor, but two quick presses of any Cancel
  would have done it. A second break arriving sooner than 300ms after the first
  is now deferred to that mark rather than sent.
- The drain of an abandoned call raced the next call on the session, which was
  refused outright ("session has a GciTsNb operation in progress"). A run
  started after a stop is now made to wait for the drain. GciTsCallInProgress is
  no help in deciding whether to wait: it answers "idle" while the abandoned Nb
  result is still uncollected, and it is that result the next call refuses over.
- The poll's error path abandoned a call without collecting its result at all.
  A soft break usually ends a call that way, so the drain added for hard breaks
  missed the common case entirely — stop a test, and every later run on that
  session failed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every function added for #427 now has a test. The ones that had none were the
quiet ones: the URI set that tells a Testing view click from any other open, the
context key behind the stop button, the reveal that navigates on purpose, the
tooltip that says an outcome predates the code, the drain of an abandoned call,
and both non-blocking run queries.

The integration tests are the point of this commit. Everything about the
non-blocking path — a run that can be interrupted, a session that has to be
usable straight afterwards — is invisible against a mocked GCI, and all three
bugs fixed in the previous commit were found by these and not by any unit test.
They also pin the parts of the run that only a real stone can answer for: that a
pass, a failure and an error come back distinguishable, that a class run reports
every test, and that a method in a slash-bearing category survives the round trip
through a gemstone:// URI.

Nothing they do is committed: the probe fixture is installed inside each test's
own transaction, which the harness aborts afterwards.

ExplorerController.revealDocument moves out of the registration closure so it
can be called directly; the handle just delegates to it now.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both surfaced only when every test file ran together, which is how the pre-push
hook runs them.

- Keying the drain by session id, not by the ActiveSession object. A WeakMap
  looked tidier, but callers build those objects freely and two of them stand
  for the same logged-in session — the thing that actually has one call
  outstanding at a time. Keyed by object, the next call missed the wait and was
  refused as "session has a GciTsNb operation in progress", which is the very
  bug the wait exists to prevent.
- The hard break is deferred now, so it rejects inside a timer tick — a whole
  turn before `expect(p).rejects` attaches a handler, which Node reports as an
  unhandled rejection and vitest fails the run over. The four cancellation tests
  claim their promise up front.

The integration file now holds one session object for the file, as the extension
does, rather than building a fresh one per call.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@MatiasFernandez MatiasFernandez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, left some non-blocking comments

Comment thread client/src/nbRunner.ts
Comment thread client/src/nbRunner.ts Outdated
Comment thread client/src/queries/debugTestMethod.ts Outdated
Comment thread client/src/gemstoneExplorer.ts Outdated
Comment thread client/src/gemstoneExplorer.ts Outdated
Comment thread client/src/sunitTestController.ts Outdated
Comment thread client/src/sunitTestController.ts Outdated
Comment thread client/src/codeExecutor.ts Outdated
Comment thread client/src/extension.ts
@ericwinger

Copy link
Copy Markdown
Member Author

Some slack discussion comments to investigate too

"I played around with the test running and I didn't find anything wrong. But I don't have experience with typical experience of running tests in gemstone. In terms of usability, the only thing I would revise is the fact that editing a test after a full class run changes other test's icon color to yellow. In IDEs for other languages yellow is almost a standard way to mention "skipped" tests, so it could be confusing to use that color and also I don't understand why we are doing that either"

_"actually, vscode use custom extension runners I think, so it may depend on the test runner. For example, in Jasper we use Vitest. Vitest in the cli shows like this:

• red is for test assertion failure OR unhandled error.
• yellow is for skipped.
• green is for PASS."

in the case I test using your branch, I just saved a new method and all other tests changed color. So even if your intention was to use yellow for "skipped", I think in that case, those tests were not really "skipped"

so apart from the color/icon decision, there's something weird happening there_

ericwinger and others added 2 commits August 21, 2026 15:20
…test

Two review follow-ups on this PR's SUnit-from-Explorer work:

- A class run that matched no results stored the class as passed, because
  passedCount === totalCount held at 0 === 0 — a pass check on a class whose
  methods all reported skipped. Store no class roll-up when totalCount is 0, so
  the class row reads the same as its skipped children.

- Cancelling a debug run came back as a raise (a soft break surfaces as the
  gem's "a soft break was received" error 6003, a hard break as
  NbCancelledError) and was reported as a test error, with a Break dialog
  naming the test. executeWithDebugger now reports a cancel distinctly and never
  opens a debugger on it; debugSingleTest leaves the test skipped with no
  verdict, and a cancelled class debug leaves no class verdict.

Unit tests added for both.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d of yellow

Two review follow-ups from the same thread:

- Recompiling a method marked every result in every dictionary stale, so saving
  one method greyed the whole tree. It now stales only the edited class (its
  roll-up and its other methods); results in other classes and dictionaries keep
  their outcomes. A recompiled class definition likewise no longer stales the
  rest of the store.

- Stale results used the queued yellow, which already means "skipped" in the
  Testing view and most IDEs. They now keep their pass/fail icon shape but are
  dimmed to a muted grey, with the tooltip noting the result predates the
  recompile.

Unit tests added/updated for both.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ericwinger

Copy link
Copy Markdown
Member Author

Fixed in 1bda47c — recompiling a method now marks only its own class stale (its roll-up and its other methods). Results in other classes and dictionaries keep their outcomes, so saving one method no longer changes unrelated tests. Covered by unit tests, and I verified it manually in the editor.

@ericwinger

Copy link
Copy Markdown
Member Author

Fixed in 1bda47c — stale results no longer use the queued yellow (which reads as "skipped" in most IDEs). They keep their pass/fail icon shape but are dimmed to a muted grey, and the tooltip notes the result predates the recompile. Covered by a unit test, and I verified it manually in the editor.

ericwinger and others added 2 commits August 21, 2026 15:52
A debug run built setUp as a bare statement before the ensure:, so a setUp that
raised skipped tearDown — the exact case a debug run exists for. Wrap setUp with
the test inside the ensure:, matching GemStone's own TestCase>>runCase. tearDown
is still only attempted (a tearDown that touches state a failed setUp never
initialised can raise in turn), same as the framework.

Test added.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ule, fix docs

- Attributed opens: gemstone.openDocument now clears its navigation claim when the
  open fires no editor-change (it threw, kept focus, or the document was already
  active). A lingering claim would otherwise make the next genuine Testing-view
  click on that method move the panes. New ExplorerController.clearAttributedOpen,
  wired through ExplorerHandle; regression test added.

- Test-selector rule: extracted the instance-side test* shape check into one
  helper (isTestSelectorShape) that both isTestSelector and decorateTestRow call,
  so the rule under test is the rule that runs.

- Removed the unused awaitSessionDrain export.

- Docs: reattached pollNbToCompletion's TSdoc to the function and documented the
  deferred second cancel; reworded markRunning to lead with the blank-frame reason
  (the queries no longer block); dropped a leftover comment paragraph on the
  explorer's sunit hook parameter.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ericwinger
ericwinger added this pull request to the merge queue Aug 21, 2026
Merged via the queue into main with commit 427cec7 Aug 21, 2026
15 checks passed
@ericwinger
ericwinger deleted the eric/issue427-sunit-run branch August 21, 2026 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No way to run SUnit tests from the GemStone Explorer

2 participants