grpctest: add test coverages of `ExitIdle` #8375

hugehoo · 2025-05-31T15:47:28Z

Add test cases for
- ExitIdle()
- ExitIdleOne()
- UpdateClientConnState
- ResolverError

RELEASE NOTES: N/A

codecov · 2025-05-31T15:52:17Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.82%. Comparing base (a5e7cd6) to head (705625a).
⚠️ Report is 22 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8375      +/-   ##
==========================================
- Coverage   82.48%   81.82%   -0.67%     
==========================================
  Files         414      413       -1     
  Lines       40464    40518      +54     
==========================================
- Hits        33376    33152     -224     
- Misses       5736     5990     +254     
- Partials     1352     1376      +24

see 48 files with indirect coverage changes

🚀 New features to boost your workflow:

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

internal/balancergroup/balancergroup_test.go

eshitachandwani · 2025-06-07T07:43:26Z

internal/balancergroup/balancergroup_test.go

+
+	stub.Register(balancerName, stub.BalancerFuncs{
+		ExitIdle: func(_ *stub.BalancerData) {
+			called = true


Suggested change

called = true

close(exitIdleCh)

Here instead of a variable , a channel can be closed and waited for in a select block since this operation is done only once.

internal/balancergroup/balancergroup_test.go

eshitachandwani · 2025-06-09T05:28:51Z

internal/balancergroup/balancergroup_test.go

+	select {
+	case <-exitIdleCh:
+		t.Fatalf("ExitIdle was called on sub-balancer even after BalancerGroup was closed")
+	case <-time.After(defaultTestTimeout):


Suggested change

case <-time.After(defaultTestTimeout):

case <-time.After(defaultShortTestTimeout):

We use defaultShortTestTimeout to wait for some time for cases that are not expected to happen , and use defaultTestTimeout for cases that are expected to happen.

internal/balancergroup/balancergroup_test.go

easwars · 2025-06-06T21:33:51Z

internal/balancergroup/balancergroup_test.go

@@ -484,6 +484,63 @@ func (s) TestBalancerGroupBuildOptions(t *testing.T) {
 	}
 }

+func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) {
+	balancerName := "stub-balancer-test-update-client-state-after-close"


Nit: balancerName could be set to t.Name(). Here and elsewhere.

eshitachandwani · 2025-06-19T05:56:39Z

Hey @hugehoo , is this ready for review or is it still a work in progress? If it is ready for review , please request a review and unassign yourself so that we can review it.

hugehoo · 2025-06-20T04:29:42Z

Hey @hugehoo , is this ready for review or is it still a work in progress? If it is ready for review , please request a review and unassign yourself so that we can review it.

hi, i'm still working on this pr, testing is still broken. i think i can request a review within this weekend.

eshitachandwani · 2025-07-07T05:56:00Z

Hey @hugehoo are you still working on this PR?

github-actions · 2025-07-20T06:50:51Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

eshitachandwani

LGTM. Adding @easwars as a second reviewer.

easwars · 2025-08-06T18:42:16Z

internal/balancergroup/balancergroup_test.go

@@ -484,6 +484,67 @@ func (s) TestBalancerGroupBuildOptions(t *testing.T) {
 	}
 }

+func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) {
+	balancerName := t.Name()
+	exitIdleCh := make(chan struct{}, 1)


This is not a channel used to indicate that ExitIdle was called. Can you please rename it appropriately. Thanks.

I changed it to clientConnStateCh

easwars · 2025-08-06T18:42:52Z

internal/balancergroup/balancergroup_test.go

+
+func (s) TestBalancerGroup_ResolverError_AfterClose(t *testing.T) {
+	balancerName := t.Name()
+	exitIdleCh := make(chan struct{}, 1)


updated name as resolveErrorCh

easwars · 2025-08-06T18:44:40Z

internal/balancergroup/balancergroup_test.go

+	bg.Add(testBalancerIDs[0], balancer.Get(balancerName))
+	bg.ExitIdleOne("non-existent-id")
+
+	if called {


Please use a channel as used in the other tests instead of a local variable.

chaged to declare channel as exitIdleCh

easwars · 2025-08-06T18:45:18Z

internal/balancergroup/balancergroup_test.go

+}
+
+func (s) TestBalancerGroup_ExitIdleOne_NonExistentID(t *testing.T) {
+	balancerName := "stub-balancer-test-exit-idle-one-missing-id"


This could be t.Name() as well, right? Any reason for that not to be the case?

i missed to fixed it,
just updated as t.Name() here too

internal/balancergroup/balancergroup_test.go

easwars · 2025-08-06T18:47:47Z

internal/balancergroup/balancergroup_test.go

+	balancerOne := "stub-balancer-test-balancer-group-exit-idle-one"
+	balancerTwo := "stub-balancer-test-balancer-group-exit-idle-two"
+	balancerThree := "stub-balancer-test-balancer-group-exit-idle-three"


Does three balancers add any more value than two?

And why not?
balancer1 := t.Name() + "-1"
balancer2 := t.Name() + "-2"

easwars · 2025-08-06T18:51:52Z

internal/balancergroup/balancergroup_test.go

+	balancerNames := []string{balancerOne, balancerTwo, balancerThree}
+	testIDs := []string{testBalancerIDs[0], testBalancerIDs[1], testBalancerIDs[2]}
+
+	exitIdleCh := make(chan string, len(balancerNames))
+
+	for _, name := range balancerNames {
+		stub.Register(name, stub.BalancerFuncs{
+			ExitIdle: func(_ *stub.BalancerData) {
+				exitIdleCh <- name
+			},
+		})
+	}


I would get rid of the slices and the loops and instead keep this downright simple and dump.

// Have two test IDs: testID1 and testID2 // Have two channels, exitIdleCh1 and exitIdleCh2 // Register two stub balancers, the first one for balancer1 that writes to exitIdleCh1 and the second one for balancer2 that writes to exitIdleCh2 // Create the balancer group, and add the child balancers in there // Call ExitIdle // Validate that both exitIdleCh1 and exitIdleCh2 are written to. (I don't think it's an issue if one of those are written to more than once). In this case though, you might have to spawn goroutines to validate that both channels are written to, since we cannot guarantee the order in which they would be written to.

applied in 9d703f2

easwars · 2025-08-06T18:52:06Z

internal/balancergroup/balancergroup_test.go

+}
+
+func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) {
+	balancerName := "stub-balancer-test-balancer-group-exit-idle-after-close"


t.Name here as well.

easwars · 2025-08-06T18:52:58Z

internal/balancergroup/balancergroup_test.go

+
+	stub.Register(balancerName, stub.BalancerFuncs{
+		ExitIdle: func(_ *stub.BalancerData) {
+			close(exitIdleCh)


In some places, you write an empty struct to the channel, while in other places, you close the channel. I understand this is minor and super nit-picky, but can you please choose one approach and stay consistent across the PR. Thanks.

i decided to send an empty struct to a buffered channel, it's more robust and consistent approach i think.

.idea/workspace.xml

xds/server_test.go

easwars

LGTM, modulo the last minor comment.

easwars · 2025-08-14T18:35:32Z

internal/balancergroup/balancergroup_test.go

+
+func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) {
+	balancerName := t.Name()
+	exitIdleCh := make(chan struct{})


Oops. Sorry, didn't catch this earlier. This channel needs to have a buffer of 1. Otherwise, the writer would block if there is no reader.

@easwars fixed here 705625a.
thx for the helpful review 👍🏻

easwars · 2025-08-15T17:57:29Z

Thank you, @hugehoo, for your contribution!

hugehoo · 2025-08-16T11:56:50Z

internal/balancergroup/balancergroup_test.go

+	case <-time.After(defaultTestShortTimeout):
+	}
+}
+
 func (s) TestBalancerExitIdleOne(t *testing.T) {
 	const balancerName = "stub-balancer-test-balancergroup-exit-idle-one"


@easwars i just got nitpicking question.
in this pr, i got reviewed to use t.Name() for balancerName. but some of the cases are still using hard-coded name. Is there any specific reason for it? or would it be better to be updated as t.name() too?

hugehoo added 2 commits June 1, 2025 00:38

add tests for ExitIdle

ef292d2

add tests for ExitIdleOne

df3f0f0

add tests for ResolveError, UpdateClientConnState

6ed4f3b

eshitachandwani self-requested a review June 6, 2025 13:52

eshitachandwani self-assigned this Jun 6, 2025

eshitachandwani reviewed Jun 6, 2025

View reviewed changes

internal/balancergroup/balancergroup_test.go Outdated Show resolved Hide resolved

internal/balancergroup/balancergroup_test.go Show resolved Hide resolved

internal/balancergroup/balancergroup_test.go Outdated Show resolved Hide resolved

eshitachandwani reviewed Jun 7, 2025

View reviewed changes

hugehoo added 2 commits June 8, 2025 16:56

fix: use channel instead to get signal

c74418d

fix: combine checks with select sentence

f2b8e02

hugehoo requested a review from eshitachandwani June 8, 2025 08:37

eshitachandwani reviewed Jun 9, 2025

View reviewed changes

eshitachandwani assigned hugehoo and unassigned eshitachandwani Jun 9, 2025

easwars reviewed Jun 9, 2025

View reviewed changes

hugehoo force-pushed the test-coverage-ExitIdle branch from 9a6e323 to f2b8e02 Compare June 15, 2025 14:14

hugehoo added 3 commits June 15, 2025 23:27

TestBalancerGroup_UpdateClientConnState_AfterClose

4994ba1

fix UpdateClientConnState

73ddce7

fix UpdateClientConnState

fd47c7d

eshitachandwani added Type: Testing Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. labels Jul 14, 2025

eshitachandwani added this to the 1.75 Release milestone Jul 14, 2025

eshitachandwani added the Status: Requires Reporter Clarification label Jul 14, 2025

github-actions bot added the stale label Jul 20, 2025

hugehoo added 2 commits July 20, 2025 17:22

fix: test

347b549

Merge branch 'master' into test-coverage-ExitIdle

1137e54

eshitachandwani approved these changes Jul 30, 2025

View reviewed changes

eshitachandwani assigned easwars and unassigned hugehoo Jul 30, 2025

Pranjali-2501 modified the milestones: 1.75 Release, 1.76 Release Aug 6, 2025

easwars reviewed Aug 6, 2025

View reviewed changes

easwars assigned hugehoo and unassigned easwars Aug 6, 2025

adapt review

143c2be

easwars assigned easwars and unassigned hugehoo Aug 7, 2025

adapt review, TestBalancerExitIdle_All

9d703f2

hugehoo requested a review from easwars August 8, 2025 16:01

easwars reviewed Aug 13, 2025

View reviewed changes

.idea/workspace.xml Outdated Show resolved Hide resolved

easwars assigned hugehoo and unassigned easwars Aug 13, 2025

easwars reviewed Aug 13, 2025

View reviewed changes

xds/server_test.go Outdated Show resolved Hide resolved

remove unnecessary file

3604f68

hugehoo requested a review from easwars August 14, 2025 00:40

easwars approved these changes Aug 14, 2025

View reviewed changes

easwars assigned easwars and hugehoo and unassigned hugehoo and easwars Aug 14, 2025

update channel with buffered size 1

705625a

hugehoo requested a review from easwars August 15, 2025 08:52

easwars approved these changes Aug 15, 2025

View reviewed changes

easwars merged commit 0ebea3e into grpc:master Aug 15, 2025
15 checks passed

hugehoo commented Aug 16, 2025

View reviewed changes

	case <-time.After(defaultTestTimeout):
	case <-time.After(defaultShortTestTimeout):

grpctest: add test coverages of ExitIdle #8375

grpctest: add test coverages of ExitIdle #8375

Uh oh!

Conversation

hugehoo commented May 31, 2025 • edited by eshitachandwani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eshitachandwani commented Jun 19, 2025

Uh oh!

hugehoo commented Jun 20, 2025

Uh oh!

eshitachandwani commented Jul 7, 2025

Uh oh!

github-actions bot commented Jul 20, 2025

Uh oh!

eshitachandwani 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!

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!

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!

Uh oh!

Uh oh!

easwars 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!

Uh oh!

easwars commented Aug 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grpctest: add test coverages of `ExitIdle` #8375

grpctest: add test coverages of `ExitIdle` #8375

hugehoo commented May 31, 2025 •

edited by eshitachandwani

Loading

codecov bot commented May 31, 2025 •

edited

Loading