-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
testkit: add a WithTiKV flag to support run unit test on real TiKV #35647
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
testkit/mockstore.go
Outdated
"github.com/pingcap/tidb/store/mockstore" | ||
"github.com/stretchr/testify/require" | ||
"github.com/tikv/client-go/v2/oracle" | ||
"github.com/tikv/client-go/v2/tikv" | ||
) | ||
|
||
// WithTiKV flag makes the test case run with real TiKV | ||
var WithTiKV = flag.String("with-tikv", "", "whether tests run with real TiKV") |
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.
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.
The background is, I need to test the paging protocol with the real tikv.
So I need rewrite all the test case or move them all?
A more reasonable solution is just add a flag ... if you don't use it, it doesn't change anything.
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.
This patch just add a flag, which won't affect CI settings. The reason I move all with real tikv tests to a dedicated test is for tagging tests in CI env.
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.
@tiancaiamao would you like to change CI logic? Or adding these code for conveniently running tests with real tikv locally?
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.
Also, with-tikv
is a string flag while its help info said "whether tests run with real TiKV"
, which is confusing...
Perhaps
// This flag is only used for debugging locally with real tikv cluster.
var tikvAddr = flag.String("tikv-addr", "", "address of tikv cluster, if set, running test with real tikv cluster")
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.
Also,
with-tikv
is a string flag while its help info said"whether tests run with real TiKV"
, which is confusing...Perhaps
// This flag is only used for debugging locally with real tikv cluster. var tikvAddr = flag.String("tikv-addr", "", "address of tikv cluster, if set, running test with real tikv cluster")
Updated.
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.
@tiancaiamao would you like to change CI logic? Or adding these code for conveniently running tests with real tikv locally?
If you want to see the full changes, it's in my branch https://github.com/tiancaiamao/tidb/tree/test-with-tikv
But not all the test cases can run under TiKV, I check and document the result here:
https://pingcap.feishu.cn/docx/doxcnEzyRXsTdCJ2EE6EZwIP2oe
I'm not adding a new CI pipeline, I just make the code base more friendly for those who want to run some test cases with a real TiKV.
/run-all-tests |
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.
LGTM
If the |
@tiancaiamao Please |
Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/d03ba526a464a86146ba548ed1ebf6714b8cb070 |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: d03ba52
|
/run-check_dev_2 |
TiDB MergeCI notify✅ Well Done! New fixed [1] after this pr merged.
|
What problem does this PR solve?
Issue Number: close #35646
Problem Summary:
Add a
-with-tikv
flag to the unit test.What is changed and how it works?
After this change, I can run any test case with a real TiKV, for example:
I can verify out test cases with real tikv. Here is how I do that:
--net
use the local port,--rm
means remove the container after the test is done.For example:
I modify the
make ut
script to automate the tests.Create the tikv docker container before test and cleanup them.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.