grpctest: add test coverages of `ExitIdle` #8375

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

Fixes: #8118

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 82.42%. Comparing base (a5e7cd6) to head (c33ece3).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8375      +/-   ##
==========================================
- Coverage   82.48%   82.42%   -0.06%     
==========================================
  Files         414      414              
  Lines       40464    40464              
==========================================
- Hits        33376    33354      -22     
- Misses       5736     5752      +16     
- Partials     1352     1358       +6

see 24 files with indirect coverage changes

🚀 New features to boost your workflow:

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

eshitachandwani · 2025-06-06T14:30:26Z

internal/balancergroup/balancergroup_test.go

+	bg.Close()
+	bg.ExitIdleOne(testBalancerIDs[0])
+
+	if called {


This should be changed to waiting for a channel similar to the test above and wait for defaultTestShortTimeout to make sure it that ExitIdleOne is not called. Something like :

select { case <-exitIdleCh: t.Fatalf("ExitIdleOne called on sub-balancer after BalancerGroup was closed") case <-time.After(defaultTestShortTimeout): } ```

Similar in all other tests.

eshitachandwani · 2025-06-06T14:33:47Z

internal/balancergroup/balancergroup_test.go

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


I am not sure how helpful this test is, maybe @arjan-bal or @easwars might be able to judge better.

eshitachandwani · 2025-06-06T16:25:29Z

internal/balancergroup/balancergroup_test.go

+	for _, expected := range balancerNames {
+		if !called[expected] {
+			t.Errorf("ExitIdle was not called for sub-balancer registered as %q", expected)
+		}
+	}


This block makes sense only in the case where one or more of the balancer's ExitIdle method is called twice and one of the balancer's method is not called. We could combine this check with the above block itself by checking if called[name] is already true and fail if it is.

@eshitachandwani thx for the review, i updated as combining checks within the select statement.

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.

eshitachandwani · 2025-06-09T05:21:59Z

internal/balancergroup/balancergroup_test.go

+
+	stub.Register(balancerName, stub.BalancerFuncs{
+		UpdateClientConnState: func(_ *stub.BalancerData, _ balancer.ClientConnState) error {
+			called = true


Change to use channel here too. In the check add a defaultShortTestTimeout case along with wait for channel in select block.

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

internal/balancergroup/balancergroup_test.go

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


Change to use channel here too. In the check add a defaultShortTestTimeout case along with wait for channel in select block.

changed all tests to use channel instead of boolean

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.

eshitachandwani · 2025-06-09T05:30:14Z

internal/balancergroup/balancergroup_test.go

+	bg.ExitIdleOne(testBalancerIDs[0])
+
+	select {
+	case <-time.After(time.Second):


Suggested change

case <-time.After(time.Second):

case <-time.After(defaultTestTImeout):

updated as defaultTestTimeout but tests got failed. i should try other way

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.

hugehoo · 2025-07-20T08:48:32Z

Hey @hugehoo are you still working on this PR?

sorry for late, i requested for review now

eshitachandwani · 2025-07-22T09:45:00Z

Hey @hugehoo ,
The name of this test was recently changed : tests (tests, 1.24, arm64, ubuntu-24.04-arm) , and that is why the test is stuck. Can you merge master so that the test can run again?

hugehoo · 2025-07-22T11:51:06Z

Hey @hugehoo , The name of this test was recently changed : tests (tests, 1.24, arm64, ubuntu-24.04-arm) , and that is why the test is stuck. Can you merge master so that the test can run again?

@eshitachandwani thx for the info, i just merged it and run the test again.

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

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

change to use channel instead boolean

0fda6d9

hugehoo requested a review from eshitachandwani July 20, 2025 08:40

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

rollback SendOrFail()

cae4825

hugehoo requested a review from easwars July 20, 2025 13:21

eshitachandwani self-assigned this Jul 22, 2025

Merge branch 'master' into test-coverage-ExitIdle

c33ece3

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

	case <-time.After(time.Second):
	case <-time.After(defaultTestTImeout):

grpctest: add test coverages of ExitIdle #8375

Are you sure you want to change the base?

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eshitachandwani Jun 6, 2025 • 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!

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!

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!

hugehoo commented Jul 20, 2025

Uh oh!

eshitachandwani commented Jul 22, 2025

Uh oh!

hugehoo commented Jul 22, 2025

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

eshitachandwani Jun 6, 2025 •

edited

Loading