balancer/randomsubsetting: Extend the unit tests in the randomsubsetting package. #8781

marek-szews · 2025-12-19T11:46:39Z

Add more unit tests to increase test coverage of 'randomsubsetting' package.

RELEASE NOTES:

balancer/randomsubsetting: Implementation of additional UT.

codecov · 2025-12-19T11:50:24Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.28%. Comparing base (2d51986) to head (ae6f169).
⚠️ Report is 59 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8781      +/-   ##
==========================================
- Coverage   83.47%   83.28%   -0.19%     
==========================================
  Files         419      414       -5     
  Lines       32595    32805     +210     
==========================================
+ Hits        27208    27321     +113     
- Misses       4017     4076      +59     
- Partials     1370     1408      +38

see 67 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Pranjali-2501 · 2026-01-23T09:20:00Z

balancer/randomsubsetting/randomsubsetting_ext_test.go

+	_ "google.golang.org/grpc/balancer/roundrobin" // For round_robin LB policy in tests
+)
+
+func (s) TestSubsettingEndpointsDomain(t *testing.T) {


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.

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

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

Pranjali-2501 · 2026-01-23T09:20:55Z

balancer/randomsubsetting/randomsubsetting_ext_test.go

+	}
+}
+
+func (s) TestUniformDistributionOfEndpoints(t *testing.T) {


Add a descriptive comment for this test.

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

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

Done. Appropriate commentary added.

Pranjali-2501 · 2026-01-23T09:44:41Z

balancer/randomsubsetting/randomsubsetting_ext_test.go

+	}
+}
+
+func (s) TestUniformDistributionOfEndpoints(t *testing.T) {


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

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.

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

Pranjali-2501 · 2026-01-23T10:01:12Z

balancer/randomsubsetting/randomsubsetting_ext_test.go

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


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

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.

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

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)

emil10001 · 2026-02-06T17:18:41Z

/gemini review

gemini-code-assist

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.

gemini-code-assist · 2026-02-06T17:20:10Z

balancer/randomsubsetting/randomsubsetting_test.go

+			t.Run(fmt.Sprint(L), func(t *testing.T) {
+				t.Helper()
+				lb := &subsettingBalancer{
+					cfg:        &lbConfig{SubsetSize: uint32(L)},
+					hashSeed:   uint64(i ^ 3 + K*i + L),
+					hashDigest: xxhash.New(),
+				}
+				subsetOfEndpoints := lb.calculateSubset(endpoints)
+
+				for _, ep := range subsetOfEndpoints {
+					EndpointCount[ep.Addresses[0].Addr]++
+				}
+			})


The use of t.Run inside this for loop is incorrect. The purpose of this loop is to perform multiple iterations to gather statistics for a single test, not to define many sub-tests. This approach creates a large number of sub-tests (equal to K) with identical names, which clutters the test output and is not the intended use of t.Run. The subset calculation logic should be executed directly within the loop.

lb := &subsettingBalancer{ cfg: &lbConfig{SubsetSize: uint32(L)}, hashSeed: uint64(i ^ 3 + K*i + L), hashDigest: xxhash.New(), } subsetOfEndpoints := lb.calculateSubset(endpoints) for _, ep := range subsetOfEndpoints { EndpointCount[ep.Addresses[0].Addr]++ }

gemini-code-assist · 2026-02-06T17:20:10Z

balancer/randomsubsetting/randomsubsetting_test.go

+			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)
+			}


Using fmt.Sprint for numeric comparison is inefficient and not idiomatic. A direct integer comparison is clearer and more performant. Additionally, the failure message can be improved to be more concise and avoid printing a potentially large list of endpoints.

Suggested change

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)

}

if got != int(tc.want) {

t.Fatalf("len(subsetOfEndpoints) = %d; want %d", got, tc.want)

}

gemini-code-assist · 2026-02-06T17:20:10Z

balancer/randomsubsetting/randomsubsetting_test.go

