balancer/randomsubsetting: Extend the unit tests in the randomsubsetting package.#8781
balancer/randomsubsetting: Extend the unit tests in the randomsubsetting package.#8781marek-szews wants to merge 8 commits intogrpc:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8781 +/- ##
==========================================
- Coverage 83.47% 81.40% -2.08%
==========================================
Files 419 416 -3
Lines 32595 33429 +834
==========================================
+ Hits 27208 27212 +4
- Misses 4017 4661 +644
- Partials 1370 1556 +186 🚀 New features to boost your workflow:
|
marek-szews
changed the title
| _ "google.golang.org/grpc/balancer/roundrobin" // For round_robin LB policy in tests | ||
| ) | ||
|
|
||
| func (s) TestSubsettingEndpointsDomain(t *testing.T) { |
There was a problem hiding this comment.
This test is similar to the existing test TestCalculateSubset_Simple.
You can remove this and add more test cases in TestCalculateSubset_Simple if you think of any.
There was a problem hiding this comment.
+1
Yes, the only possible case that could be added to TestCalculateSubset_Simple is the case where the number of endpoints is strictly greater than the subset size.
There was a problem hiding this comment.
Indeed it is only one test case missing, i had renamed function and added to test set.
| } | ||
| } | ||
|
|
||
| func (s) TestUniformDistributionOfEndpoints(t *testing.T) { |
There was a problem hiding this comment.
Add a descriptive comment for this test.
There was a problem hiding this comment.
Move this test to the existing test file randomsubsetting_test.go.
There was a problem hiding this comment.
+1 to both the above comments. Please add comments about why the math is the way it is in the test.
There was a problem hiding this comment.
Done. Appropriate commentary added.
| } | ||
| } | ||
|
|
||
| func (s) TestUniformDistributionOfEndpoints(t *testing.T) { |
There was a problem hiding this comment.
Can you run the test ~10k times on your local to verify that the test is not flaky.
There was a problem hiding this comment.
How often does this flake currently? We do not want to add tests that can flake. We want a test that is guaranteed to pass every time that it runs, and when it fails, it should really mean that there is a bug in the code.
There was a problem hiding this comment.
I have added two examples of tests for which the uniform distribution verification is negative.
|
|
||
| endpoints := makeEndpoints(16) | ||
| expected := iteration / len(endpoints) * subsetSize | ||
| diff = expected / 7 // allow ~14% difference |
There was a problem hiding this comment.
Can you explain the reasoning behind 14% error-rate.
There was a problem hiding this comment.
Can you look at this and modify the test to calculate how many iterations are needed for statistical significance, instead of arbitrarily adding huge error rate to satisfy small range of iterations.
There was a problem hiding this comment.
I carefully analyzed the test acceptance criteria and came to the conclusion that it was too naive. Finally I use the Chi-square goodness-of-fit test, standard statistical method used to validate whether a dataset follows a uniform distribution. It assesses if the observed frequencies in your data align with the expected frequencies of a theoretical distribution where every outcome has an equal chance of occurring.
marek-szews
left a comment
marek-szews
left a comment
There was a problem hiding this comment.
All comments were addressed. Additionally, each test of distribution concludes with a short report.
=== NAME Test/UniformDistributionOfEndpoints
randomsubsetting_test.go:404: Test Case: Endpoints=16, SubsetSize=4, Iterations=10
randomsubsetting_test.go:409: Distribution check passed:
Endpoint | ExpValue | Diff from E | Status
---------------------------------------------------------------------------
endpoint-5 | 2.50 | 1.50 | ! > E ± σ (Noticeable)
endpoint-9 | 2.50 | 1.50 | ! > E ± σ (Noticeable)
endpoint-12 | 2.50 | 1.50 | ! > E ± σ (Noticeable)
endpoint-13 | 2.50 | 1.50 | ! > E ± σ (Noticeable)
endpoint-0 | 2.50 | 2.50 | ! > E ± σ (Noticeable)
endpoint-4 | 2.50 | 0.50 | E ± σ (Normal)
endpoint-10 | 2.50 | 0.50 | E ± σ (Normal)
endpoint-11 | 2.50 | 0.50 | E ± σ (Normal)
endpoint-1 | 2.50 | 1.50 | ! > E ± σ (Noticeable)
endpoint-6 | 2.50 | 0.50 | E ± σ (Normal)
endpoint-2 | 2.50 | 2.50 | ! > E ± σ (Noticeable)
endpoint-7 | 2.50 | 0.50 | E ± σ (Normal)
endpoint-8 | 2.50 | 0.50 | E ± σ (Normal)
endpoint-14 | 2.50 | 0.50 | E ± σ (Normal)
endpoint-15 | 2.50 | 1.50 | ! > E ± σ (Noticeable)
endpoint-3 | 2.50 | 1.50 | ! > E ± σ (Noticeable)
---------------------------------------------------------------------------
Distribution is uniform (χ²=12.00 <= critical value=24.99)
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds valuable unit tests for the randomsubsetting balancer, significantly increasing test coverage. The new tests include simple subset size validation and a statistical test for uniform endpoint distribution. My review focuses on improving the implementation of these tests for better clarity, correctness, and adherence to idiomatic Go practices. The suggestions include simplifying comparison logic, correcting the use of t.Run in a statistical test loop, refactoring complex boolean logic, and using idiomatic function signatures for maps.
marek-szews
left a comment
marek-szews
left a comment
There was a problem hiding this comment.
All comments left by Gemini Code Assist have been resolved. Test execution time has been reduced from 3 seconds to 0.1 seconds. The test output has also become much clearer.
marek-szews
left a comment
marek-szews
left a comment
There was a problem hiding this comment.
Please check the current shape of implementation.
marek-szews
left a comment
marek-szews
left a comment
There was a problem hiding this comment.
Could you verify my rework
|
Apologies for the delay, I'll take a look at it today. |
| } | ||
| } | ||
|
|
||
| func (s) TestSubsettingEndpointsSimply(t *testing.T) { |
There was a problem hiding this comment.
Test TestSubsettingEndpointsSimply and existing test TestCalculateSubset_Simple are doing the same think.
As mentioned here we can get rid of TestSubsettingEndpointsSimply completely and added a testcase in TestCalculateSubset_Simple in which the endpoints are strictly greater than subset size.
Something like this
func (s) TestCalculateSubset_Simple(t *testing.T) {
tests := []struct {
name string
endpoints []resolver.Endpoint
subsetSize uint32
want []resolver.Endpoint
}{
// existing code
{
name: "SubsetSizeLessThanNumberOfEndpoints",
endpoints: makeEndpoints(15),
subsetSize: 5,
want: makeEndpoints(5),
},
}
// existing code
}
There was a problem hiding this comment.
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).
| } | ||
|
|
||
| func (s) TestUniformDistributionOfEndpoints(t *testing.T) { | ||
|
|
There was a problem hiding this comment.
Remove this extra line.
|
|
||
| for _, tc := range testCases { | ||
| endpoints := makeEndpoints(tc.eps) | ||
| // From a set of N numbers, we randomly select K-times a subset of L numbers, |
There was a problem hiding this comment.
Instead of keeping it as an inline comment inside the test loop, we should move it to the official Godoc for TestUniformDistributionOfEndpoints
Something like this in the beginning:
| // From a set of N numbers, we randomly select K-times a subset of L numbers, | |
| // TestUniformDistributionOfEndpoints verifies that the random subsetting | |
| // policy achieves a uniform distribution across backends. From a set of N | |
| // numbers, it randomly selects K-times a subset of L numbers, where L < N. | |
| // Then it calculates how many times each number belonging to set N appears, | |
| // compute the variance and standard deviation, and use a Chi-Square test to | |
| // check whether the distribution is uniform. |
There was a problem hiding this comment.
Done and move to the beginning of function declaration.
There was a problem hiding this comment.
Done and move to the beginning of function declaration.
| sigma := math.Sqrt(variance) // Standard Deviation σ(N) = sqrt(σ²(N)) | ||
|
|
||
| EndpointCount := make(map[string]int, N) | ||
| initEndpointCount(EndpointCount, endpoints) |
There was a problem hiding this comment.
Since initEndpointCount() is a very simple helper used only once within TestUniformDistributionOfEndpoints, we could consider inlining this logic directly into the main test function.
| // ChiSquareCriticalValue calculates the critical value for alpha (e.g., 0.05) | ||
| // and degrees of freedom (df). | ||
| func chiSquareCriticalValue(alpha float64, df float64) float64 { | ||
| // 1. Find the Z-score for the given alpha. |
There was a problem hiding this comment.
Please remove 1./2. from the comments.
| func chiSquareCriticalValue(alpha float64, df float64) float64 { | ||
| // 1. Find the Z-score for the given alpha. | ||
| // For alpha = 0.05 (95% confidence), Z is approx 1.64485 | ||
| z := getZScore(1 - alpha) |
There was a problem hiding this comment.
Same here.
getZScore is only used once within chiSquareCriticalValue and serves as a simple lookup table, we can inline this logic too.
There was a problem hiding this comment.
@marek-szews I have added some comments, please take a look at it.
|
@arjan-bal and @easwars, could you please take a look at the statistical verification logic introduced in Thanks |
We already have an implementation of this statistical verification logic here: @arjan-bal knows this better than anyone else. But we should try and see if this can reused here instead of reimplementing it. |
marek-szews
left a comment
marek-szews
left a comment
There was a problem hiding this comment.
All comments have been taken into account, please check again.
| } | ||
| } | ||
|
|
||
| func (s) TestSubsettingEndpointsSimply(t *testing.T) { |
There was a problem hiding this comment.
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).
| } | ||
|
|
||
| func (s) TestUniformDistributionOfEndpoints(t *testing.T) { | ||
|
|
|
|
||
| for _, tc := range testCases { | ||
| endpoints := makeEndpoints(tc.eps) | ||
| // From a set of N numbers, we randomly select K-times a subset of L numbers, |
There was a problem hiding this comment.
Done and move to the beginning of function declaration.
| sigma := math.Sqrt(variance) // Standard Deviation σ(N) = sqrt(σ²(N)) | ||
|
|
||
| EndpointCount := make(map[string]int, N) | ||
| initEndpointCount(EndpointCount, endpoints) |
|
|
||
| for _, tc := range testCases { | ||
| endpoints := makeEndpoints(tc.eps) | ||
| // From a set of N numbers, we randomly select K-times a subset of L numbers, |
There was a problem hiding this comment.
Done and move to the beginning of function declaration.
| sigma := math.Sqrt(variance) // Standard Deviation σ(N) = sqrt(σ²(N)) | ||
|
|
||
| EndpointCount := make(map[string]int, N) | ||
| initEndpointCount(EndpointCount, endpoints) |
| // ChiSquareCriticalValue calculates the critical value for alpha (e.g., 0.05) | ||
| // and degrees of freedom (df). | ||
| func chiSquareCriticalValue(alpha float64, df float64) float64 { | ||
| // 1. Find the Z-score for the given alpha. |
| func chiSquareCriticalValue(alpha float64, df float64) float64 { | ||
| // 1. Find the Z-score for the given alpha. | ||
| // For alpha = 0.05 (95% confidence), Z is approx 1.64485 | ||
| z := getZScore(1 - alpha) |
5 participants
Add more unit tests to increase test coverage of 'randomsubsetting' package.
RELEASE NOTES: