integration: test with multi events in synced #4239
Conversation
| @@ -636,6 +634,68 @@ func TestV3WatchMultipleEventsFromCurrentRevision(t *testing.T) { | |||
| clus.Terminate(t) | |||
| } | |||
|
|
|||
| // TestV3WatchMultipleEventsFromUnsynced tests Watch APIs from unsynced watchers. | |||
| func TestV3WatchMultipleEventsFromUnsynced(t *testing.T) { | |||
There was a problem hiding this comment.
This is really similar to TestV3WatchMultipleEventsFromCurrentRevision. Can the insert logic be unified to use only txns or multiple PUTs? If so, there can be a helper function testWatchMultipleEvents(t *testing.T, rev int64) that handles both cases without so much code duplication.
There was a problem hiding this comment.
Sure I will fix it. Thanks!
|
@heyitsanthony Just addressed. PTAL. Thanks! |
| t.Fatalf("kvc.Txn failed: %+v", tresp) | ||
| if startRev == 0 { // test synced | ||
| for i := 0; i < 3; i++ { | ||
| if _, err := kvc.Put(context.TODO(), &pb.PutRequest{Key: []byte(fmt.Sprintf("foo%d", i)), Value: []byte("bar")}); err != nil { |
There was a problem hiding this comment.
would this work just as well with only txns or puts but not both? if so it would simplify the logic a bit
There was a problem hiding this comment.
Right. will fix the logic to make it simpler.
| testV3WatchMultipleEvents(t, 1, true) | ||
| } | ||
|
|
||
| func testV3WatchMultipleEvents(t *testing.T, startRev int64, isTxn bool) { |
There was a problem hiding this comment.
@heyitsanthony Just fixed by adding isTxn option. PTAL. Thanks!
There was a problem hiding this comment.
OK, if txn/put matters, can you add tests for testV3WatchMultipleEvents(t, 0, true) and testV3WatchMultipleEvents(t, 1, false)?
|
lgtm once the extra txn/put cases are covered |
|
@heyitsanthony All cases are covered. Will merge after CI passes. Thanks! |
| t.Fatalf("kvc.Txn failed: %+v", tresp) | ||
| var wevents []*storagepb.Event | ||
|
|
||
| if isTxn { |
There was a problem hiding this comment.
this whole if else block looks pretty crazy. probably we need to refactor it.
There was a problem hiding this comment.
Agree. Will try to make it cleaner. Thanks!
gyuho
changed the title
gyuho
force-pushed
the
multi_unsynced
branch
2 times, most recently
from
f03e593 to
4607014
Compare
|
@xiang90 @heyitsanthony Could you take a look again? I made it simpler with smaller testing cases. Thanks, |
|
Closing this. Will figure out test failures. |
Related #4216.