test: run the nine @Test methods JUnit was skipping, and fail on any it skips

[arjun28115]

Nine @Test methods in composeApp have never run. One of them no longer describes the code. The build stays green through all of it, and this PR makes that impossible to repeat.

Why they don't run

@Test
fun `requestApproval times out and fails closed if operator does not respond`() =
    runBlocking {
        ...
        assertIs<McpApprovalDecision.Timeout>(decision)   // returns the Timeout
    }

An expression body takes the type of its last expression, and runBlocking<T> returns whatever its block does. So a test that ends in assertIs, assertNotNull, assertFailsWith or a call returning a value compiles to a method that returns a value. JUnit Jupiter does not execute a @Test method that returns a value. It reports a WARNING-level discovery issue, which Gradle doesn't show, and it doesn't count the method as skipped. Nothing fails. The method just isn't there.

The nine, found by reading the return type of every @Test method in the compiled test classes, then confirmed missing from a run's results:

Test class	Method	Returned
McpApprovalGateTest	requestApproval times out and fails closed if operator does not respond	McpApprovalDecision.Timeout
McpApprovalGateTest	exceeding pending buffer capacity immediately returns Denied buffer full	McpApprovalDecision
McpApprovalGateTest	deny all rejects exactly the visible snapshot without persisting policy	McpApprovalDecision.Approved
McpApprovalGateTest	an approval request carries the tool description, policy, and remaining timeout to the dialog	McpApprovalDecision
PluginUIServiceBridgeTest	a plugin that dies right after binding leaves no stuck claim behind	SurfaceRegistration.Accepted
PluginUIServiceBridgeTest	unregistering an unknown surface is unaffected by having no credential (renamed, see below)	Empty
UpdateJarIdentityVetTest	a jar that is not there is rejected	Throwable
RepairAdviceTest	caller cancellation is not converted to missing advice	CancellationException
WorkspaceMcpToolProviderTest	open_workspace with createIfAbsent says the file was saved when the window never registers	LayoutWorkspace

Four are the MCP approval gate's own guarantees: fail closed on timeout, and deny when the buffer is full.

What running them found

Eight pass once they run. One doesn't:

