balancer/randomsubsetting: Extend the unit tests in the randomsubsetting package#9059
balancer/randomsubsetting: Extend the unit tests in the randomsubsetting package#9059marek-szews wants to merge 1 commit intogrpc:masterfrom
Conversation
…ackage. RELEASE NOTES: balancer/randomsubsetting: Implementation of additional UT.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9059 +/- ##
==========================================
- Coverage 83.08% 83.08% -0.01%
==========================================
Files 413 413
Lines 33269 33269
==========================================
- Hits 27642 27640 -2
- Misses 4215 4216 +1
- Partials 1412 1413 +1 🚀 New features to boost your workflow:
|
|
This pull request is a continuation of the work started in PR#8781. Due to a high volume of merge conflicts that made the previous PR impossible to merge cleanly, I have decided to close the old one and start fresh here with the latest changes. |
Pranjali-2501
left a comment
Pranjali-2501
left a comment
There was a problem hiding this comment.
@marek-szews, PTAL at the comments.
|
|
||
| // ChiSquareCriticalValue calculates the critical value for alpha (e.g., 0.05) | ||
| // and degrees of freedom (df). | ||
| func chiSquareCriticalValue(alpha float64, df float64) float64 { |
There was a problem hiding this comment.
As mentioned here, we should try and see if this can reused here instead of reimplementing it.
There was a problem hiding this comment.
If I have to reuse this code in randomsubsetting_test.go, then i need to do :
-
Export it (rename it to PearsonsChiSquareTest).
-
Move it to a more general test utility package if we want to follow strict package boundaries (since it is currently under a roundrobin directory).
Please let me know if the above proposal meets your expectations.
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { |
There was a problem hiding this comment.
I don't see any point of adding a new test which will assert against the lenght of subset returned from b.calculateSubset.
Test TestCalculateSubset_Simple is sufficient, you just need to add a case when given endpoints are larger than the subset size.
Please remove TestSubsettingEndpointsSimply test.
There was a problem hiding this comment.
Following your suggestions, I added a new case to the existing test in which the size of the subset is smaller than the number of Endpoints.
FAIL: Test (0.06s)
--- FAIL: Test/CalculateSubset_Simple (0.00s)
--- FAIL: Test/CalculateSubset_Simple/SubsetSizeSmallerThanNumberOfEndpoints#01 (0.00s)
/usr/local/google/home/szews/tmp/grpc-go/balancer/randomsubsetting/randomsubsetting_test.go:171: calculateSubset() returned diff (-want +got):
[]resolver.Endpoint{
+ {Addresses: []resolver.Address{s{Addr: "endpoint-13", ServerName: "", }}},
+ {Addresses: []resolver.Address{s{Addr: "endpoint-10", ServerName: "", }}},
+ {Addresses: []resolver.Address{s{Addr: "endpoint-12", ServerName: "", }}},
+ {Addresses: []resolver.Address{s{Addr: "endpoint-6", ServerName: "", }}},
{Addresses: {s{Addr: "endpoint-0", ServerName: "", }}},
- {Addresses: []resolver.Address{s{Addr: "endpoint-1", ServerName: "", }}},
- {Addresses: []resolver.Address{s{Addr: "endpoint-2", ServerName: "", }}},
- {Addresses: []resolver.Address{s{Addr: "endpoint-3", ServerName: "", }}},
- {Addresses: []resolver.Address{s{Addr: "endpoint-4", ServerName: "", }}},
}
FAIL
FAIL google.golang.org/grpc/balancer/randomsubsetting 0.105s
FAIL
I have described this problem last time in details:
Not exactly like that, function makeEndpoints(15) will produce the series of endpoints with a arithmetic order {endpoint_0, endpoint_1, ..., endpoint_14}. Same makeEndpoints(5) give us {endpoint_0, endpoint_1,.., endpoint_4}. But the result of LB.calculateSubset(eps) is unpredictable. One time we can received subset {endpoint_4, endpoint_5, endpoint_8, endpoint_11, endpoint_14} another time completely different. Validation of members of subset 99% will failed. I have left those two tests separated, because the first of them, check the content of set (boundary condition is meet so method always return the unchanged set - with a origin order) while the second test due to randomisation of content of subset, validate only the cardinality of the set (number of elements).
<<<<
marek-szews
left a comment
marek-szews
left a comment
There was a problem hiding this comment.
Would you refer to my answers to the asked questions ?
2 participants
Add more unit tests to increase test coverage of 'randomsubsetting' package.
RELEASE NOTES: none