-
Notifications
You must be signed in to change notification settings - Fork 161
Refactor integration tests to use Testcontainers for TypeScript client and server #310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ontextprotocol#267) Ignore flaky `testMultipleClientParallel`
…t client and server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors integration tests to use Testcontainers for running TypeScript client and server components, replacing the previous approach of cloning the TypeScript SDK during test execution. The change aims to improve test reliability and performance by using a Docker image with the SDK pre-installed.
Key changes:
- Introduces Testcontainers library for managing Docker containers during tests
- Replaces direct process execution with containerized TypeScript clients and servers
- Adds a Dockerfile for building the TypeScript SDK container image
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/utils/Dockerfile | New Dockerfile for TypeScript SDK container with Node.js and pre-installed dependencies |
kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/TsTestBase.kt | Major refactor to use Docker containers instead of local processes for TypeScript execution |
kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/sse/TsEdgeCasesTestSse.kt | Updates test methods to use new containerized client execution |
kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/sse/TsClientKotlinServerTestSse.kt | Simplifies client execution to use new containerized approach |
kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/sse/KotlinServerForTsClientSse.kt | Removes unused import |
kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/sse/KotlinClientTsServerTestSse.kt | Updates to use ContainerProcess instead of Process |
kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/sse/KotlinClientTsServerEdgeCasesTestSse.kt | Updates to use ContainerProcess instead of Process |
kotlin-sdk-test/build.gradle.kts | Adds Testcontainers dependency |
gradle/libs.versions.toml | Adds Testcontainers version configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...t/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/TsTestBase.kt
Show resolved
Hide resolved
...t/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/TsTestBase.kt
Outdated
Show resolved
Hide resolved
...t/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/TsTestBase.kt
Outdated
Show resolved
Hide resolved
BTW, With test containers we may build image before run, without pushing it to docker registry. |
Motivation and Context
Current approach relies on cloning the TypeScript SDK before all tests, which may increase flakiness and slows down the whole test suite. The changes in this PR introduce testcontainers library usage to have a docker image with the SDK already cloned. The tests use the image for both server and client interactions via port binding and directories mounting.
Dockerfile is used to build the image which is then uploaded to our internal JB Docker registry. This means that we no longer run the tests on the latest master of TypeScript SDK. It can help reduce flakiness, but we may not immediately see an issue if something changes in it. We may consider building and uploading a new image automatically on CI.
The tests are expected to fail on CI for now as
TS_SDK_IMAGE
needs to be added to the repository secrects.How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context