tests: add external starter mode realtikv harness#68463
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note-none
|
@ChangRui-Ryan I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/XL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a README subsection, a bash test runner that starts an external starter-mode tidb-server with dynamic ports and readiness polling, a Bazel go_test target, and Go integration tests that validate starter /config, sysvar contracts, protocol max-packet enforcement, and session-state round-trip. ChangesStarter Mode Test Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Runner as run-starter-tests-with-server.sh
participant PortFinder as find_available_port
participant TiDB as tidb-server
participant Status as wait_for_tidb_server
participant GoTest as go test
Runner->>PortFinder: request free SQL & status ports
PortFinder-->>Runner: available ports
Runner->>Runner: write temporary starter.toml (ports, max-allowed-packet, tikv-worker-url)
Runner->>TiDB: start tidb-server (background)
TiDB-->>Runner: background PID
Runner->>Status: poll ${TIDB_STATUS_URL}/config until ready
Status->>TiDB: HTTP GET /config
TiDB-->>Status: JSON config response
Status-->>Runner: server ready
Runner->>GoTest: export env vars and run go test with NEXT_GEN=1, tags
GoTest-->>Runner: test results
Runner->>TiDB: cleanup trap kills tidb-server on exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @ChangRui-Ryan. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #68463 +/- ##
================================================
- Coverage 77.2761% 76.5112% -0.7650%
================================================
Files 2010 1992 -18
Lines 555474 557927 +2453
================================================
- Hits 429249 426877 -2372
- Misses 125305 130933 +5628
+ Partials 920 117 -803
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/realtikvtest/scripts/next-gen/README.md`:
- Around line 80-83: The README uses relative paths assuming current directory
tests/realtikvtest/scripts/next-gen/ (e.g.,
"./run-starter-tests-with-server.sh"); update the commands to be copy-pasteable
from the repository root by prefixing with the repo-root path (e.g.,
"tests/realtikvtest/scripts/next-gen/run-starter-tests-with-server.sh") or
explicitly state "run from tests/realtikvtest/scripts/next-gen/" for each
affected command (notably the occurrences of
"./run-starter-tests-with-server.sh" around the shown block and the commands at
lines ~92-94) so users can run them directly from the repo root.
In `@tests/realtikvtest/scripts/next-gen/run-starter-tests-with-server.sh`:
- Around line 79-83: The EXIT cleanup currently only stops background processes
but doesn't remove the temporary data directory created in data_dir; update the
EXIT/trap cleanup block (the handler that kills tidb-server and other test
processes) to also remove the temp directory by adding a safe removal like: if
[[ -n "${data_dir:-}" && -d "${data_dir}" ]]; then rm -rf -- "${data_dir}"; fi
so the tmp artifacts created by data_dir are always cleaned up.
In `@tests/realtikvtest/startertest/starter_external_test.go`:
- Line 74: The test currently hard-codes the TiKV worker URL to
"localhost:19000" (asserting cfg.TiKVWorkerURL) which breaks when the harness
assigns a free port; instead assert the stable shape: parse cfg.TiKVWorkerURL
(or use a regex) and verify the host equals "localhost" and the port is a
non-empty numeric value (e.g., use net.SplitHostPort(cfg.TiKVWorkerURL) and
require.Equal(t, "localhost", host) plus require.Regexp(t, `^\d+$`, port)), or
compare against a harness-provided expected value if one is available.
- Around line 115-119: The test's error substring checks are case-sensitive (see
the require.Truef call that inspects err.Error()), causing flakes; change the
assertion to compare against a lowercase error string by calling
strings.ToLower(err.Error()) and match the lowercase literals
"max_allowed_packet", "packet bigger" and "invalid connection" so the
require.Truef(...) still checks the same three conditions but
case-insensitively; update the require.Truef invocation that references
err.Error() accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c64e7654-5b30-45b1-ad5d-07e3b6a4b435
📒 Files selected for processing (4)
tests/realtikvtest/scripts/next-gen/README.mdtests/realtikvtest/scripts/next-gen/run-starter-tests-with-server.shtests/realtikvtest/startertest/BUILD.bazeltests/realtikvtest/startertest/starter_external_test.go
|
/retest |
|
@ChangRui-Ryan: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
1 participant
What problem does this PR solve?
Issue Number: ref #67765
Problem Summary
The existing next-gen RealTiKV scripts only run TiDB in-process through Go tests. They do not provide a way to validate starter mode through a real external
tidb-serverprocess, so startup config handling, MySQL protocol behavior, and status/config endpoint behavior for starter mode were not covered.What changed and how does it work
Added an external starter-mode test harness under
tests/realtikvtest/scripts/next-gen. The new script reuses the existing next-gen cluster bootstrap for PD/TiKV/TiKV-worker/MinIO, then starts a real next-gentidb-serverwithdeploy-mode = "starter"and runstests/realtikvtest/startertestthrough the MySQL protocol.The new starter tests verify the external server’s
/configoutput, starter-specific sysvar contracts,max_allowed_packetenforcement at the protocol boundary, basic SQL behavior, andSHOW/SET SESSION_STATESroundtrip behavior.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Tests
Documentation