unregistering an unknown surface is unaffected by having no credential asserts that a caller with no credential can unregister a surface nobody owns. That was true when it was written (#430, Sep 9). Since #776 (Sep 16), ProcessIdentityInterceptor authenticates every RPC, so the call is refused UNAUTHENTICATED before the bridge sees it. The tightening was deliberate and the interceptor's KDoc says so. The test just never ran to notice. It is renamed an unknown surface unregisters as a no-op, but only for an authenticated caller and pins the current contract: an authenticated caller still gets the bridge's idempotent no-op, and an anonymous one is refused UNAUTHENTICATED at the transport.

The change

• Each of the nine is declared (): Unit =. With an expected type of Unit, the block's last expression is coerced and the method returns void. Nothing else in eight of them changes.

• The stale test is rewritten as above, with a KDoc saying why.

• composeApp's Test tasks set junit.platform.discovery.issue.severity.critical=WARNING, so a discovery issue fails the run instead of passing silently. Checked: with one of the nine reverted to () =, the run fails with

  DiscoveryIssueException: TestEngine with ID 'junit-jupiter' encountered a critical issue during test discovery:
  (1) [WARNING] @Test method '... RepairAdviceTest.caller cancellation is not converted to missing advice()' must not return a value. It will not be executed.
  

Verification

• Full :composeApp:desktopTest with the guard on: 6,529 tests, 0 failed, 5 skipped. The nine all run and pass. The guard finds no other discovery issue anywhere in composeApp.

• ktlintCheck and detekt: clean.

• What running them buys, measured by breaking the approval gate (McpApprovalGateTest, as on dev and with this PR):

  Mutation in McpApprovalBus	As on dev (3 tests run)	With this PR (7 run)
  a full buffer returns Approved() instead of QueueFull, so the tool runs without approval	not caught, build green	caught by exceeding pending buffer capacity immediately returns Denied buffer full
  a timeout returns Approved() instead of Timeout	caught by a neighbouring test	caught by that test and requestApproval times out and fails closed

  In fairness to the existing suite: the second row was already covered, and the cap has two layers (the pending-list check and the request channel's capacity), so an off-by-one in either alone is masked by the other. The first row is the one that shipped unguarded.

• The other modules: plugin-platform's compiled test classes (icons, repository, logging, loader) have no value-returning @Test. The guard is set in composeApp's tasks.withType<Test> only, which is where all nine were. Widening it to every module is a one-line follow-up if wanted.

🤖 Generated with Claude Code

[fluck-boss]

@claude review

[claude]

Claude finished @fluck-boss's task in 6m 8s —— View job

---

Review: PR #1667

• Read the diff against origin/dev
• Verify the nine signature changes
• Check the Gradle discovery-issue guard
• Check the rewritten PluginUIServiceBridgeTest case
• Look for other value-returning @Test methods the guard would catch
• Post review

---

Verdict: correct, well-diagnosed, and worth landing. The mechanism is described accurately, the (): Unit = fix is the right one (the expected type propagates into runBlocking<T> / withTempDir<T>, T infers Unit, and the lambda's last expression is coerced), the one stale test is genuinely stale, and the guard is the part that stops this recurring. Nothing here is a blocker. Below are three things worth considering and two nits.

What I verified

• All nine compile-shape changes are sound. Eight wrap runBlocking; UpdateJarIdentityVetTest:115 wraps withTempDir, whose signature is internal inline fun <T> withTempDir(block: (File) -> T): T (PluginJarTestFixtures.kt:53) — generic, so T = Unit infers. Longest new line is ~113 chars against max_line_length = 140.
• The rewritten test matches the file's own idiom. PluginUIServiceBridgeTest:622-628 is the same assertFailsWith<StatusException> / Status.Code.UNAUTHENTICATED shape as the neighbouring RegisterUI with no credential at all is refused (:541-545), and pluginAs/anonymousPlugin (:131, :144) do what the new assertions need.
• No global-state leak from the newly-running tests. WorkspaceMcpToolProviderTest sets WorkspaceMcpToolProvider.windowCreator (a process-global) at :316, and @AfterTest at :78-81 nulls it — so the test that had never run before does not now leak into its neighbours.
• I looked for other offenders outside composeApp (the guard's blind spot). Grepping every module for an expression-bodied @Test ending in assertIs / assertNotNull / assertFailsWith, the hits in modules/* and plugin-platform/* are all either block-bodied, runTest (JVM TestResult is Unit, which the 6,529-test run implicitly confirms), or — in plugin-loader — the runBlocking<Unit> idiom (LoadTimeSignatureVerificationTest:97, 110, 122, …, MinApiVersionGateTest). So nothing is currently broken there, but note that this trap has already been met and solved a second way in this repo, which is an argument for the guard rather than against it.

1. The guard's scope is narrower than the failure mode (worth widening now)

The property is set in composeApp/build.gradle.kts:2500 only. My grep above is a point-in-time check, and it cannot see the shape that actually bit you hardest — a last expression that is an ordinary value-returning call (SurfaceRegistration.Accepted, LayoutWorkspace), which no regex finds. modules/*, server/ and buildSrc are unguarded.

There is a root allprojects { } at build.gradle.kts:42 that already carries the cross-module conventions (detekt, ktlint). One line there covers everything:

tasks.withType<Test>().configureEach {
    systemProperty("junit.platform.discovery.issue.severity.critical", "WARNING")
}

The PR body offers this as a follow-up; given the property is inert where there is nothing to report, I'd take it in this PR rather than leave the guarantee module-shaped. Fix this →

2. A Gradle system property does not reach the IDE

systemProperty on the Test task only applies when tests run through Gradle. A developer running a test class from IntelliJ with Run tests using: IntelliJ IDEA gets no guard, so the feedback arrives at CI rather than at the keystroke that caused it — for a defect whose whole character is "you don't notice".

A junit-platform.properties on the test runtime classpath (composeApp/src/desktopTest/resources/junit-platform.properties) is read by the platform however it is launched, and is also per-module without editing a build file each time. There is none in the repo today, so no conflict. Either mechanism alone works; the resource is strictly the wider net, and a system property still overrides it if someone needs to relax it for one run.

3. Please say in the comment what a failure looks like

This is the part I'd most like added, because the blast radius is not obvious: a DiscoveryIssueException aborts discovery for the whole engine. One warning anywhere in composeApp means zero tests run, not one test failing. And useJUnitPlatform() here registers vintage as well (junit-vintage-engine, for the JUnit 4 rules createComposeRule needs), so a future Compose or JUnit bump that introduces a new WARNING-level discovery issue takes the entire suite out at once, with an error message that does not look like a test failure.

That is the right trade — silently skipping is worse — but the next person to meet it will be staring at 6,500 vanished tests. One more sentence in the comment at composeApp/build.gradle.kts:2496-2500 naming the property and saying "relax this to ERROR if a toolchain bump introduces an unrelated warning" turns a confusing outage into a two-minute fix.

Relatedly, this repo records invariants of exactly this kind in AGENTS.md (there is already a composeApp test-isolation subsection under Workflow Rules). "A @Test that returns a value is not executed; declare it (): Unit =" belongs there too — a build-file comment is not where someone writing a test looks.

Nits

• McpApprovalGateTest:145 uses delay(50) followed by assertEquals(2, bus.pendingList.value.size), where its sibling at :166 uses the deterministic bus.pendingList.first { it.size == 2 }. In practice runBlocking's single-threaded event loop makes the delay near-deterministic, so this is not a live flake — but this test has never run on CI before, and a fixed sleep as a scheduling barrier is the kind of thing that only starts failing under load. Since the file already has the right form ten lines away, I'd switch it while you're in here.
• PluginUIServiceBridgeTest:625 — the "no-op" half asserts only that the call did not throw. Adding an assertNull(registry.surfaceOf("never-registered")) after it (or asserting a real registration is untouched) would make no-op an observed fact rather than the absence of an exception, which is the standard the KDoc you wrote sets for the other half.

None of these change my read: this is a good catch, the diagnosis is right, and the guard is the durable half.
• branch fix/tests-that-never-run

[fluck-boss]

Merging. Gate complete: CI green on the head, Claude review has nothing blocking, my own read clean.