tests: migrate KV semantics (e2e/ctl_v3_kv) to common test framework #20730
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lavishpal The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @lavishpal. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
cc: @serathius |
|
/ok-to-test |
tests/common/v3_kv_test.go
Outdated
| require.NoError(t, err) | ||
| } | ||
|
|
||
| // Get key1 |
There was a problem hiding this comment.
Please make it a table test.
tcs := []struct{
key string
options config.GetOptions
expectResponse clientv3.GetResponse
} {
{"key1", config.GetOptions{}, clientv3.GetResponse{KVs: []clientv3.KeyValues{Key: []byte("key1"}, Value: []byte("val1"), ModRevision: 2, Version: 1, CreateRevision: 2}}
}
tests/common/v3_kv_test.go
Outdated
| } | ||
|
|
||
| // Test Get with prefix, sort, limit, count-only, keys-only | ||
| func TestKV_GetVariants(t *testing.T) { |
There was a problem hiding this comment.
From getTest there are missing cases for order and sort-by
| testCtl(t, putTest, withCfg(*e2e.NewConfigClientTLS()), withFlagByEnv()) | ||
| } | ||
| func TestCtlV3PutIgnoreValue(t *testing.T) { testCtl(t, putTestIgnoreValue) } | ||
| func TestCtlV3PutIgnoreLease(t *testing.T) { testCtl(t, putTestIgnoreLease) } |
|
|
||
| func TestCtlV3GetTimeout(t *testing.T) { testCtl(t, getTest, withDefaultDialTimeout()) } | ||
|
|
||
| func TestCtlV3GetFormat(t *testing.T) { testCtl(t, getFormatTest) } |
|
|
||
| func TestCtlV3GetFormat(t *testing.T) { testCtl(t, getFormatTest) } | ||
| func TestCtlV3GetRev(t *testing.T) { testCtl(t, getRevTest) } | ||
| func TestCtlV3GetMinMaxCreateModRev(t *testing.T) { testCtl(t, getMinMaxCreateModRevTest) } |
|
|
||
| func TestCtlV3DelTimeout(t *testing.T) { testCtl(t, delTest, withDefaultDialTimeout()) } | ||
|
|
||
| func TestCtlV3GetRevokedCRL(t *testing.T) { |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 84 files with indirect coverage changes @@ Coverage Diff @@
## main #20730 +/- ##
==========================================
+ Coverage 63.27% 69.16% +5.89%
==========================================
Files 420 422 +2
Lines 34817 34817
==========================================
+ Hits 22031 24082 +2051
+ Misses 11400 9335 -2065
- Partials 1386 1400 +14 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
k8s-ci-robot
added
area/clientv3
area/documentation
area/etcdctl
area/etcdutl
area/tooling
size/XXL
and removed
size/XL
labels
Signed-off-by: Lavish Pal <[email protected]>
lavishpal
force-pushed
the
migrate-e2e-ctl-v3
branch
from
c490701 to
7d00897
Compare
|
/retest |
tests/common/v3_kv_test.go
Outdated
| name string | ||
| key string | ||
| options config.GetOptions | ||
| validateFunc func(t *testing.T, resp *clientv3.GetResponse) |
There was a problem hiding this comment.
Please test full response for comparison, not just random fields.
tests/common/v3_kv_test.go
Outdated
| } | ||
|
|
||
| // getTest - Comprehensive get operations with prefix, sorting, and limits | ||
| func getTest(ctx context.Context, t *testing.T, client intf.Client) { |
There was a problem hiding this comment.
I don't think this function is used anywhere
There was a problem hiding this comment.
Got it, I just create this function as a helper function.
There was a problem hiding this comment.
? I don't know what you mean.
tests/common/v3_kv_test.go
Outdated
| } | ||
|
|
||
| // getFormatTest - Test different ways of getting formatted data | ||
| func getFormatTest(ctx context.Context, t *testing.T, client intf.Client) { |
There was a problem hiding this comment.
I will remove this function also because I added this as a helper function which is not used anywhere.
tests/common/v3_kv_test.go
Outdated
| } | ||
|
|
||
| // getMinMaxCreateModRevTest - Test get operations with revision validation | ||
| func getMinMaxCreateModRevTest(ctx context.Context, t *testing.T, client intf.Client) { |
There was a problem hiding this comment.
I will remove this function also because I added this as a helper function which is not used anywhere.
|
This PR will not move forward far until you address all the comments. We cannot merge a migration PR that drops half of the test. Please consider splitting this PR and migrating one test at a time. |
I understand the concern about dropping tests. Just to confirm, do you suggest I create a separate PR for each test migration instead of combining them? |
Yes, smaller steps are easier and quicker to review. |
lavishpal
force-pushed
the
migrate-e2e-ctl-v3
branch
from
3ea5fe9 to
516ee78
Compare
Hi @serathius , I only migrate the |
Signed-off-by: Lavish Pal <[email protected]>
lavishpal
force-pushed
the
migrate-e2e-ctl-v3
branch
from
516ee78 to
89d6a0f
Compare
|
|
||
| // TestKV_PutTimeout migrates TestCtlV3PutTimeout from e2e to integration framework | ||
| // Original e2e test: func TestCtlV3PutTimeout(t *testing.T) { testCtl(t, putTest, withDefaultDialTimeout()) } | ||
| func TestKV_PutTimeout(t *testing.T) { |
There was a problem hiding this comment.
Please remove the matching e2e that its being migrated
|
Test failed: |
Signed-off-by: Lavish Pal <[email protected]>
Signed-off-by: Lavish Pal <[email protected]>
|
@lavishpal: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
| Order clientv3.SortOrder | ||
| SortBy clientv3.SortTarget | ||
| Timeout time.Duration | ||
| KeysOnly bool |
There was a problem hiding this comment.
It's not enough to just add a field to options, you need to implement it. Don't think you use KeysOnly in the current PR. You can just remove it for now.
| // Put operation - should work within timeout | ||
| err := client.Put(ctx, key, value, config.PutOptions{}) | ||
| // don't require.NoError here because timeout behavior may vary | ||
| // test validates that the client handles timeouts gracefully |
There was a problem hiding this comment.
At the current state of PR you don't set lower timeout, but use the default one. This means we should not expect an timeout. Please add validation of error.
There was a problem hiding this comment.
Instead of creating a new file, we can add tests in tests/common/kv_test.go.
|
TestCtlV3PutTimeout is for testing the etcdctl command with the Line 299 in 3531079 First, we need to support this option in the common framework. You can refer to the existing My idea is something like this: e2e// tests/framework/e2e/etcdctl.go
type EtcdctlV3 struct {
cfg ClientConfig
endpoints []string
authConfig clientv3.AuthConfig
dialTimeout time.Duration
}
...
func WithDialTimeout(dialTimeout time.Duration) config.ClientOption {
return func(c any) {
ctl := c.(*EtcdctlV3)
ctl.dialTimeout = dialTimeout
}
}// tests/common/e2e_test.go
func WithDialTimeout(dialTimeout time.Duration) config.ClientOption {
return e2e.WithDialTimeout(dialTimeout)
}integration// tests/framework/integration/cluster.go
func WithDialTimeout(dialTimeout time.Duration) framecfg.ClientOption {
return func(c any) {
cfg := c.(*clientv3.Config)
cfg.DialTimeout = dialTimeout
}
}// tests/common/integration_test.go
func WithDialTimeout(dialTimeout time.Duration) config.ClientOption {
return integration.WithDialTimeout(dialTimeout)
}Then, start implementing testutils.MustClient(clus.Client(WithDialTimeout(2 * time.Second)))Note: 2 sec is default (ref) |
Migrate KV tests to the common test framework and remove legacy e2e suite.
Test Migration Mapping
Changes Made
Files Modified:
tests/e2e/ctl_v3_kv_test.go→ DELETEDtests/common/v3_kv_test.go→ CREATED (4 test functions)tests/e2e/ctl_v3_test.go→ MODIFIED (added helper functions)tests/framework/config/client.go→ MODIFIED (addedKeysOnlyfield toGetOptions)Test Coverage:
Added Helper Functions:
ctlV3Put(cx ctlCtx, key, value, leaseID string, flags ...string) errorctlV3Get(cx ctlCtx, args []string, kvs ...kv) errorctlV3Del(cx ctlCtx, args []string, num int) errorkv struct{key, val string}typeFramework Benefits
Migration from etcdctl CLI to programmatic API:
config.PutOptions,config.GetOptions,config.DeleteOptionsImproved maintainability:
Other notes
context.WithTimeout(t.Context(), 15*time.Second)(15s timeouts) to reduce flakiness due to CI varianceKeysOnlyfield to framework config to support keys-only Get operationsRef
Fixes #20550