terrymanu commented on PR #39238:
URL: https://github.com/apache/shardingsphere/pull/39238#issuecomment-5076550258
### Summary
**Review Result: Mergeable**
**Reason:** No code-level blocker was found. The test boundaries are simpler
while critical protocol, session, database, and workflow behaviors remain
covered. This result applies only to code correctness; CI not reviewed by
request.
### Evidence
- `mcp/bootstrap/.../StreamableHttpMCPServerIT.java:74` starts a real HTTP
server and covers initialization, protocol headers, session isolation, deletion
lifecycle, Origin validation, and session attribution without Docker, a
database, or a model.
- The removed fine-grained protocol E2E tests are covered by this IT and
existing tests such as `StreamableHttpMCPServletTest`,
`ShardingSphereServerTransportSecurityValidatorTest`, and
`MCPSessionManagerTest`.
- Database queries, transaction cleanup, cross-database behavior, HTTP/STDIO
transports, Proxy workflows, and distribution runtime behavior remain covered
by the retained production runtime E2E tests. No critical behavior was left
without test ownership.
- `test/e2e/mcp/pom.xml:139` clearly separates deterministic runtime E2E
tests from the `llm-e2e` group. Command-line `-Dtest` overrides and profile
selection were verified successfully.
- `.github/workflows/ci-mcp.yml:64` explicitly runs the new HTTP IT.
`.github/workflows/e2e-mcp.yml:163` and `:319` use the corresponding Maven
profiles and remove duplicated workflow configuration.
- `.github/workflows/e2e-mcp.yml:204` retains the official conformance
scenarios and continues to verify that each scenario exists and produces valid
checks.
- The English and Chinese test documentation consistently describes the
responsibilities of IT, deterministic E2E, LLM E2E, and conformance testing.
- The other three cross-module changes only remove TODO comments and do not
affect executable behavior.
### Review Details
- **Review Focus:** Code Correctness Review — CI not reviewed by request.
- **Reviewed Scope:** All 34 files reported by GitHub; latest head
`4ae8fd4a064ce67af4438f01ceaf0e7c1eefe6f6`, base `master` at
`5486b2be95f7c9f77f77abe0036f2c77fbdb825e`, and local merge-base
`56b4594a3812351b7736a5a3b8ca82ddba0b446e`. The local triple-dot file list
exactly matched GitHub `/pulls/39238/files`.
- **Not Reviewed Scope:** GitHub Actions, check runs, and workflow logs were
not queried. Full scenarios requiring Docker, a real model, or the upstream
conformance tool were not executed locally.
- **Verification:**
- `./mvnw -pl mcp/bootstrap verify -DskipITs=false -Dspotless.skip=true -B
-ntp` — exit 0; all 300 unit tests and 8 integration tests passed.
- `./mvnw -pl test/e2e/mcp test -Pe2e.mcp
-Dtest=ProductionMCPClientTransportFactoryTest -DskipITs -Dspotless.skip=true
-B -ntp` — exit 0; both tests passed.
- `./mvnw -pl test/e2e/mcp test-compile -DskipTests -DskipITs
-Dspotless.skip=true -B -ntp` — exit 0.
- `./mvnw spotless:check -Pcheck -T1C -B -ntp` — exit 0.
- `./mvnw checkstyle:check -Pcheck -T1C -B -ntp` — exit 0 with no
violations.
- Syntax validation passed for the modified Maven POM and both GitHub
Actions YAML files.
- **Release Note / User Docs:** No release note is required because this is
an internal test and CI boundary refactoring. The English and Chinese MCP E2E
documentation has been updated and is consistent with the actual commands.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
