-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
migrate auth tests to common #7 #15578
Conversation
Signed-off-by: Chao Chen <chaochn@amazon.com>
88d7cd1
to
9a2553f
Compare
err = rootAuthClient.Put(ctx, "key", "value", config.PutOptions{LeaseID: lresp.ID}) | ||
require.NoError(t, err) | ||
|
||
_, err = rootAuthClient.Revoke(ctx, lresp.ID) |
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.
If we have a test targeting this method specifically, it would be good to assert what's returning actually.
[I know that it was not tested previously... ]
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.
clientv3 Lease revoke response is the type alias of the proto buf generated one. It only contains ResponseHeader
.
type LeaseRevokeResponse struct {
Header *ResponseHeader `protobuf:"bytes,1,opt,name=header,proto3" json:"header,omitempty"`
XXX_NoUnkeyedLiteral struct{} `json:"-"`
XXX_unrecognized []byte `json:"-"`
XXX_sizecache int32 `json:"-"`
}
I was blindly migrate the old logic to this one. How about assert the revoked lease is no longer in LeaseList response and get response is empty?
cc := testutils.MustClient(clus.Client()) | ||
testutils.ExecuteUntil(ctx, t, func() { | ||
require.NoErrorf(t, setupAuth(cc, []authRole{testRole}, []authUser{rootUser, testUser}), "failed to enable auth") | ||
rootAuthClient := testutils.MustClient(clus.Client(WithAuth(rootUserName, rootPassword))) |
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.
Logically this are 2 subtests, so ideally we should run them in tested t.run(...).
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.
Could you please point to which 2 tests? I guess root and test user, respectively?
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.
It seems not a big deal to me.
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.
Thank you. 2 comments, but not blocking as already the code is a big improvement.
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
Thank you @chaochn47
Related to the ask from #14041 to split tests into smaller pieces.
So code review will be reviewer friendly and easier to reason about.
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.