Skip to content
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

integration: V3 grpc with clientv3 (only Put) #4336

Merged
merged 1 commit into from
Jan 29, 2016

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Jan 29, 2016

No description provided.

@gyuho
Copy link
Contributor Author

gyuho commented Jan 29, 2016

@heyitsanthony If you haven't worked on integration test on clientv3, can I work on it?
Kind of duplicate with grpc test, but it's good to have both. Please review.

/cc @xiang90

kvc := clus.RandClient().KV
key := []byte("foo")
reqput := &pb.PutRequest{Key: key, Value: []byte("bar")}
if !useClient {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be separate functions instead of one big if/else

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok will try to wrap it.

@heyitsanthony
Copy link
Contributor

one last bit--- v3_grpc_test.go is getting huge; the non-grpc stuff can go in v3_client_test.go

@xiang90
Copy link
Contributor

xiang90 commented Jan 29, 2016

we should move this to clientv3/integration as I mentioned here #4332 (comment).

We should try our best to not introduce external dependency in our server tests.

@gyuho
Copy link
Contributor Author

gyuho commented Jan 29, 2016

@xiang90 Ok I will put these to the separate directory. Thanks

@gyuho
Copy link
Contributor Author

gyuho commented Jan 29, 2016

@heyitsanthony @xiang90 Just separated. PTAL. Thanks!

// limitations under the License.

/*
Package integration implements tests built upon embedded etcd, and focus on
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not seem correct. this is inside client pkg. we want to test the correctness of client, not etcd.

@xiang90
Copy link
Contributor

xiang90 commented Jan 29, 2016

@gyuho Let's start with the simple test TestKVPut. TestKVPut should test put keys into etcd. After each put, we should do a get to get the put key. If it succeeds, the test passes.

t.Fatalf("couldn't put key (%v)", err)
}

resprange, err := kvc.Range(key, "", 0, 0, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use get.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@gyuho
Copy link
Contributor Author

gyuho commented Jan 29, 2016

@xiang90 Agree. This should be testing only clients.

Just cleaned up to have TestKVPut. PTAL. Thanks!

}

if !bytes.Equal([]byte(val), resprange.Kvs[0].Value) {
t.Errorf("expected value baz, got %s", resprange.Kvs[0].Value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.errorf(val = %s, want %s)

@xiang90
Copy link
Contributor

xiang90 commented Jan 29, 2016

LGTM after fixing the last few comments.

Then we can test KVGet, KVRange, KVDelete, KVDeleteRange.


func TestKVPut(t *testing.T) {
defer testutil.AfterTest(t)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first create a lease with very long ttl here.

if err != nil {
t.Fatalf("couldn't lease (%v)", err)
}
leaseID := lease.LeaseID(lresp.ID)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I create lease here this way, since lease part hasn't been implemented yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea... i would suggest just leave this as a todo instead of using some half-baked stuff in server side.

@gyuho gyuho force-pushed the clientv3_test branch 2 times, most recently from 7d2e0b8 to 7463307 Compare January 29, 2016 05:10
}
if tt.leaseID != lease.LeaseID(resp.Kvs[0].Lease) {
t.Errorf("#%d: val = %d, want %d", i, tt.leaseID, resp.Kvs[0].Lease)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiang90 Just added lease id check here and TODO: create lease from client side above. PTAL.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i mean just remove the lease create stuff and comment out the second test case. Thanks!

@gyuho gyuho force-pushed the clientv3_test branch 2 times, most recently from 4f8b351 to ff599be Compare January 29, 2016 05:19
@gyuho
Copy link
Contributor Author

gyuho commented Jan 29, 2016

Thanks. Just fixed and updated test script to run integration test in clientv3. Will write tests for
other functions in a separate PR.

@xiang90
Copy link
Contributor

xiang90 commented Jan 29, 2016

LGTM if test passes

gyuho added a commit that referenced this pull request Jan 29, 2016
integration: V3 grpc with clientv3 (only Put)
@gyuho gyuho merged commit e7f50d8 into etcd-io:master Jan 29, 2016
@gyuho
Copy link
Contributor Author

gyuho commented Jan 29, 2016

Thanks

@gyuho gyuho deleted the clientv3_test branch January 29, 2016 05:38
@gyuho gyuho restored the clientv3_test branch January 29, 2016 05:38
@gyuho gyuho deleted the clientv3_test branch January 29, 2016 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants