Skip to content
112 changes: 112 additions & 0 deletions balancer/randomsubsetting/randomsubsetting_ext_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/*
*
* Copyright 2025 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package randomsubsetting

import (
"fmt"
"testing"

xxhash "github.com/cespare/xxhash/v2"
"google.golang.org/grpc/resolver"

_ "google.golang.org/grpc/balancer/roundrobin" // For round_robin LB policy in tests
)

func (s) TestSubsettingEndpointsDomain(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it is only one test case missing, i had renamed function and added to test set.


testCases := []struct {
endpoints []resolver.Endpoint
subsetSize uint32
want uint32
}{
{
endpoints: makeEndpoints(4),
subsetSize: 0,
want: 0,
},
{
endpoints: makeEndpoints(3),
subsetSize: 4,
want: 3,
},
{
endpoints: makeEndpoints(5),
subsetSize: 3,
want: 3,
},
{
endpoints: []resolver.Endpoint{},
subsetSize: 1,
want: 0,
},
}

for _, tc := range testCases {
t.Run(fmt.Sprint(tc.subsetSize), func(t *testing.T) {
b := &subsettingBalancer{
cfg: &lbConfig{SubsetSize: tc.subsetSize},
hashSeed: 0,
hashDigest: xxhash.New(),
}
subsetOfEndpoints := b.calculateSubset(tc.endpoints)
got := len(subsetOfEndpoints)
if fmt.Sprint(got) != fmt.Sprint(tc.want) {
t.Fatalf("subset size=%v; endpoints(%v) = %v; want %v", tc.want, tc.endpoints, got, tc.want)
}
})
}
}

func (s) TestUniformDistributionOfEndpoints(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a descriptive comment for this test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Move this test to the existing test file randomsubsetting_test.go.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to both the above comments. Please add comments about why the math is the way it is in the test.

Copy link
Contributor Author

@marek-szews marek-szews Feb 3, 2026

Choose a reason for hiding this comment

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

Done. Appropriate commentary added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you run the test ~10k times on your local to verify that the test is not flaky.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added two examples of tests for which the uniform distribution verification is negative.


var (
subsetSize = 4
iteration = 1600
diff int
)

endpoints := makeEndpoints(16)
expected := iteration / len(endpoints) * subsetSize
diff = expected / 7 // allow ~14% difference
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain the reasoning behind 14% error-rate.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

EndpointCount := make(map[string]int, len(endpoints))

for i := 0; i < iteration; i++ {
t.Run(fmt.Sprint(subsetSize), func(t *testing.T) {
t.Helper()
lb := &subsettingBalancer{
cfg: &lbConfig{SubsetSize: uint32(subsetSize)},
hashSeed: uint64(i ^ 3 + iteration*i + subsetSize),
hashDigest: xxhash.New(),
}
subsetOfEndpoints := lb.calculateSubset(endpoints)

for _, ep := range subsetOfEndpoints {
EndpointCount[ep.Addresses[0].Addr]++
}
})
}
// Verify the distribution is uniform within a small diff range.
// The expected count for each endpoint is: iteration / total_endpoints * subset_size
// e.g. 1600 / 16 * 4 = 400 +/- diff
for epAddr, count := range EndpointCount {
if count < expected-diff || count > expected+diff {
t.Fatalf("endpoint %v selected %v times; expected <=> %v", epAddr, count, expected)
}
}
}
Loading