fix(test): improve timing stability in tests (#506)

This commit is contained in:
Konstantin Pavlov 2026-02-06 12:25:18 +02:00 • committed by GitHub
commit 7eb2a93c46
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 66 additions and 35 deletions

View file

@ -1,14 +1,12 @@
package io.modelcontextprotocol.kotlin.sdk.shared package io.modelcontextprotocol.kotlin.sdk.shared
import io.kotest.assertions.nondeterministic.eventually
import io.kotest.matchers.shouldBe
import io.modelcontextprotocol.kotlin.sdk.types.InitializedNotification import io.modelcontextprotocol.kotlin.sdk.types.InitializedNotification
import io.modelcontextprotocol.kotlin.sdk.types.JSONRPCMessage import io.modelcontextprotocol.kotlin.sdk.types.JSONRPCMessage
import io.modelcontextprotocol.kotlin.sdk.types.PingRequest import io.modelcontextprotocol.kotlin.sdk.types.PingRequest
import io.modelcontextprotocol.kotlin.sdk.types.toJSON import io.modelcontextprotocol.kotlin.sdk.types.toJSON
import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.delay
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertTrue
import kotlin.test.fail import kotlin.test.fail
import kotlin.time.Duration.Companion.seconds import kotlin.time.Duration.Companion.seconds
@ -23,12 +21,18 @@ abstract class BaseTransportTest {
transport.onClose { didClose = true } transport.onClose { didClose = true }
transport.start() transport.start()
delay(1.seconds)
assertFalse(didClose, "Transport should not be closed immediately after start") // Verify transport stays open after start (use eventually for cross-platform reliability)
eventually(2.seconds) {
didClose shouldBe false
}
transport.close() transport.close()
assertTrue(didClose, "Transport should be closed after close() call")
// Verify transport is closed after close() call
eventually(2.seconds) {
didClose shouldBe true
}
} }
protected suspend fun testTransportRead(transport: Transport) { protected suspend fun testTransportRead(transport: Transport) {
@ -60,7 +64,7 @@ abstract class BaseTransportTest {
finished.await() finished.await()
assertEquals(messages, readMessages, "Assert messages received") messages shouldBe readMessages
transport.close() transport.close()
} }

View file

@ -1,12 +1,12 @@
package io.modelcontextprotocol.kotlin.sdk.client package io.modelcontextprotocol.kotlin.sdk.client
import io.kotest.assertions.nondeterministic.eventually
import io.modelcontextprotocol.kotlin.sdk.shared.BaseTransportTest import io.modelcontextprotocol.kotlin.sdk.shared.BaseTransportTest
import io.modelcontextprotocol.kotlin.sdk.types.Implementation import io.modelcontextprotocol.kotlin.sdk.types.Implementation
import io.modelcontextprotocol.kotlin.sdk.types.McpException import io.modelcontextprotocol.kotlin.sdk.types.McpException
import io.modelcontextprotocol.kotlin.test.utils.createSleepyProcessBuilder import io.modelcontextprotocol.kotlin.test.utils.createSleepyProcessBuilder
import io.modelcontextprotocol.kotlin.test.utils.createTeeProcessBuilder import io.modelcontextprotocol.kotlin.test.utils.createTeeProcessBuilder
import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.delay
import kotlinx.coroutines.runBlocking import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.test.runTest import kotlinx.coroutines.test.runTest
import kotlinx.io.asSink import kotlinx.io.asSink
@ -23,7 +23,6 @@ import kotlin.concurrent.atomics.ExperimentalAtomicApi
import kotlin.test.assertFalse import kotlin.test.assertFalse
import kotlin.test.assertTrue import kotlin.test.assertTrue
import kotlin.test.fail import kotlin.test.fail
import kotlin.time.Duration.Companion.milliseconds
import kotlin.time.Duration.Companion.seconds import kotlin.time.Duration.Companion.seconds
@Timeout(30, unit = TimeUnit.SECONDS) @Timeout(30, unit = TimeUnit.SECONDS)
@ -89,17 +88,21 @@ class StdioClientTransportTest : BaseTransportTest() {
transport.onClose { didClose.store(true) } transport.onClose { didClose.store(true) }
transport.start() transport.start()
delay(1.seconds)
assertFalse(didClose.load(), "Transport should not be closed immediately after start") // Verify transport stays open after start (use eventually for cross-platform reliability)
eventually(2.seconds) {
assertFalse(didClose.load(), "Transport should not be closed immediately after start")
}
// Destroy process BEFORE close() to unblock stdin reader // Destroy process BEFORE close() to unblock stdin reader
process.destroyForcibly() process.destroyForcibly()
delay(100.milliseconds) // Give time for EOF to propagate
transport.close() transport.close()
assertTrue(didClose.load(), "Transport should be closed after close() call") // Verify transport is closed after close() call
eventually(2.seconds) {
assertTrue(didClose.load(), "Transport should be closed after close() call")
}
} }
@Test @Test

