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

grpclb: Refactoring GrpclbLoadBalancerTest unit tests #11677

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

SreeramdasLavanya
Copy link
Contributor

Refactoring Unit Test cases for GrpclbLoadBalancerTest.

Fixes #2772

verify(helper, times(4)).refreshNameResolution();
}

private StreamObserver<LoadBalanceResponse> getFastForwardBeforeRetry(int nanos, InOrder inOrder,
Copy link
Member

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the method

assertEquals(0, fakeClock.numPendingTasks(LB_RPC_RETRY_TASK_FILTER));

// Balancer closes it immediately (erroneously)
lbResponseObserver.onCompleted();
Copy link
Member

Choose a reason for hiding this comment

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

Move this out to main test method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handled as suggested, please review

assertEquals(1, lbRequestObservers.size());
StreamObserver<LoadBalanceRequest> lbRequestObserver = lbRequestObservers.poll();
verify(lbRequestObserver).onNext(eq(expectedInitialRequest));
assertEquals(0, fakeClock.numPendingTasks(LB_RPC_RETRY_TASK_FILTER));
Copy link
Member

Choose a reason for hiding this comment

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

The assertions and verifications here are very common. Can be done differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to different method, please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking GrpclbLoadBalancerTest unit tests should be more unitary
2 participants