-
Notifications
You must be signed in to change notification settings - Fork 4.6k
xds/clusterimpl: Convert existing unit tests to e2e style (1/N) #8549
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8549 +/- ##
==========================================
+ Coverage 82.00% 82.01% +0.01%
==========================================
Files 413 415 +2
Lines 40523 40694 +171
==========================================
+ Hits 33230 33377 +147
- Misses 5909 5926 +17
- Partials 1384 1391 +7 🚀 New features to boost your workflow:
|
CircuitBreaking
and Drop
end2end Tests for xds/clusterimpl Mirroring Existing Unit Test Coverage.
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.
Please make your PR description comply with the suggestions from here: https://github.com/grpc/grpc-go/blob/master/CONTRIBUTING.md#pr-descriptions
Also, please consider changing your PR title to a shorter one that is more of a summary than being overly specific in it. Thanks.
CircuitBreaking
and Drop
end2end Tests for xds/clusterimpl Mirroring Existing Unit Test Coverage.CircuitBreaking
.
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.
Also, same comment about the PR title as made in the other PR.
And, we should ideally be getting rid of the existing test as part of it moving it an e2e style, unless we have a good reason not to, in which case, it might make sense to include it in the PR description.
} | ||
gotRPCDropRate := float64(cs.TotalDroppedRequests) / float64(rpcCount) | ||
if (math.Trunc(math.Abs(gotRPCDropRate-wantRPCDropRate)*100) / 100) > errorTolerance { | ||
t.Errorf("Drop rate goes out of errortolerance got: %v, want: %v, totalDroppedRequest: %v, totalIssuesRequest: %v", (math.Trunc(math.Abs(gotRPCDropRate-wantRPCDropRate)*100) / 100), errorTolerance, cs.TotalDroppedRequests, cs.UpstreamLocalityStats[0].TotalIssuedRequests) |
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.
This might be easier to read (both in code and the output) as two separate error statements:
t.Errorf("Drop rate goes out of errortolerance got: %v, want: %v", (math.Trunc(math.Abs(gotRPCDropRate-wantRPCDropRate)*100) / 100), errorTolerance)
t.Errorf("totalDroppedRequest: %v, totalIssuesRequest: %v", cs.TotalDroppedRequests, cs.UpstreamLocalityStats[0].TotalIssuedRequests)
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 have moved this logic to a helper function and need to return as soon as it fails.
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.
Please update the PR title to be consistent with the other PR that you have outstanding.
CircuitBreaking
.
Partially addresses: #6113
Added
TestCircuitBreaking
andTestDropByCategory
end2end Tests in xds/clusterimpl/tests Mirroring Existing Unit Test and delete them.RELEASE NOTES: None