View file

@ -1,6 +1,10 @@
package io.modelcontextprotocol.kotlin.sdk.server package io.modelcontextprotocol.kotlin.sdk.server
import io.kotest.assertions.nondeterministic.eventually
import io.kotest.assertions.withClue
import io.kotest.matchers.collections.shouldBeEmpty
import io.kotest.matchers.collections.shouldHaveSize import io.kotest.matchers.collections.shouldHaveSize
import io.kotest.matchers.shouldBe
import io.modelcontextprotocol.kotlin.sdk.types.Method.Defined.NotificationsResourcesUpdated import io.modelcontextprotocol.kotlin.sdk.types.Method.Defined.NotificationsResourcesUpdated
import io.modelcontextprotocol.kotlin.sdk.types.ReadResourceResult import io.modelcontextprotocol.kotlin.sdk.types.ReadResourceResult
import io.modelcontextprotocol.kotlin.sdk.types.ResourceUpdatedNotification import io.modelcontextprotocol.kotlin.sdk.types.ResourceUpdatedNotification
@ -9,13 +13,11 @@ import io.modelcontextprotocol.kotlin.sdk.types.SubscribeRequest
import io.modelcontextprotocol.kotlin.sdk.types.SubscribeRequestParams import io.modelcontextprotocol.kotlin.sdk.types.SubscribeRequestParams
import io.modelcontextprotocol.kotlin.sdk.types.TextResourceContents import io.modelcontextprotocol.kotlin.sdk.types.TextResourceContents
import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.delay
import kotlinx.coroutines.runBlocking import kotlinx.coroutines.runBlocking
import org.awaitility.kotlin.await import org.awaitility.kotlin.await
import org.awaitility.kotlin.untilAsserted import org.awaitility.kotlin.untilAsserted
import org.junit.jupiter.api.Test import org.junit.jupiter.api.Test
import java.util.concurrent.ConcurrentLinkedQueue import java.util.concurrent.ConcurrentLinkedQueue
import kotlin.test.assertFalse
import kotlin.test.assertTrue import kotlin.test.assertTrue
import kotlin.time.Duration.Companion.seconds import kotlin.time.Duration.Companion.seconds
import kotlin.uuid.ExperimentalUuidApi import kotlin.uuid.ExperimentalUuidApi
@ -115,7 +117,7 @@ class ServerResourcesNotificationSubscribeTest : AbstractServerFeaturesTest() {
} }
@Test @Test
fun `notification should not be send when removed resource does not exists`() = runBlocking { fun `notification should not be send when removed resource does not exists`(): Unit = runBlocking {
println("Thread ${Thread.currentThread().name} test 1") println("Thread ${Thread.currentThread().name} test 1")
// Track notifications // Track notifications
val notifications = ConcurrentLinkedQueue<ResourceUpdatedNotification>() val notifications = ConcurrentLinkedQueue<ResourceUpdatedNotification>()
@ -126,13 +128,15 @@ class ServerResourcesNotificationSubscribeTest : AbstractServerFeaturesTest() {
// Try to remove a non-existent resource // Try to remove a non-existent resource
val result = server.removeResource("non-existent-resource") val result = server.removeResource("non-existent-resource")
assertFalse(result, "Removing non-existent resource should return false") withClue("Removing non-existent resource should return false") {
result shouldBe false
}
// Verify the result // Verify no notification is sent (use eventually for cross-platform reliability)
delay(1.seconds) withClue("No notification should be sent when resource doesn't exist") {
assertTrue( eventually(2.seconds) {
notifications.isEmpty(), notifications.shouldBeEmpty()
"No notification should be sent when resource doesn't exist", }
) }
} }
} }

View file