+	if chi2 > criticalValue {
+		if dd.expectSuccess {
+			testPassed = false
+		} else {
+			testPassed = true
+		}
+		reportBuilder.WriteString(fmt.Sprintf("Distribution is not uniform (χ²=%.2f > critical value=%.2f)", chi2, criticalValue))
+	} else {
+		if !dd.expectSuccess {
+			testPassed = false
+		}
+		reportBuilder.WriteString(fmt.Sprintf("Distribution is uniform (χ²=%.2f <= critical value=%.2f)", chi2, criticalValue))
+	}


The logic for setting testPassed is correct but unnecessarily complex and verbose. It can be simplified to a single boolean expression, which makes the code more readable and maintainable.

isUniform := chi2 <= criticalValue testPassed = isUniform == dd.expectSuccess if isUniform { reportBuilder.WriteString(fmt.Sprintf("Distribution is uniform (χ²=%.2f <= critical value=%.2f)", chi2, criticalValue)) } else { reportBuilder.WriteString(fmt.Sprintf("Distribution is not uniform (χ²=%.2f > critical value=%.2f)", chi2, criticalValue)) }

gemini-code-assist · 2026-02-06T17:20:10Z

balancer/randomsubsetting/randomsubsetting_test.go

+func initEndpointCount(epCount *map[string]int, eps []resolver.Endpoint) {
+	for _, ep := range eps {
+		(*epCount)[ep.Addresses[0].Addr] = 0
+	}
+}


The function initEndpointCount takes a pointer to a map (*map[string]int). In Go, maps are reference types, so passing them by value is sufficient to allow the function to modify their contents. Using a pointer is unidiomatic unless the function needs to re-assign the map variable itself. Please change the signature to accept a map by value. Note that the call to this function on line 387 will also need to be updated from initEndpointCount(&EndpointCount, endpoints) to initEndpointCount(EndpointCount, endpoints).

func initEndpointCount(epCount map[string]int, eps []resolver.Endpoint) { for _, ep := range eps { epCount[ep.Addresses[0].Addr] = 0 } }

Extend the test coverage of randomsubsetting package.

04b9193

marek-szews changed the title ~~balancer/randomsubsetting: Extend the test coverage of randomsubsetting package.~~ balancer/randomsubsetting: Extend the unit tests in the randomsubsetting package. Dec 19, 2025

Fix the CI checks failures.

a2f51bc

Pranjali-2501 reviewed Jan 23, 2026

View reviewed changes

Pranjali-2501 added the Type: Testing label Jan 23, 2026

Pranjali-2501 assigned marek-szews Jan 23, 2026

Pranjali-2501 added this to the 1.80 Release milestone Jan 23, 2026

easwars assigned easwars and unassigned easwars Jan 27, 2026

easwars added the Status: Requires Reporter Clarification label Jan 27, 2026

marek-szews added 5 commits January 30, 2026 14:54

Rework of PR. Add validation of uniform distribution.

7638565

Rework of PR. Add negative scenarios.

b79c675

Rework of PR. Change of name of function.

26c52b6

Rework of PR. Change of name of function.

7a18d55

Rework of PR. Fix of static analysis error.

ae6f169

marek-szews commented Feb 6, 2026

View reviewed changes

gemini-code-assist bot reviewed Feb 6, 2026

View reviewed changes

balancer/randomsubsetting: Extend the unit tests in the randomsubsetting package. #8781

Are you sure you want to change the base?

balancer/randomsubsetting: Extend the unit tests in the randomsubsetting package. #8781

Uh oh!

Conversation

marek-szews commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marek-szews Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marek-szews left a comment

Choose a reason for hiding this comment

Uh oh!

emil10001 commented Feb 6, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marek-szews commented Dec 19, 2025 •

edited

Loading

codecov bot commented Dec 19, 2025 •

edited

Loading

marek-szews Feb 3, 2026 •

edited

Loading