-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
grpclb: Refactoring GrpclbLoadBalancerTest unit tests #11677
base: master
Are you sure you want to change the base?
Changes from 20 commits
ab97045
fef4c92
778cfb4
328bcbf
6a66054
4113845
09c3509
62a88ec
f658685
c51a5e4
02e92c5
6e4efc7
18b74c0
d236b88
f9ffbc6
b44e402
c97e580
0f800f7
d53e504
8882e0f
32ab82e
7714353
1154c7c
958f400
012661e
a26ae99
3b54a76
1ef7a9f
bb5caa8
2eba6a0
a8f7128
79a229c
af3d144
b1f8fa8
5c7f722
cec8e45
e3160d8
860f0a6
c15b040
7908de2
e845fa5
c70226e
7f058af
c4ff128
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1780,48 +1780,23 @@ public void grpclbMultipleAuthorities() throws Exception { | |
|
||
@Test | ||
public void grpclbBalancerStreamClosedAndRetried() throws Exception { | ||
LoadBalanceRequest expectedInitialRequest = | ||
LoadBalanceRequest.newBuilder() | ||
.setInitialRequest( | ||
InitialLoadBalanceRequest.newBuilder().setName(SERVICE_AUTHORITY).build()) | ||
.build(); | ||
LoadBalanceRequest expectedInitialRequest = getIntialLoadBalanceRequest(); | ||
|
||
InOrder inOrder = | ||
inOrder(mockLbService, backoffPolicyProvider, backoffPolicy1, backoffPolicy2, helper); | ||
List<EquivalentAddressGroup> grpclbBalancerList = createResolvedBalancerAddresses(1); | ||
deliverResolvedAddresses(Collections.<EquivalentAddressGroup>emptyList(), grpclbBalancerList); | ||
|
||
assertEquals(1, fakeOobChannels.size()); | ||
@SuppressWarnings("unused") | ||
ManagedChannel oobChannel = fakeOobChannels.poll(); | ||
StreamObserver<LoadBalanceRequest> lbRequestObserver; | ||
StreamObserver<LoadBalanceResponse> lbResponseObserver; | ||
|
||
// First balancer RPC | ||
inOrder.verify(mockLbService).balanceLoad(lbResponseObserverCaptor.capture()); | ||
StreamObserver<LoadBalanceResponse> lbResponseObserver = lbResponseObserverCaptor.getValue(); | ||
assertEquals(1, lbRequestObservers.size()); | ||
StreamObserver<LoadBalanceRequest> lbRequestObserver = lbRequestObservers.poll(); | ||
verify(lbRequestObserver).onNext(eq(expectedInitialRequest)); | ||
assertEquals(0, fakeClock.numPendingTasks(LB_RPC_RETRY_TASK_FILTER)); | ||
|
||
// Balancer closes it immediately (erroneously) | ||
lbResponseObserver.onCompleted(); | ||
getFirstBalancerRpc(expectedInitialRequest, inOrder); | ||
|
||
// Will start backoff sequence 1 (10ns) | ||
inOrder.verify(backoffPolicyProvider).get(); | ||
inOrder.verify(backoffPolicy1).nextBackoffNanos(); | ||
assertEquals(1, fakeClock.numPendingTasks(LB_RPC_RETRY_TASK_FILTER)); | ||
inOrder.verify(helper).refreshNameResolution(); | ||
|
||
// Fast-forward to a moment before the retry | ||
fakeClock.forwardNanos(9); | ||
verifyNoMoreInteractions(mockLbService); | ||
// Then time for retry | ||
fakeClock.forwardNanos(1); | ||
inOrder.verify(mockLbService).balanceLoad(lbResponseObserverCaptor.capture()); | ||
lbResponseObserver = lbResponseObserverCaptor.getValue(); | ||
assertEquals(1, lbRequestObservers.size()); | ||
lbRequestObserver = lbRequestObservers.poll(); | ||
verify(lbRequestObserver).onNext(eq(expectedInitialRequest)); | ||
assertEquals(0, fakeClock.numPendingTasks(LB_RPC_RETRY_TASK_FILTER)); | ||
lbResponseObserver = getFastForwardBeforeRetry(9, inOrder, expectedInitialRequest); | ||
|
||
// Balancer closes it with an error. | ||
lbResponseObserver.onError(Status.UNAVAILABLE.asException()); | ||
|
@@ -1832,16 +1807,7 @@ public void grpclbBalancerStreamClosedAndRetried() throws Exception { | |
inOrder.verify(helper).refreshNameResolution(); | ||
|
||
// Fast-forward to a moment before the retry | ||
fakeClock.forwardNanos(100 - 1); | ||
verifyNoMoreInteractions(mockLbService); | ||
// Then time for retry | ||
fakeClock.forwardNanos(1); | ||
inOrder.verify(mockLbService).balanceLoad(lbResponseObserverCaptor.capture()); | ||
lbResponseObserver = lbResponseObserverCaptor.getValue(); | ||
assertEquals(1, lbRequestObservers.size()); | ||
lbRequestObserver = lbRequestObservers.poll(); | ||
verify(lbRequestObserver).onNext(eq(expectedInitialRequest)); | ||
assertEquals(0, fakeClock.numPendingTasks(LB_RPC_RETRY_TASK_FILTER)); | ||
lbResponseObserver = getFastForwardBeforeRetry(100 - 1, inOrder, expectedInitialRequest); | ||
|
||
// Balancer sends initial response. | ||
lbResponseObserver.onNext(buildInitialResponse()); | ||
|
@@ -1868,21 +1834,58 @@ public void grpclbBalancerStreamClosedAndRetried() throws Exception { | |
inOrder.verify(helper).refreshNameResolution(); | ||
|
||
// Fast-forward to a moment before the retry, the time spent in the last try is deducted. | ||
fakeClock.forwardNanos(10 - 4 - 1); | ||
getFastForwardBeforeRetry(10 - 4 - 1, inOrder, expectedInitialRequest); | ||
|
||
// Wrapping up | ||
verify(backoffPolicyProvider, times(2)).get(); | ||
verify(backoffPolicy1, times(2)).nextBackoffNanos(); | ||
verify(backoffPolicy2, times(1)).nextBackoffNanos(); | ||
verify(helper, times(4)).refreshNameResolution(); | ||
} | ||
|
||
private StreamObserver<LoadBalanceResponse> getFastForwardBeforeRetry(int nanos, InOrder inOrder, | ||
LoadBalanceRequest expectedInitialRequest) { | ||
// Fast-forward to a moment before the retry | ||
fakeClock.forwardNanos(nanos); | ||
verifyNoMoreInteractions(mockLbService); | ||
// Then time for retry | ||
fakeClock.forwardNanos(1); | ||
inOrder.verify(mockLbService).balanceLoad(lbResponseObserverCaptor.capture()); | ||
StreamObserver<LoadBalanceResponse> lbResponseObserver = lbResponseObserverCaptor.getValue(); | ||
assertEquals(1, lbRequestObservers.size()); | ||
lbRequestObserver = lbRequestObservers.poll(); | ||
StreamObserver<LoadBalanceRequest> lbRequestObserver = lbRequestObservers.poll(); | ||
verify(lbRequestObserver).onNext(eq(expectedInitialRequest)); | ||
assertEquals(0, fakeClock.numPendingTasks(LB_RPC_RETRY_TASK_FILTER)); | ||
return lbResponseObserver; | ||
} | ||
|
||
// Wrapping up | ||
verify(backoffPolicyProvider, times(2)).get(); | ||
verify(backoffPolicy1, times(2)).nextBackoffNanos(); | ||
verify(backoffPolicy2, times(1)).nextBackoffNanos(); | ||
verify(helper, times(4)).refreshNameResolution(); | ||
private void getFirstBalancerRpc(LoadBalanceRequest expectedInitialRequest, InOrder inOrder) { | ||
// First balancer RPC | ||
inOrder.verify(mockLbService).balanceLoad(lbResponseObserverCaptor.capture()); | ||
StreamObserver<LoadBalanceResponse> lbResponseObserver = lbResponseObserverCaptor.getValue(); | ||
assertEquals(1, lbRequestObservers.size()); | ||
StreamObserver<LoadBalanceRequest> lbRequestObserver = lbRequestObservers.poll(); | ||
verify(lbRequestObserver).onNext(eq(expectedInitialRequest)); | ||
assertEquals(0, fakeClock.numPendingTasks(LB_RPC_RETRY_TASK_FILTER)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The assertions and verifications here are very common. Can be done differently? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to different method, please review |
||
|
||
// Balancer closes it immediately (erroneously) | ||
lbResponseObserver.onCompleted(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this out to main test method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handled as suggested, please review |
||
} | ||
|
||
private LoadBalanceRequest getIntialLoadBalanceRequest() { | ||
LoadBalanceRequest expectedInitialRequest = | ||
LoadBalanceRequest.newBuilder() | ||
.setInitialRequest( | ||
InitialLoadBalanceRequest.newBuilder().setName(SERVICE_AUTHORITY).build()) | ||
.build(); | ||
|
||
List<EquivalentAddressGroup> grpclbBalancerList = createResolvedBalancerAddresses(1); | ||
deliverResolvedAddresses(Collections.<EquivalentAddressGroup>emptyList(), grpclbBalancerList); | ||
|
||
assertEquals(1, fakeOobChannels.size()); | ||
@SuppressWarnings("unused") | ||
ManagedChannel oobChannel = fakeOobChannels.poll(); | ||
return expectedInitialRequest; | ||
} | ||
|
||
@Test | ||
|
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.
I see the work done by this method aptly suggests it's name to be - fastForwardAndRetry()
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.
Renamed the method