balancer/randomsubsetting: Extend the unit tests in the randomsubsetting package by marek-szews · Pull Request #9059 · grpc/grpc-go

marek-szews · 2026-04-13T11:16:00Z

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

RELEASE NOTES: none

…ackage. RELEASE NOTES: balancer/randomsubsetting: Implementation of additional UT.

codecov · 2026-04-13T11:18:32Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.13%. Comparing base (d574bad) to head (a2d4f2b).
⚠️ Report is 27 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9059      +/-   ##
==========================================
+ Coverage   83.08%   83.13%   +0.05%     
==========================================
  Files         413      413              
  Lines       33269    33476     +207     
==========================================
+ Hits        27642    27831     +189     
- Misses       4215     4226      +11     
- Partials     1412     1419       +7

Files with missing lines	Coverage Δ
internal/testutils/roundrobin/roundrobin.go	`78.72% <100.00%> (+0.82%)`	⬆️

... and 42 files with indirect coverage changes

🚀 New features to boost your workflow:

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

marek-szews · 2026-04-13T11:30:09Z

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

@marek-szews, PTAL at the comments.

marek-szews

Would you refer to my answers to the asked questions ?

Pranjali-2501 · 2026-04-22T18:23:22Z

+		{
+			eps:        16,
+			subsetSize: 4,
+			iteration:  10,


Why are we using such a small value for iterations here.

That explains the situation perfectly! Using a hardcoded seed in your tests creates a deterministic environment where the algorithm's choices are fixed for every run.
While this is excellent for reproducibility, it can create a "false sense of randomness" if the specific seed happens to generate a sequence that fits your distribution criteria perfectly, even with low iterations.

Pranjali-2501 · 2026-04-22T18:23:24Z

+			positive:   true,
+		},
+		{
+			eps:        4,


When the subset size equals the total number of endpoints (eps == subsetSize), the algorithm returns all endpoints every time.

We can remove this testcase.

I agree with your point regarding the first part of the statement; however, the successful outcome of the verification process is not entirely definitive. It primarily depends on the number of repetitions to ensure the sample is statistically representative.
The distribution will only begin to align once this specific threshold is exceeded. If we wish to examine the Load Balancer algorithm's actual output, a relevant test can be found in the TestCalculateSubset_Simple() function, specifically under the test name SubsetSizeEqualToNumberOfEndpoints.

It was my underlying objective to demonstrate the correlation between EPs, subsetSize, and the total iteration count. As subsetSize converges with EPs, the number of iterations required for successful verification increases significantly.

EndPoints |16 | 8 | 4 |
subsetSize | 4 | 4 | 4 |
repetition 1600 -> 3200 -> 6400

Pranjali-2501 · 2026-04-22T18:23:25Z

+			iteration:  6400,
+			positive:   true,
+		},
+		{


I think we don't need the false assertion here. IMO, just having testcases that are expected to pass is sufficient.

The fact that your tests do not fail is actually the point—it confirms that the algorithm is operationally sound. However, checking for a uniform distribution serves a different purpose than simply checking for a "pass/fail" result.

Pranjali-2501 · 2026-04-22T18:24:29Z

@marek-szews , please fix Testing / static checks (latest-1) (pull_request).

marek-szews

All of your comments have been reviewed and addressed. Please find my response below.

easwars · 2026-05-05T04:20:37Z

+		for i := 0; i < K; i++ {
+			lb := &subsettingBalancer{
+				cfg:        &lbConfig{SubsetSize: uint32(L)},
+				hashSeed:   uint64(i ^ 3 + K*i + L),


What is the significance of the way this seed value is computed? Why can't it just be i?

easwars · 2026-05-05T04:21:24Z

+		N := len(endpoints)
+		L := int(tc.subsetSize)
+		K := int(tc.iteration)
+		p := float64(L) / float64(N) // Probability of x ∈ N being drawn p(x) = L / N
+		E := float64(K) * p          // Expected Value (Mean) E(N) = K * p


Having single letter variable names that are spread through the function does not help with readability at all. Please get rid of these variables and instead use the RHS of these variables directly in the code.

easwars · 2026-05-05T04:21:42Z

+		p := float64(L) / float64(N) // Probability of x ∈ N being drawn p(x) = L / N
+		E := float64(K) * p          // Expected Value (Mean) E(N) = K * p
+
+		EndpointCount := make(map[string]int, N)


s/EndpointCount/endpointCount

easwars · 2026-05-05T04:22:44Z

+			expectedCounts[k] = E
+		}
+
+		err := roundrobin.PearsonsChiSquareTest(t, observedCounts, expectedCounts, 0.05)


How was the value of 0.05 arrived at?

easwars · 2026-05-05T04:27:42Z

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


From what I can see, this test is actually testing the hashing function. If we really want to test the functionality in the policy, we should be overriding the hashing function in some deterministic fashion, so that we can verify the algorithm in the policy.

I'm concerned about a bunch of things in this test, but most importantly, if this test fails, how would one go about debugging? What do the scenarios in the different table entries correspond to, how were they arrived at?

Add more unit tests to increase test coverage of 'randomsubsetting' p…

5eb8fdb

…ackage. RELEASE NOTES: balancer/randomsubsetting: Implementation of additional UT.

marek-szews mentioned this pull request Apr 13, 2026

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

Closed

Pranjali-2501 self-assigned this Apr 14, 2026

Pranjali-2501 self-requested a review April 14, 2026 03:40

Pranjali-2501 reviewed Apr 14, 2026

View reviewed changes

Comment thread balancer/randomsubsetting/randomsubsetting_test.go Outdated

Comment thread balancer/randomsubsetting/randomsubsetting_test.go Outdated

Pranjali-2501 added the Type: Testing label Apr 14, 2026

Pranjali-2501 added this to the 1.81 Release milestone Apr 14, 2026

Pranjali-2501 assigned marek-szews and unassigned Pranjali-2501 Apr 14, 2026

Pranjali-2501 modified the milestones: 1.81 Release, 1.82 Release Apr 15, 2026

marek-szews commented Apr 16, 2026

View reviewed changes

marek-szews added 2 commits April 21, 2026 08:30

Rework of PR. Change function to validate the uniform distribution.

80375b7

Fix static analysis error.

606019e

Pranjali-2501 reviewed Apr 22, 2026

View reviewed changes

marek-szews added 4 commits April 24, 2026 09:01

Rework of PR - small corrections.

f49ff3e

Fix gofmt issues in randomsubsetting_test.go

019955f

Fix gofmt issue (extra blank line) in randomsubsetting_test.go

bb394a0

Fix lint error in roundrobin.go comment

a2d4f2b

marek-szews commented Apr 24, 2026

View reviewed changes

Pranjali-2501 assigned easwars and unassigned marek-szews May 5, 2026

Pranjali-2501 requested a review from easwars May 5, 2026 03:42

easwars reviewed May 5, 2026

View reviewed changes

easwars assigned marek-szews and unassigned easwars May 5, 2026

easwars added the Status: Requires Reporter Clarification label May 5, 2026

Conversation

marek-szews commented Apr 13, 2026 • edited by easwars Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marek-szews commented Apr 13, 2026

Uh oh!

Pranjali-2501 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marek-szews left a comment

Choose a reason for hiding this comment

Uh oh!

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 Apr 24, 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!

Uh oh!

Pranjali-2501 commented Apr 22, 2026

Uh oh!

marek-szews left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marek-szews commented Apr 13, 2026 •

edited by easwars

Loading

codecov Bot commented Apr 13, 2026 •

edited

Loading

marek-szews Apr 24, 2026 •

edited

Loading