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: add TestV3WatchWithPrevKV #6591

Closed
wants to merge 1 commit into from

Conversation

hongchaodeng
Copy link
Contributor

No description provided.

Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

I think this kind of test belongs in ./integration and not ./clientv3/integration since it's testing the server API? /cc @xiang90

if tt.vals[0] != string(watchResp.Events[0].PrevKv.Value) {
t.Errorf("#%d: unequal value: want=%s, get=%s", i, tt.vals[0], watchResp.Events[0].PrevKv.Value)
}
case <-ctx.Done():
Copy link
Contributor

Choose a reason for hiding this comment

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

this should raise a timeout error

for i, tt := range tests {
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)

etcdcli.Put(ctx, tt.key, tt.vals[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

error check?


etcdcli.Put(ctx, tt.key, tt.vals[0])
wch := etcdcli.Watch(ctx, tt.key, clientv3.WithPrefix(), clientv3.WithPrevKV())
etcdcli.Put(ctx, tt.key, tt.vals[1])
Copy link
Contributor

Choose a reason for hiding this comment

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

error check?

@xiang90
Copy link
Contributor

xiang90 commented Oct 5, 2016

let's move this to integration. i think we already have this test somewhere, no?

@heyitsanthony
Copy link
Contributor

@xiang90 there's a test for delete prevkV but nothing for watchers.

@hongchaodeng hongchaodeng changed the title watcher: add TestWatchPrevKV integration: add TestV3WatchWithPrevKV Oct 5, 2016
@hongchaodeng
Copy link
Contributor Author

@heyitsanthony
Fixed. PTAL

t.Fatalf("fail to Put: %v", err)
}

wch := etcdcli.Watch(ctx, tt.key, clientv3.WithPrefix(), clientv3.WithPrevKV())
Copy link
Contributor

Choose a reason for hiding this comment

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

the tests in this file are supposed to use the grpc interface...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we have another place (another dir or file) to test using native clients?
It would be beneficial since users can look into the test and reuse the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

example should not be in the test. example should be in the doc or client example code.

Copy link
Contributor

Choose a reason for hiding this comment

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

clientv3/integration has native client tests.

integration does not depend on clientv3 code, As @heyitsanthony mentioned, for this test, we just want to test the correctness of server side and grpc calls?

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
This is not example/docs.
It's just more close to user or native use cases.

@gyuho If so, I would like to move this test into clientv3/integration.

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.

4 participants