@ -1,5 +1,6 @@
package io.modelcontextprotocol.kotlin.sdk.client package io.modelcontextprotocol.kotlin.sdk.client
import io.kotest.assertions.nondeterministic.eventually
import io.kotest.assertions.throwables.shouldThrow import io.kotest.assertions.throwables.shouldThrow
import io.kotest.matchers.shouldBe import io.kotest.matchers.shouldBe
import io.kotest.matchers.string.shouldContain import io.kotest.matchers.string.shouldContain
@ -13,6 +14,7 @@ import kotlinx.coroutines.test.runTest
import kotlin.test.BeforeTest import kotlin.test.BeforeTest
import kotlin.test.Test import kotlin.test.Test
import kotlin.time.Duration.Companion.milliseconds import kotlin.time.Duration.Companion.milliseconds
import kotlin.time.Duration.Companion.seconds
abstract class AbstractClientTransportLifecycleTest<T : AbstractTransport> { abstract class AbstractClientTransportLifecycleTest<T : AbstractTransport> {
@ -60,11 +62,15 @@ abstract class AbstractClientTransportLifecycleTest<T : AbstractTransport> {
val transport = createTransport() val transport = createTransport()
transport.start() transport.start()
// Give transport time to initialize
delay(50.milliseconds) delay(50.milliseconds)
transport.close() transport.close()
shouldThrow<Exception> { // Verify that send throws after close using eventually for reliability
transport.send(PingRequest().toJSON()) eventually(2.seconds) {
shouldThrow<Exception> {
transport.send(PingRequest().toJSON())
}
} }
} }
@ -76,13 +82,17 @@ abstract class AbstractClientTransportLifecycleTest<T : AbstractTransport> {
transport.onClose { closeCallCount++ } transport.onClose { closeCallCount++ }
transport.start() transport.start()
// Give transport time to initialize
delay(50.milliseconds) delay(50.milliseconds)
// Multiple close attempts // Multiple close attempts
transport.close() transport.close()
transport.close() transport.close()
closeCallCount shouldBe 1 // Verify onClose was called exactly once using eventually for cross-platform reliability
eventually(2.seconds) {
closeCallCount shouldBe 1
}
} }
protected abstract fun createTransport(): T protected abstract fun createTransport(): T

View file

@ -1,5 +1,6 @@
package io.modelcontextprotocol.kotlin.sdk.client.stdio package io.modelcontextprotocol.kotlin.sdk.client.stdio
import io.kotest.assertions.nondeterministic.eventually
import io.kotest.assertions.throwables.shouldThrow import io.kotest.assertions.throwables.shouldThrow
import io.kotest.matchers.booleans.shouldBeFalse import io.kotest.matchers.booleans.shouldBeFalse
import io.kotest.matchers.shouldBe import io.kotest.matchers.shouldBe
@ -27,7 +28,7 @@ import java.util.stream.Stream
import kotlin.concurrent.atomics.AtomicBoolean import kotlin.concurrent.atomics.AtomicBoolean
import kotlin.concurrent.atomics.ExperimentalAtomicApi import kotlin.concurrent.atomics.ExperimentalAtomicApi
import kotlin.test.Test import kotlin.test.Test
import kotlin.time.Duration.Companion.milliseconds import kotlin.time.Duration.Companion.seconds
/** /**
* Tests for StdioClientTransport error handling: EOF, IO errors, and edge cases. * Tests for StdioClientTransport error handling: EOF, IO errors, and edge cases.
@ -57,10 +58,13 @@ class StdioClientTransportErrorHandlingTest {
transport.onClose { closeCalled.store(true) } transport.onClose { closeCalled.store(true) }
transport.start() transport.start()
delay(200.milliseconds)
// Stderr EOF should not close transport // Stderr EOF should not close transport
closeCalled.load() shouldBe false // Use eventually to handle timing differences across platforms (especially Windows)
eventually(2.seconds) {
// Wait for stderr to be processed, then verify transport is still open
closeCalled.load() shouldBe false
}
transport.close() transport.close()
closeCalled.load() shouldBe true closeCalled.load() shouldBe true
@ -87,9 +91,13 @@ class StdioClientTransportErrorHandlingTest {
transport.onClose { closeCallCount++ } transport.onClose { closeCallCount++ }
transport.start() transport.start()
delay(100.milliseconds)
// Explicit close after error already closed it // FATAL stderr should trigger close, wait for it to complete
eventually(2.seconds) {
closeCallCount shouldBe 1
}
// Explicit close after error already closed it (should be no-op)
transport.close() transport.close()
closeCallCount shouldBe 1 closeCallCount shouldBe 1
@ -109,10 +117,12 @@ class StdioClientTransportErrorHandlingTest {
transport.onError { errorCalled = true } transport.onError { errorCalled = true }
transport.start() transport.start()
delay(100.milliseconds)
// Empty input should close cleanly without error // Empty input should close cleanly without error
errorCalled.shouldBeFalse() // Wait for EOF processing to complete, verify no error was called
eventually(2.seconds) {
errorCalled.shouldBeFalse()
}
} }
companion object { companion object {