From ef292d2057cfdcfa8c07dc32e5b6a6974a2abdad Mon Sep 17 00:00:00 2001 From: hoo Date: Sun, 1 Jun 2025 00:38:25 +0900 Subject: [PATCH 01/17] add tests for `ExitIdle` --- internal/balancergroup/balancergroup_test.go | 82 +++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index 65e5af6312b8..25529b66ba0b 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -505,7 +505,7 @@ func (s) TestBalancerExitIdleOne(t *testing.T) { builder := balancer.Get(balancerName) bg.Add(testBalancerIDs[0], builder) - // Call ExitIdle on the child policy. + // Call ExitIdleOne on the child policy. bg.ExitIdleOne(testBalancerIDs[0]) select { case <-time.After(time.Second): @@ -640,3 +640,83 @@ func (s) TestBalancerGracefulSwitch(t *testing.T) { } } } + +func (s) TestBalancerExitIdle_All(t *testing.T) { + const balancerOne = "stub-balancer-test-balancer-group-exit-idle-one" + const balancerTwo = "stub-balancer-test-balancer-group-exit-idle-two" + const balancerThree = "stub-balancer-test-balancer-group-exit-idle-three" + + 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 + }, + }) + } + + cc := testutils.NewBalancerClientConn(t) + bg := New(Options{ + CC: cc, + BuildOpts: balancer.BuildOptions{}, + StateAggregator: nil, + Logger: nil, + }) + defer bg.Close() + + bg.Add(testIDs[0], balancer.Get(balancerOne)) + bg.Add(testIDs[1], balancer.Get(balancerTwo)) + bg.Add(testIDs[2], balancer.Get(balancerThree)) + + bg.ExitIdle() + + called := make(map[string]bool) + for i := 0; i < len(balancerNames); i++ { + select { + case name := <-exitIdleCh: + called[name] = true + case <-time.After(time.Second): + t.Fatalf("Timeout: ExitIdle not called for all sub-balancers, got %d/%d", len(called), len(balancerNames)) + } + } + + for _, expected := range balancerNames { + if !called[expected] { + t.Errorf("ExitIdle was not called for sub-balancer registered as %q", expected) + } + } +} + +func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) { + const balancerName = "stub-balancer-test-balancer-group-exit-idle-after-close" + + called := false + + stub.Register(balancerName, stub.BalancerFuncs{ + ExitIdle: func(_ *stub.BalancerData) { + called = true + }, + }) + + cc := testutils.NewBalancerClientConn(t) + bg := New(Options{ + CC: cc, + BuildOpts: balancer.BuildOptions{}, + StateAggregator: nil, + Logger: nil, + }) + + bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) + + bg.Close() + + bg.ExitIdle() + + if called { + t.Fatalf("ExitIdle was called on sub-balancer even after BalancerGroup was closed") + } +} From df3f0f0ea19aa9d067c124abb2797997d221522c Mon Sep 17 00:00:00 2001 From: hoo Date: Sun, 1 Jun 2025 00:47:03 +0900 Subject: [PATCH 02/17] add tests for `ExitIdleOne` --- internal/balancergroup/balancergroup_test.go | 66 +++++++++++++++++--- 1 file changed, 57 insertions(+), 9 deletions(-) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index 25529b66ba0b..cde650c1a95d 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -514,6 +514,58 @@ func (s) TestBalancerExitIdleOne(t *testing.T) { } } +func (s) TestBalancerGroup_ExitIdleOne_AfterClose(t *testing.T) { + balancerName := "stub-balancer-test-exit-idle-one-after-close" + called := false + + stub.Register(balancerName, stub.BalancerFuncs{ + ExitIdle: func(_ *stub.BalancerData) { + called = true + }, + }) + + bg := New(Options{ + CC: testutils.NewBalancerClientConn(t), + BuildOpts: balancer.BuildOptions{}, + StateAggregator: nil, + Logger: nil, + }) + + bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) + bg.Close() + bg.ExitIdleOne(testBalancerIDs[0]) + + if called { + t.Fatalf("ExitIdleOne called ExitIdle on sub-balancer after BalancerGroup was closed") + } +} + +func (s) TestBalancerGroup_ExitIdleOne_NonExistentID(t *testing.T) { + balancerName := "stub-balancer-test-exit-idle-one-missing-id" + called := false + + stub.Register(balancerName, stub.BalancerFuncs{ + ExitIdle: func(_ *stub.BalancerData) { + called = true + }, + }) + + bg := New(Options{ + CC: testutils.NewBalancerClientConn(t), + BuildOpts: balancer.BuildOptions{}, + StateAggregator: nil, + Logger: nil, + }) + defer bg.Close() + + bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) + bg.ExitIdleOne("non-existent-id") + + if called { + t.Fatalf("ExitIdleOne called ExitIdle on wrong sub-balancer ID") + } +} + // TestBalancerGracefulSwitch tests the graceful switch functionality for a // child of the balancer group. At first, the child is configured as a round // robin load balancer, and thus should behave accordingly. The test then @@ -642,9 +694,9 @@ func (s) TestBalancerGracefulSwitch(t *testing.T) { } func (s) TestBalancerExitIdle_All(t *testing.T) { - const balancerOne = "stub-balancer-test-balancer-group-exit-idle-one" - const balancerTwo = "stub-balancer-test-balancer-group-exit-idle-two" - const balancerThree = "stub-balancer-test-balancer-group-exit-idle-three" + 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" balancerNames := []string{balancerOne, balancerTwo, balancerThree} testIDs := []string{testBalancerIDs[0], testBalancerIDs[1], testBalancerIDs[2]} @@ -692,8 +744,7 @@ func (s) TestBalancerExitIdle_All(t *testing.T) { } func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) { - const balancerName = "stub-balancer-test-balancer-group-exit-idle-after-close" - + balancerName := "stub-balancer-test-balancer-group-exit-idle-after-close" called := false stub.Register(balancerName, stub.BalancerFuncs{ @@ -702,18 +753,15 @@ func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) { }, }) - cc := testutils.NewBalancerClientConn(t) bg := New(Options{ - CC: cc, + CC: testutils.NewBalancerClientConn(t), BuildOpts: balancer.BuildOptions{}, StateAggregator: nil, Logger: nil, }) bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) - bg.Close() - bg.ExitIdle() if called { From 6ed4f3b2e5d0e6f05e66fcab7a648b7254ef82ed Mon Sep 17 00:00:00 2001 From: hoo Date: Sun, 1 Jun 2025 00:58:01 +0900 Subject: [PATCH 03/17] add tests for `ResolveError`, `UpdateClientConnState` --- internal/balancergroup/balancergroup_test.go | 57 ++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index cde650c1a95d..7dae4fdfbac6 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/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" + called := false + + stub.Register(balancerName, stub.BalancerFuncs{ + UpdateClientConnState: func(_ *stub.BalancerData, _ balancer.ClientConnState) error { + called = true + return nil + }, + }) + + bg := New(Options{ + CC: testutils.NewBalancerClientConn(t), + BuildOpts: balancer.BuildOptions{}, + StateAggregator: nil, + Logger: nil, + }) + + bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) + bg.Close() + + err := bg.UpdateClientConnState(testBalancerIDs[0], balancer.ClientConnState{}) + if err != nil { + t.Fatalf("Expected nil error, got %v", err) + } + if called { + t.Fatalf("UpdateClientConnState was called after BalancerGroup was closed") + } +} + +func (s) TestBalancerGroup_ResolverError_AfterClose(t *testing.T) { + balancerName := "stub-balancer-test-resolver-error-after-close" + called := false + + stub.Register(balancerName, stub.BalancerFuncs{ + ResolverError: func(_ *stub.BalancerData, _ error) { + called = true + }, + }) + + bg := New(Options{ + CC: testutils.NewBalancerClientConn(t), + BuildOpts: balancer.BuildOptions{}, + StateAggregator: nil, + Logger: nil, + }) + + bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) + bg.Close() + + bg.ResolverError(errors.New("test error")) + + if called { + t.Fatalf("ResolverError was called on sub-balancer after BalancerGroup was closed") + } +} + func (s) TestBalancerExitIdleOne(t *testing.T) { const balancerName = "stub-balancer-test-balancergroup-exit-idle-one" exitIdleCh := make(chan struct{}, 1) From c74418dcc767ec776c49c7a6c867f6a59a787a2e Mon Sep 17 00:00:00 2001 From: "elric.lim" Date: Sun, 8 Jun 2025 16:56:48 +0900 Subject: [PATCH 04/17] fix: use channel instead to get signal --- internal/balancergroup/balancergroup_test.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index 7dae4fdfbac6..256287e7ffe6 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -573,11 +573,11 @@ func (s) TestBalancerExitIdleOne(t *testing.T) { func (s) TestBalancerGroup_ExitIdleOne_AfterClose(t *testing.T) { balancerName := "stub-balancer-test-exit-idle-one-after-close" - called := false + exitIdleCh := make(chan struct{}) stub.Register(balancerName, stub.BalancerFuncs{ ExitIdle: func(_ *stub.BalancerData) { - called = true + close(exitIdleCh) }, }) @@ -592,7 +592,9 @@ func (s) TestBalancerGroup_ExitIdleOne_AfterClose(t *testing.T) { bg.Close() bg.ExitIdleOne(testBalancerIDs[0]) - if called { + select { + case <-time.After(time.Second): + case <-exitIdleCh: t.Fatalf("ExitIdleOne called ExitIdle on sub-balancer after BalancerGroup was closed") } } @@ -802,11 +804,11 @@ func (s) TestBalancerExitIdle_All(t *testing.T) { func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) { balancerName := "stub-balancer-test-balancer-group-exit-idle-after-close" - called := false + exitIdleCh := make(chan struct{}) stub.Register(balancerName, stub.BalancerFuncs{ ExitIdle: func(_ *stub.BalancerData) { - called = true + close(exitIdleCh) }, }) @@ -821,7 +823,9 @@ func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) { bg.Close() bg.ExitIdle() - if called { + select { + case <-exitIdleCh: t.Fatalf("ExitIdle was called on sub-balancer even after BalancerGroup was closed") + case <-time.After(defaultTestTimeout): } } From f2b8e021daceef7b31230b4fb152d5381e813ff9 Mon Sep 17 00:00:00 2001 From: "elric.lim" Date: Sun, 8 Jun 2025 17:24:29 +0900 Subject: [PATCH 05/17] fix: combine checks with select sentence --- internal/balancergroup/balancergroup_test.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index 256287e7ffe6..c80605bc92bd 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -789,17 +789,14 @@ func (s) TestBalancerExitIdle_All(t *testing.T) { for i := 0; i < len(balancerNames); i++ { select { case name := <-exitIdleCh: + if called[name] { + t.Fatalf("ExitIdle was called multiple times for sub-balancer %q", name) + } called[name] = true case <-time.After(time.Second): t.Fatalf("Timeout: ExitIdle not called for all sub-balancers, got %d/%d", len(called), len(balancerNames)) } } - - for _, expected := range balancerNames { - if !called[expected] { - t.Errorf("ExitIdle was not called for sub-balancer registered as %q", expected) - } - } } func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) { From 4994ba104c210b55823d7488bd4345a628faa5b8 Mon Sep 17 00:00:00 2001 From: hoo Date: Sun, 15 Jun 2025 23:27:49 +0900 Subject: [PATCH 06/17] TestBalancerGroup_UpdateClientConnState_AfterClose --- internal/balancergroup/balancergroup_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index c80605bc92bd..e667ccf471bc 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -486,11 +486,11 @@ func (s) TestBalancerGroupBuildOptions(t *testing.T) { func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { balancerName := "stub-balancer-test-update-client-state-after-close" - called := false + exitIdleCh := make(chan struct{}) stub.Register(balancerName, stub.BalancerFuncs{ UpdateClientConnState: func(_ *stub.BalancerData, _ balancer.ClientConnState) error { - called = true + exitIdleCh <- struct{}{} return nil }, }) @@ -509,8 +509,10 @@ func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { if err != nil { t.Fatalf("Expected nil error, got %v", err) } - if called { + select { + case <-exitIdleCh: t.Fatalf("UpdateClientConnState was called after BalancerGroup was closed") + case <-time.After(defaultTestShortTimeout): } } From 73ddce790a69ffcf338ace4c2b4c467506f1bbee Mon Sep 17 00:00:00 2001 From: hoo Date: Sun, 15 Jun 2025 23:59:53 +0900 Subject: [PATCH 07/17] fix UpdateClientConnState --- internal/balancergroup/balancergroup_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index e667ccf471bc..ec9397f29f59 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -485,8 +485,9 @@ func (s) TestBalancerGroupBuildOptions(t *testing.T) { } func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { - balancerName := "stub-balancer-test-update-client-state-after-close" - exitIdleCh := make(chan struct{}) + t.Parallel() + balancerName := t.Name() + exitIdleCh := make(chan struct{}, 1) stub.Register(balancerName, stub.BalancerFuncs{ UpdateClientConnState: func(_ *stub.BalancerData, _ balancer.ClientConnState) error { @@ -505,10 +506,10 @@ func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) bg.Close() - err := bg.UpdateClientConnState(testBalancerIDs[0], balancer.ClientConnState{}) - if err != nil { + if err := bg.UpdateClientConnState(testBalancerIDs[0], balancer.ClientConnState{}); err != nil { t.Fatalf("Expected nil error, got %v", err) } + select { case <-exitIdleCh: t.Fatalf("UpdateClientConnState was called after BalancerGroup was closed") From fd47c7d47dade37178c7bad30ad8b307d49cd69e Mon Sep 17 00:00:00 2001 From: hoo Date: Mon, 16 Jun 2025 00:11:59 +0900 Subject: [PATCH 08/17] fix UpdateClientConnState --- internal/balancergroup/balancergroup_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index ec9397f29f59..50ca806223f0 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -30,6 +30,7 @@ import ( "google.golang.org/grpc/balancer/weightedtarget/weightedaggregator" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/internal" "google.golang.org/grpc/internal/balancer/stub" "google.golang.org/grpc/internal/channelz" "google.golang.org/grpc/internal/grpctest" @@ -485,7 +486,6 @@ func (s) TestBalancerGroupBuildOptions(t *testing.T) { } func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { - t.Parallel() balancerName := t.Name() exitIdleCh := make(chan struct{}, 1) @@ -495,6 +495,7 @@ func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { return nil }, }) + internal.BalancerUnregister(balancerName) bg := New(Options{ CC: testutils.NewBalancerClientConn(t), From 347b549e2f85b9b1b83896b9eb4a2f5a1df2d0bb Mon Sep 17 00:00:00 2001 From: "elric.lim" Date: Sun, 20 Jul 2025 17:22:48 +0900 Subject: [PATCH 09/17] fix: test --- xds/server_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/xds/server_test.go b/xds/server_test.go index 13d01a7b974e..c2cf080d29c9 100644 --- a/xds/server_test.go +++ b/xds/server_test.go @@ -382,7 +382,7 @@ func (s) TestServeSuccess(t *testing.T) { modeChangeCh := testutils.NewChannel() modeChangeOption := ServingModeCallback(func(addr net.Addr, args ServingModeChangeArgs) { t.Logf("Server mode change callback invoked for listener %q with mode %q and error %v", addr.String(), args.Mode, args.Err) - modeChangeCh.Send(args.Mode) + modeChangeCh.SendOrFail(args.Mode) }) server, err := NewGRPCServer(modeChangeOption, BootstrapContentsForTesting(bootstrapContents)) if err != nil { @@ -515,7 +515,7 @@ func (s) TestHandleListenerUpdate_NoXDSCreds(t *testing.T) { modeChangeCh := testutils.NewChannel() modeChangeOption := ServingModeCallback(func(addr net.Addr, args ServingModeChangeArgs) { t.Logf("Server mode change callback invoked for listener %q with mode %q and error %v", addr.String(), args.Mode, args.Err) - modeChangeCh.Send(args.Mode) + modeChangeCh.SendOrFail(args.Mode) }) server, err := NewGRPCServer(modeChangeOption, BootstrapContentsForTesting(bootstrapContents)) if err != nil { @@ -607,7 +607,7 @@ func (s) TestHandleListenerUpdate_ErrorUpdate(t *testing.T) { modeChangeCh := testutils.NewChannel() modeChangeOption := ServingModeCallback(func(addr net.Addr, args ServingModeChangeArgs) { t.Logf("Server mode change callback invoked for listener %q with mode %q and error %v", addr.String(), args.Mode, args.Err) - modeChangeCh.Send(args.Mode) + modeChangeCh.SendOrFail(args.Mode) }) server, err := NewGRPCServer(modeChangeOption, BootstrapContentsForTesting(bootstrapContents)) if err != nil { From 0fda6d9212dd51b844f93d34cf3a70e5618f6522 Mon Sep 17 00:00:00 2001 From: "elric.lim" Date: Sun, 20 Jul 2025 17:38:59 +0900 Subject: [PATCH 10/17] change to use channel instead boolean --- internal/balancergroup/balancergroup_test.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index a6914864e7f3..e14ff5583e76 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -520,11 +520,11 @@ func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { func (s) TestBalancerGroup_ResolverError_AfterClose(t *testing.T) { balancerName := "stub-balancer-test-resolver-error-after-close" - called := false + exitIdleCh := make(chan struct{}, 1) stub.Register(balancerName, stub.BalancerFuncs{ ResolverError: func(_ *stub.BalancerData, _ error) { - called = true + exitIdleCh <- struct{}{} }, }) @@ -540,8 +540,10 @@ func (s) TestBalancerGroup_ResolverError_AfterClose(t *testing.T) { bg.ResolverError(errors.New("test error")) - if called { + select { + case <-exitIdleCh: t.Fatalf("ResolverError was called on sub-balancer after BalancerGroup was closed") + case <-time.After(defaultTestShortTimeout): } } @@ -597,7 +599,7 @@ func (s) TestBalancerGroup_ExitIdleOne_AfterClose(t *testing.T) { bg.ExitIdleOne(testBalancerIDs[0]) select { - case <-time.After(time.Second): + case <-time.After(defaultTestTimeout): case <-exitIdleCh: t.Fatalf("ExitIdleOne called ExitIdle on sub-balancer after BalancerGroup was closed") } @@ -826,6 +828,6 @@ func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) { select { case <-exitIdleCh: t.Fatalf("ExitIdle was called on sub-balancer even after BalancerGroup was closed") - case <-time.After(defaultTestTimeout): + case <-time.After(defaultTestShortTimeout): } } From cae4825abbeb0ea0613d293cb6a9a7dc800b6d5f Mon Sep 17 00:00:00 2001 From: "elric.lim" Date: Sun, 20 Jul 2025 17:50:13 +0900 Subject: [PATCH 11/17] rollback SendOrFail() --- xds/server_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/xds/server_test.go b/xds/server_test.go index 9bb53793be70..ed0179b59e1a 100644 --- a/xds/server_test.go +++ b/xds/server_test.go @@ -33,6 +33,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/uuid" + "google.golang.org/grpc" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/credentials/insecure" @@ -382,7 +383,7 @@ func (s) TestServeSuccess(t *testing.T) { modeChangeCh := testutils.NewChannel() modeChangeOption := ServingModeCallback(func(addr net.Addr, args ServingModeChangeArgs) { t.Logf("Server mode change callback invoked for listener %q with mode %q and error %v", addr.String(), args.Mode, args.Err) - modeChangeCh.SendOrFail(args.Mode) + modeChangeCh.Send(args.Mode) }) server, err := NewGRPCServer(modeChangeOption, BootstrapContentsForTesting(bootstrapContents)) if err != nil { @@ -515,7 +516,7 @@ func (s) TestHandleListenerUpdate_NoXDSCreds(t *testing.T) { modeChangeCh := testutils.NewChannel() modeChangeOption := ServingModeCallback(func(addr net.Addr, args ServingModeChangeArgs) { t.Logf("Server mode change callback invoked for listener %q with mode %q and error %v", addr.String(), args.Mode, args.Err) - modeChangeCh.SendOrFail(args.Mode) + modeChangeCh.Send(args.Mode) }) server, err := NewGRPCServer(modeChangeOption, BootstrapContentsForTesting(bootstrapContents)) if err != nil { From bd2f76183b0b74c3c8fdb9f491262c4ae38ac4f3 Mon Sep 17 00:00:00 2001 From: hoo Date: Wed, 30 Jul 2025 00:20:32 +0900 Subject: [PATCH 12/17] update test code --- internal/balancergroup/balancergroup_test.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index e14ff5583e76..f6a87dbe33c4 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -519,7 +519,7 @@ func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { } func (s) TestBalancerGroup_ResolverError_AfterClose(t *testing.T) { - balancerName := "stub-balancer-test-resolver-error-after-close" + balancerName := t.Name() exitIdleCh := make(chan struct{}, 1) stub.Register(balancerName, stub.BalancerFuncs{ @@ -578,7 +578,7 @@ func (s) TestBalancerExitIdleOne(t *testing.T) { } func (s) TestBalancerGroup_ExitIdleOne_AfterClose(t *testing.T) { - balancerName := "stub-balancer-test-exit-idle-one-after-close" + balancerName := t.Name() exitIdleCh := make(chan struct{}) stub.Register(balancerName, stub.BalancerFuncs{ @@ -599,7 +599,7 @@ func (s) TestBalancerGroup_ExitIdleOne_AfterClose(t *testing.T) { bg.ExitIdleOne(testBalancerIDs[0]) select { - case <-time.After(defaultTestTimeout): + case <-time.After(defaultTestShortTimeout): case <-exitIdleCh: t.Fatalf("ExitIdleOne called ExitIdle on sub-balancer after BalancerGroup was closed") } @@ -798,8 +798,14 @@ func (s) TestBalancerExitIdle_All(t *testing.T) { t.Fatalf("ExitIdle was called multiple times for sub-balancer %q", name) } called[name] = true - case <-time.After(time.Second): - t.Fatalf("Timeout: ExitIdle not called for all sub-balancers, got %d/%d", len(called), len(balancerNames)) + case <-time.After(defaultTestTimeout): + var balancers []string + for _, name := range balancerNames { + if !called[name] { + balancers = append(balancers, name) + } + } + t.Fatalf("Timeout waiting for ExitIdle. Missing calls from: %v", balancers) } } } From a32977304ca566441e7cadac8714d34729db28e2 Mon Sep 17 00:00:00 2001 From: hoo Date: Wed, 30 Jul 2025 00:38:13 +0900 Subject: [PATCH 13/17] remove balancer unregister --- internal/balancergroup/balancergroup_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index f6a87dbe33c4..c948703d0411 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -30,7 +30,6 @@ import ( "google.golang.org/grpc/balancer/weightedtarget/weightedaggregator" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/credentials/insecure" - "google.golang.org/grpc/internal" "google.golang.org/grpc/internal/balancer/stub" "google.golang.org/grpc/internal/channelz" "google.golang.org/grpc/internal/grpctest" @@ -495,7 +494,6 @@ func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { return nil }, }) - internal.BalancerUnregister(balancerName) bg := New(Options{ CC: testutils.NewBalancerClientConn(t), From 143c2be3d20b8f8fcde576549b5e4551e8b69a13 Mon Sep 17 00:00:00 2001 From: "elric.lim" Date: Thu, 7 Aug 2025 09:03:27 +0900 Subject: [PATCH 14/17] adapt review --- .idea/workspace.xml | 186 +++++++++++++++++++ internal/balancergroup/balancergroup_test.go | 40 ++-- 2 files changed, 207 insertions(+), 19 deletions(-) create mode 100644 .idea/workspace.xml diff --git a/.idea/workspace.xml b/.idea/workspace.xml new file mode 100644 index 000000000000..7f935b56d102 --- /dev/null +++ b/.idea/workspace.xml @@ -0,0 +1,186 @@ + + + + + + + + + + + + + + { + "lastFilter": { + "state": "OPEN", + "assignee": "hugehoo" + } +} + { + "selectedUrlAndAccountId": { + "url": "https://github.com/hugehoo/grpc-go.git", + "accountId": "98a587ae-f002-4a3e-a3d1-a1cb3b1bb8cb" + } +} + { + "associatedIndex": 4 +} + + + + { + "keyToString": { + "Go Test.TestParseConfig in google.golang.org/grpc/credentials/tls/certprovider/pemfile.executor": "Run", + "Go Test.TestParseConfig/bad_refresh_duration in google.golang.org/grpc/credentials/tls/certprovider/pemfile.executor": "Run", + "Go Test.TestParseConfig/good_config#01 in google.golang.org/grpc/credentials/tls/certprovider/pemfile.executor": "Run", + "Go Test.go test google.golang.org/grpc/credentials/tls/certprovider/pemfile.executor": "Run", + "Go Test.go test google.golang.org/grpc/internal/balancergroup.executor": "Run", + "Go Test.server_test.go.executor": "Run", + "Makefile Target.test.executor": "Run", + "Makefile Target.testrace.executor": "Run", + "Makefile Target.vet.executor": "Run", + "ModuleVcsDetector.initialDetectionPerformed": "true", + "RunOnceActivity.GoLinterPluginOnboarding": "true", + "RunOnceActivity.GoLinterPluginStorageMigration": "true", + "RunOnceActivity.ShowReadmeOnStart": "true", + "RunOnceActivity.git.unshallow": "true", + "RunOnceActivity.go.formatter.settings.were.checked": "true", + "RunOnceActivity.go.migrated.go.modules.settings": "true", + "RunOnceActivity.go.modules.go.list.on.any.changes.was.set": "true", + "git-widget-placeholder": "test-coverage-ExitIdle", + "go.import.settings.migrated": "true", + "go.sdk.automatically.set": "true", + "last_opened_file_path": "/Users/limsunghoo/GolandProjects/hugehoo/grpc-go", + "node.js.detected.package.eslint": "true", + "node.js.selected.package.eslint": "(autodetect)", + "nodejs_package_manager_path": "npm" + } +} + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + 1747651334069 + + + + + + + + + + + + + + true + + + + + + file://$PROJECT_DIR$/examples/features/advancedtls/client/main.go + 54 + + + file://$PROJECT_DIR$/internal/balancergroup/balancergroup_test.go + 522 + + + + + \ No newline at end of file diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index c948703d0411..d1f926d3c75f 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -486,11 +486,11 @@ func (s) TestBalancerGroupBuildOptions(t *testing.T) { func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { balancerName := t.Name() - exitIdleCh := make(chan struct{}, 1) + clientConnStateCh := make(chan struct{}, 1) stub.Register(balancerName, stub.BalancerFuncs{ UpdateClientConnState: func(_ *stub.BalancerData, _ balancer.ClientConnState) error { - exitIdleCh <- struct{}{} + clientConnStateCh <- struct{}{} return nil }, }) @@ -510,7 +510,7 @@ func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { } select { - case <-exitIdleCh: + case <-clientConnStateCh: t.Fatalf("UpdateClientConnState was called after BalancerGroup was closed") case <-time.After(defaultTestShortTimeout): } @@ -518,11 +518,11 @@ func (s) TestBalancerGroup_UpdateClientConnState_AfterClose(t *testing.T) { func (s) TestBalancerGroup_ResolverError_AfterClose(t *testing.T) { balancerName := t.Name() - exitIdleCh := make(chan struct{}, 1) + resolveErrorCh := make(chan struct{}, 1) stub.Register(balancerName, stub.BalancerFuncs{ ResolverError: func(_ *stub.BalancerData, _ error) { - exitIdleCh <- struct{}{} + resolveErrorCh <- struct{}{} }, }) @@ -539,7 +539,7 @@ func (s) TestBalancerGroup_ResolverError_AfterClose(t *testing.T) { bg.ResolverError(errors.New("test error")) select { - case <-exitIdleCh: + case <-resolveErrorCh: t.Fatalf("ResolverError was called on sub-balancer after BalancerGroup was closed") case <-time.After(defaultTestShortTimeout): } @@ -604,12 +604,12 @@ func (s) TestBalancerGroup_ExitIdleOne_AfterClose(t *testing.T) { } func (s) TestBalancerGroup_ExitIdleOne_NonExistentID(t *testing.T) { - balancerName := "stub-balancer-test-exit-idle-one-missing-id" - called := false + balancerName := t.Name() + exitIdleCh := make(chan struct{}, 1) stub.Register(balancerName, stub.BalancerFuncs{ ExitIdle: func(_ *stub.BalancerData) { - called = true + exitIdleCh <- struct{}{} }, }) @@ -624,7 +624,9 @@ func (s) TestBalancerGroup_ExitIdleOne_NonExistentID(t *testing.T) { bg.Add(testBalancerIDs[0], balancer.Get(balancerName)) bg.ExitIdleOne("non-existent-id") - if called { + select { + case <-time.After(defaultTestShortTimeout): + case <-exitIdleCh: t.Fatalf("ExitIdleOne called ExitIdle on wrong sub-balancer ID") } } @@ -756,11 +758,11 @@ func (s) TestBalancerGracefulSwitch(t *testing.T) { } func (s) TestBalancerExitIdle_All(t *testing.T) { - 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" + balancer1 := t.Name() + "-1" + balancer2 := t.Name() + "-2" + balancer3 := t.Name() + "-3" - balancerNames := []string{balancerOne, balancerTwo, balancerThree} + balancerNames := []string{balancer1, balancer2, balancer3} testIDs := []string{testBalancerIDs[0], testBalancerIDs[1], testBalancerIDs[2]} exitIdleCh := make(chan string, len(balancerNames)) @@ -782,9 +784,9 @@ func (s) TestBalancerExitIdle_All(t *testing.T) { }) defer bg.Close() - bg.Add(testIDs[0], balancer.Get(balancerOne)) - bg.Add(testIDs[1], balancer.Get(balancerTwo)) - bg.Add(testIDs[2], balancer.Get(balancerThree)) + bg.Add(testIDs[0], balancer.Get(balancer1)) + bg.Add(testIDs[1], balancer.Get(balancer2)) + bg.Add(testIDs[2], balancer.Get(balancer3)) bg.ExitIdle() @@ -809,12 +811,12 @@ func (s) TestBalancerExitIdle_All(t *testing.T) { } func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) { - balancerName := "stub-balancer-test-balancer-group-exit-idle-after-close" + balancerName := t.Name() exitIdleCh := make(chan struct{}) stub.Register(balancerName, stub.BalancerFuncs{ ExitIdle: func(_ *stub.BalancerData) { - close(exitIdleCh) + exitIdleCh <- struct{}{} }, }) From 9d703f291b91c906283670c19ab50644080903d4 Mon Sep 17 00:00:00 2001 From: "elric.lim" Date: Sat, 9 Aug 2025 01:00:18 +0900 Subject: [PATCH 15/17] adapt review, TestBalancerExitIdle_All --- internal/balancergroup/balancergroup_test.go | 64 +++++++++++--------- 1 file changed, 36 insertions(+), 28 deletions(-) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index d1f926d3c75f..1a734592773c 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -760,20 +760,22 @@ func (s) TestBalancerGracefulSwitch(t *testing.T) { func (s) TestBalancerExitIdle_All(t *testing.T) { balancer1 := t.Name() + "-1" balancer2 := t.Name() + "-2" - balancer3 := t.Name() + "-3" - balancerNames := []string{balancer1, balancer2, balancer3} - testIDs := []string{testBalancerIDs[0], testBalancerIDs[1], testBalancerIDs[2]} + testID1, testID2 := testBalancerIDs[0], testBalancerIDs[1] - exitIdleCh := make(chan string, len(balancerNames)) + exitIdleCh1, exitIdleCh2 := make(chan struct{}, 1), make(chan struct{}, 1) - for _, name := range balancerNames { - stub.Register(name, stub.BalancerFuncs{ - ExitIdle: func(_ *stub.BalancerData) { - exitIdleCh <- name - }, - }) - } + stub.Register(balancer1, stub.BalancerFuncs{ + ExitIdle: func(_ *stub.BalancerData) { + exitIdleCh1 <- struct{}{} + }, + }) + + stub.Register(balancer2, stub.BalancerFuncs{ + ExitIdle: func(_ *stub.BalancerData) { + exitIdleCh2 <- struct{}{} + }, + }) cc := testutils.NewBalancerClientConn(t) bg := New(Options{ @@ -784,28 +786,34 @@ func (s) TestBalancerExitIdle_All(t *testing.T) { }) defer bg.Close() - bg.Add(testIDs[0], balancer.Get(balancer1)) - bg.Add(testIDs[1], balancer.Get(balancer2)) - bg.Add(testIDs[2], balancer.Get(balancer3)) + bg.Add(testID1, balancer.Get(balancer1)) + bg.Add(testID2, balancer.Get(balancer2)) bg.ExitIdle() - called := make(map[string]bool) - for i := 0; i < len(balancerNames); i++ { + errCh := make(chan error, 2) + + go func() { select { - case name := <-exitIdleCh: - if called[name] { - t.Fatalf("ExitIdle was called multiple times for sub-balancer %q", name) - } - called[name] = true + case <-exitIdleCh1: + errCh <- nil case <-time.After(defaultTestTimeout): - var balancers []string - for _, name := range balancerNames { - if !called[name] { - balancers = append(balancers, name) - } - } - t.Fatalf("Timeout waiting for ExitIdle. Missing calls from: %v", balancers) + errCh <- fmt.Errorf("timeout waiting for ExitIdle on balancer1") + } + }() + + go func() { + select { + case <-exitIdleCh2: + errCh <- nil + case <-time.After(defaultTestTimeout): + errCh <- fmt.Errorf("timeout waiting for ExitIdle on balancer2") + } + }() + + for i := 0; i < 2; i++ { + if err := <-errCh; err != nil { + t.Fatal(err) } } } From 3604f685fa9362c48ad121b30992371dc45050c6 Mon Sep 17 00:00:00 2001 From: hoo Date: Thu, 14 Aug 2025 09:37:14 +0900 Subject: [PATCH 16/17] remove unnecessary file --- .idea/workspace.xml | 186 -------------------------------------------- xds/server_test.go | 1 - 2 files changed, 187 deletions(-) delete mode 100644 .idea/workspace.xml diff --git a/.idea/workspace.xml b/.idea/workspace.xml deleted file mode 100644 index 7f935b56d102..000000000000 --- a/.idea/workspace.xml +++ /dev/null @@ -1,186 +0,0 @@ - - - - - - - - - - - - - - { - "lastFilter": { - "state": "OPEN", - "assignee": "hugehoo" - } -} - { - "selectedUrlAndAccountId": { - "url": "https://github.com/hugehoo/grpc-go.git", - "accountId": "98a587ae-f002-4a3e-a3d1-a1cb3b1bb8cb" - } -} - { - "associatedIndex": 4 -} - - - - { - "keyToString": { - "Go Test.TestParseConfig in google.golang.org/grpc/credentials/tls/certprovider/pemfile.executor": "Run", - "Go Test.TestParseConfig/bad_refresh_duration in google.golang.org/grpc/credentials/tls/certprovider/pemfile.executor": "Run", - "Go Test.TestParseConfig/good_config#01 in google.golang.org/grpc/credentials/tls/certprovider/pemfile.executor": "Run", - "Go Test.go test google.golang.org/grpc/credentials/tls/certprovider/pemfile.executor": "Run", - "Go Test.go test google.golang.org/grpc/internal/balancergroup.executor": "Run", - "Go Test.server_test.go.executor": "Run", - "Makefile Target.test.executor": "Run", - "Makefile Target.testrace.executor": "Run", - "Makefile Target.vet.executor": "Run", - "ModuleVcsDetector.initialDetectionPerformed": "true", - "RunOnceActivity.GoLinterPluginOnboarding": "true", - "RunOnceActivity.GoLinterPluginStorageMigration": "true", - "RunOnceActivity.ShowReadmeOnStart": "true", - "RunOnceActivity.git.unshallow": "true", - "RunOnceActivity.go.formatter.settings.were.checked": "true", - "RunOnceActivity.go.migrated.go.modules.settings": "true", - "RunOnceActivity.go.modules.go.list.on.any.changes.was.set": "true", - "git-widget-placeholder": "test-coverage-ExitIdle", - "go.import.settings.migrated": "true", - "go.sdk.automatically.set": "true", - "last_opened_file_path": "/Users/limsunghoo/GolandProjects/hugehoo/grpc-go", - "node.js.detected.package.eslint": "true", - "node.js.selected.package.eslint": "(autodetect)", - "nodejs_package_manager_path": "npm" - } -} - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 1747651334069 - - - - - - - - - - - - - - true - - - - - - file://$PROJECT_DIR$/examples/features/advancedtls/client/main.go - 54 - - - file://$PROJECT_DIR$/internal/balancergroup/balancergroup_test.go - 522 - - - - - \ No newline at end of file diff --git a/xds/server_test.go b/xds/server_test.go index ed0179b59e1a..921b8864570a 100644 --- a/xds/server_test.go +++ b/xds/server_test.go @@ -33,7 +33,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/uuid" - "google.golang.org/grpc" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/credentials/insecure" From 705625a43963bd79091e3945307a69836c162acb Mon Sep 17 00:00:00 2001 From: hoo Date: Fri, 15 Aug 2025 17:51:17 +0900 Subject: [PATCH 17/17] update channel with buffered size 1 --- internal/balancergroup/balancergroup_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/balancergroup/balancergroup_test.go b/internal/balancergroup/balancergroup_test.go index 1a734592773c..5d3029c882a8 100644 --- a/internal/balancergroup/balancergroup_test.go +++ b/internal/balancergroup/balancergroup_test.go @@ -820,7 +820,7 @@ func (s) TestBalancerExitIdle_All(t *testing.T) { func (s) TestBalancerGroup_ExitIdle_AfterClose(t *testing.T) { balancerName := t.Name() - exitIdleCh := make(chan struct{}) + exitIdleCh := make(chan struct{}, 1) stub.Register(balancerName, stub.BalancerFuncs{ ExitIdle: func(_ *stub.BalancerData) {