Copilot commented on code in PR #3133:
URL: https://github.com/apache/dubbo-go/pull/3133#discussion_r2638201707
##########
protocol/triple/client_test.go:
##########
@@ -94,3 +96,561 @@ func TestDualTransport(t *testing.T) {
})
assert.True(t, ok, "transport should implement http.RoundTripper")
}
+
+func TestClientManager_GetClient(t *testing.T) {
+ tests := []struct {
+ desc string
+ cm *clientManager
+ method string
+ expectErr bool
+ }{
+ {
+ desc: "method exists",
+ cm: &clientManager{
+ triClients: map[string]*tri.Client{
+ "TestMethod":
tri.NewClient(&http.Client{}, "http://localhost:8080/test";),
+ },
+ },
+ method: "TestMethod",
+ expectErr: false,
+ },
+ {
+ desc: "method not exists",
+ cm: &clientManager{
+ triClients: map[string]*tri.Client{
+ "TestMethod":
tri.NewClient(&http.Client{}, "http://localhost:8080/test";),
+ },
+ },
+ method: "NonExistMethod",
+ expectErr: true,
+ },
+ {
+ desc: "empty triClients",
+ cm: &clientManager{
+ triClients: map[string]*tri.Client{},
+ },
+ method: "AnyMethod",
+ expectErr: true,
+ },
+ }
+
+ for _, test := range tests {
+ t.Run(test.desc, func(t *testing.T) {
+ client, err := test.cm.getClient(test.method)
+ if test.expectErr {
+ assert.NotNil(t, err)
+ assert.Nil(t, client)
+ assert.Contains(t, err.Error(), "missing triple
client")
+ } else {
+ assert.Nil(t, err)
+ assert.NotNil(t, client)
+ }
+ })
+ }
+}
+
+func TestClientManager_Close(t *testing.T) {
+ cm := &clientManager{
+ isIDL: true,
+ triClients: map[string]*tri.Client{
+ "Method1": tri.NewClient(&http.Client{},
"http://localhost:8080/test1";),
+ "Method2": tri.NewClient(&http.Client{},
"http://localhost:8080/test2";),
+ },
+ }
+
+ err := cm.close()
+ assert.Nil(t, err)
+}
+
+func TestClientManager_CallMethods_MissingClient(t *testing.T) {
+ cm := &clientManager{
+ triClients: map[string]*tri.Client{},
+ }
+ ctx := context.Background()
+
+ t.Run("callUnary missing client", func(t *testing.T) {
+ err := cm.callUnary(ctx, "NonExist", nil, nil)
+ assert.NotNil(t, err)
+ assert.Contains(t, err.Error(), "missing triple client")
+ })
+
+ t.Run("callClientStream missing client", func(t *testing.T) {
+ stream, err := cm.callClientStream(ctx, "NonExist")
+ assert.NotNil(t, err)
+ assert.Nil(t, stream)
+ assert.Contains(t, err.Error(), "missing triple client")
+ })
+
+ t.Run("callServerStream missing client", func(t *testing.T) {
+ stream, err := cm.callServerStream(ctx, "NonExist", nil)
+ assert.NotNil(t, err)
+ assert.Nil(t, stream)
+ assert.Contains(t, err.Error(), "missing triple client")
+ })
+
+ t.Run("callBidiStream missing client", func(t *testing.T) {
+ stream, err := cm.callBidiStream(ctx, "NonExist")
+ assert.NotNil(t, err)
+ assert.Nil(t, stream)
+ assert.Contains(t, err.Error(), "missing triple client")
+ })
+}
+
+func Test_genKeepAliveOptions(t *testing.T) {
+ defaultInterval, _ :=
time.ParseDuration(constant.DefaultKeepAliveInterval)
+ defaultTimeout, _ :=
time.ParseDuration(constant.DefaultKeepAliveTimeout)
+
+ tests := []struct {
+ desc string
+ url *common.URL
+ tripleConf *global.TripleConfig
+ expectOptsLen int
+ expectInterval time.Duration
+ expectTimeout time.Duration
+ expectErr bool
+ }{
+ {
+ desc: "nil triple config",
+ url: common.NewURLWithOptions(),
+ tripleConf: nil,
+ expectOptsLen: 2, // readMaxBytes, sendMaxBytes
+ expectInterval: defaultInterval,
+ expectTimeout: defaultTimeout,
+ expectErr: false,
+ },
+ {
+ desc: "url with max msg size",
+ url: common.NewURLWithOptions(
+
common.WithParamsValue(constant.MaxCallRecvMsgSize, "10MB"),
+
common.WithParamsValue(constant.MaxCallSendMsgSize, "10MB"),
+ ),
+ tripleConf: nil,
+ expectOptsLen: 2,
+ expectInterval: defaultInterval,
+ expectTimeout: defaultTimeout,
+ expectErr: false,
+ },
+ {
+ desc: "url with keepalive params",
+ url: common.NewURLWithOptions(
+
common.WithParamsValue(constant.KeepAliveInterval, "60s"),
+
common.WithParamsValue(constant.KeepAliveTimeout, "20s"),
+ ),
+ tripleConf: nil,
+ expectOptsLen: 2,
+ expectInterval: 60 * time.Second,
+ expectTimeout: 20 * time.Second,
+ expectErr: false,
+ },
+ {
+ desc: "triple config with keepalive",
+ url: common.NewURLWithOptions(),
+ tripleConf: &global.TripleConfig{
+ KeepAliveInterval: "45s",
+ KeepAliveTimeout: "15s",
+ },
+ expectOptsLen: 2,
+ expectInterval: 45 * time.Second,
+ expectTimeout: 15 * time.Second,
+ expectErr: false,
+ },
+ {
+ desc: "triple config with invalid interval",
+ url: common.NewURLWithOptions(),
+ tripleConf: &global.TripleConfig{
+ KeepAliveInterval: "invalid",
+ },
+ expectErr: true,
+ },
+ {
+ desc: "triple config with invalid timeout",
+ url: common.NewURLWithOptions(),
+ tripleConf: &global.TripleConfig{
+ KeepAliveTimeout: "invalid",
+ },
+ expectErr: true,
+ },
+ {
+ desc: "empty triple config",
+ url: common.NewURLWithOptions(),
+ tripleConf: &global.TripleConfig{},
+ expectOptsLen: 2,
+ expectInterval: defaultInterval,
+ expectTimeout: defaultTimeout,
+ expectErr: false,
+ },
+ }
+
+ for _, test := range tests {
+ t.Run(test.desc, func(t *testing.T) {
+ opts, interval, timeout, err :=
genKeepAliveOptions(test.url, test.tripleConf)
+ if test.expectErr {
+ assert.NotNil(t, err)
+ } else {
+ assert.Nil(t, err)
+ assert.Equal(t, test.expectOptsLen, len(opts))
+ assert.Equal(t, test.expectInterval, interval)
+ assert.Equal(t, test.expectTimeout, timeout)
+ }
+ })
+ }
+}
+
+func Test_newClientManager_Serialization(t *testing.T) {
+ tests := []struct {
+ desc string
+ serialization string
+ expectIDL bool
+ expectPanic bool
+ }{
+ {
+ desc: "protobuf serialization",
+ serialization: constant.ProtobufSerialization,
+ expectIDL: true,
+ expectPanic: false,
+ },
+ {
+ desc: "json serialization",
+ serialization: constant.JSONSerialization,
+ expectIDL: true,
+ expectPanic: false,
+ },
+ {
+ desc: "hessian2 serialization",
+ serialization: constant.Hessian2Serialization,
+ expectIDL: false,
+ expectPanic: false,
+ },
+ {
+ desc: "msgpack serialization",
+ serialization: constant.MsgpackSerialization,
+ expectIDL: false,
+ expectPanic: false,
+ },
+ {
+ desc: "unsupported serialization",
+ serialization: "unsupported",
+ expectPanic: true,
+ },
+ }
+
+ for _, test := range tests {
+ t.Run(test.desc, func(t *testing.T) {
+ url := common.NewURLWithOptions(
+ common.WithLocation("localhost:20000"),
+ common.WithPath("com.example.TestService"),
+ common.WithMethods([]string{"TestMethod"}),
+
common.WithParamsValue(constant.SerializationKey, test.serialization),
+ )
+
+ if test.expectPanic {
+ assert.Panics(t, func() {
+ _, _ = newClientManager(url)
+ })
+ } else {
+ cm, err := newClientManager(url)
+ assert.Nil(t, err)
+ assert.NotNil(t, cm)
+ assert.Equal(t, test.expectIDL, cm.isIDL)
+ }
+ })
+ }
+}
+
+func Test_newClientManager_NoMethods(t *testing.T) {
+ // Test when url has no methods and no RpcServiceKey attribute
+ url := common.NewURLWithOptions(
+ common.WithLocation("localhost:20000"),
+ common.WithPath("com.example.TestService"),
+ )
+
+ cm, err := newClientManager(url)
+ assert.NotNil(t, err)
+ assert.Nil(t, cm)
+ assert.Contains(t, err.Error(), "can't get methods")
+}
+
+func Test_newClientManager_WithMethods(t *testing.T) {
+ url := common.NewURLWithOptions(
+ common.WithLocation("localhost:20000"),
+ common.WithPath("com.example.TestService"),
+ common.WithMethods([]string{"Method1", "Method2", "Method3"}),
+ )
+
+ cm, err := newClientManager(url)
+ assert.Nil(t, err)
+ assert.NotNil(t, cm)
+ assert.Equal(t, 3, len(cm.triClients))
+ assert.Contains(t, cm.triClients, "Method1")
+ assert.Contains(t, cm.triClients, "Method2")
+ assert.Contains(t, cm.triClients, "Method3")
+}
+
+func Test_newClientManager_WithGroupAndVersion(t *testing.T) {
+ url := common.NewURLWithOptions(
+ common.WithLocation("localhost:20000"),
+ common.WithPath("com.example.TestService"),
+ common.WithMethods([]string{"TestMethod"}),
+ common.WithParamsValue(constant.GroupKey, "testGroup"),
+ common.WithParamsValue(constant.VersionKey, "1.0.0"),
+ )
+
+ cm, err := newClientManager(url)
+ assert.Nil(t, err)
+ assert.NotNil(t, cm)
+}
+
+func Test_newClientManager_WithTimeout(t *testing.T) {
+ url := common.NewURLWithOptions(
+ common.WithLocation("localhost:20000"),
+ common.WithPath("com.example.TestService"),
+ common.WithMethods([]string{"TestMethod"}),
+ common.WithParamsValue(constant.TimeoutKey, "5s"),
+ )
+
+ cm, err := newClientManager(url)
+ assert.Nil(t, err)
+ assert.NotNil(t, cm)
+}
+
+func Test_newClientManager_InvalidTLSConfig(t *testing.T) {
+ url := common.NewURLWithOptions(
+ common.WithLocation("localhost:20000"),
+ common.WithPath("com.example.TestService"),
+ common.WithMethods([]string{"TestMethod"}),
+ )
+ // Set invalid TLS config type
+ url.SetAttribute(constant.TLSConfigKey, "invalid-type")
+
+ cm, err := newClientManager(url)
+ assert.NotNil(t, err)
+ assert.Nil(t, cm)
+ assert.Contains(t, err.Error(), "TLSConfig configuration failed")
+}
+
+func Test_newClientManager_HTTP3WithoutTLS(t *testing.T) {
+ url := common.NewURLWithOptions(
+ common.WithLocation("localhost:20000"),
+ common.WithPath("com.example.TestService"),
+ common.WithMethods([]string{"TestMethod"}),
+ )
+ // Enable HTTP/3 without TLS config
+ tripleConfig := &global.TripleConfig{
+ Http3: &global.Http3Config{
+ Enable: true,
+ },
+ }
+ url.SetAttribute(constant.TripleConfigKey, tripleConfig)
+
+ cm, err := newClientManager(url)
+ assert.NotNil(t, err)
+ assert.Nil(t, cm)
+ assert.Contains(t, err.Error(), "must have TLS config")
+}
+
+// mockService is a mock service for testing reflection-based client creation
+type mockService struct{}
+
+func (m *mockService) Reference() string {
+ return "mockService"
+}
+
+func (m *mockService) TestMethod1(ctx context.Context, req string) (string,
error) {
+ return req, nil
+}
+
+func (m *mockService) TestMethod2(ctx context.Context, req int) (int, error) {
+ return req, nil
+}
+
+func Test_newClientManager_WithRpcService(t *testing.T) {
+ url := common.NewURLWithOptions(
+ common.WithLocation("localhost:20000"),
+ common.WithPath("com.example.TestService"),
+ // No methods specified, will use reflection
+ )
+ url.SetAttribute(constant.RpcServiceKey, &mockService{})
+
+ cm, err := newClientManager(url)
+ assert.Nil(t, err)
+ assert.NotNil(t, cm)
+ // Should have methods from mockService (Reference, TestMethod1,
TestMethod2)
+ assert.True(t, len(cm.triClients) >= 2)
+}
+
+func TestDualTransport_Structure(t *testing.T) {
+ keepAliveInterval := 30 * time.Second
+ keepAliveTimeout := 5 * time.Second
+
+ transport := newDualTransport(nil, keepAliveInterval, keepAliveTimeout)
+ assert.NotNil(t, transport)
+
+ dt, ok := transport.(*dualTransport)
+ assert.True(t, ok)
+ assert.NotNil(t, dt.http2Transport)
+ assert.NotNil(t, dt.http3Transport)
+ assert.NotNil(t, dt.altSvcCache)
+}
+
+func Test_newClientManager_HTTP2WithTLS(t *testing.T) {
+ // This test requires valid TLS config files
+ // Skip if files don't exist
+ url := common.NewURLWithOptions(
+ common.WithLocation("localhost:20000"),
+ common.WithPath("com.example.TestService"),
+ common.WithMethods([]string{"TestMethod"}),
+ )
+
+ // Set a valid TLS config structure but with non-existent files
+ // This will test the TLS config parsing path
+ tlsConfig := &global.TLSConfig{
+ CACertFile: "non-existent-ca.crt",
+ TLSCertFile: "non-existent-server.crt",
+ TLSKeyFile: "non-existent-server.key",
+ }
+ url.SetAttribute(constant.TLSConfigKey, tlsConfig)
+
+ // Should fail due to missing cert files, but tests the TLS path
+ cm, err := newClientManager(url)
+ // Either succeeds (if TLS validation is lenient) or fails with TLS
error
+ if err != nil {
+ // Expected - TLS files don't exist
+ t.Logf("Expected TLS error: %v", err)
+ } else {
+ assert.NotNil(t, cm)
+ }
+}
Review Comment:
The test comment states "Skip if files don't exist", but the test doesn't
actually skip execution when files are missing. The test has ambiguous
expectations - it allows both success and failure outcomes. This makes the test
non-deterministic and reduces its value. Either use t.Skip() to properly skip
the test when preconditions aren't met, or make the test expectations concrete
by using mock TLS configuration that doesn't require external files.
##########
protocol/triple/triple_test.go:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package triple
+
+import (
+ "testing"
+)
+
+import (
+ "github.com/stretchr/testify/assert"
+)
+
+import (
+ "dubbo.apache.org/dubbo-go/v3/common/extension"
+)
+
+func TestNewTripleProtocol(t *testing.T) {
+ tp := NewTripleProtocol()
+
+ assert.NotNil(t, tp)
+ assert.NotNil(t, tp.serverMap)
+ assert.Empty(t, tp.serverMap)
+}
+
+func TestGetProtocol(t *testing.T) {
+ // reset singleton for test isolation
+ tripleProtocol = nil
+
+ p1 := GetProtocol()
+ assert.NotNil(t, p1)
+
+ // should return same instance (singleton)
+ p2 := GetProtocol()
+ assert.Same(t, p1, p2)
Review Comment:
This test modifies the global singleton variable tripleProtocol, which can
cause test isolation issues. While the reset is intended for test isolation,
modifying package-level variables in tests can lead to race conditions if tests
run in parallel or affect other tests that depend on the singleton. Consider
using a test-specific approach or ensuring this test runs in isolation with
proper cleanup to restore the original state after the test completes.
##########
protocol/triple/server_test.go:
##########
@@ -135,7 +141,7 @@ func TestServer_ProtocolSelection(t *testing.T) {
// Extract the protocol selection logic for testing
var callProtocol string
- if tripleConfig != nil && tripleConfig.Http3 != nil &&
tripleConfig.Http3.Enable {
+ if tripleConfig.Http3 != nil &&
tripleConfig.Http3.Enable {
Review Comment:
The nil check for tripleConfig was removed, but this creates an
inconsistency with the actual implementation in server.go line 85, which checks
`tripleConf != nil && tripleConf.Http3 != nil && tripleConf.Http3.Enable`.
While this works in the test because tripleConfig is guaranteed to be non-nil
within the test scope (created on line 138-140), it would be better to keep the
condition consistent with the actual implementation to accurately test the
production code path.
```suggestion
if tripleConfig != nil && tripleConfig.Http3 != nil &&
tripleConfig.Http3.Enable {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
