Copilot commented on code in PR #3127: URL: https://github.com/apache/dubbo-go/pull/3127#discussion_r2637687703
########## server/server_test.go: ########## @@ -0,0 +1,417 @@ +/* + * 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 server + +import ( + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common" + "dubbo.apache.org/dubbo-go/v3/global" +) + +// Test NewServer creates a server successfully +func TestNewServer(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + assert.NotNil(t, srv) + assert.NotNil(t, srv.cfg) + assert.NotNil(t, srv.svcOptsMap) + assert.NotNil(t, srv.interfaceNameServices) +} + +// Test NewServer with options +func TestNewServerWithOptions(t *testing.T) { + appCfg := &global.ApplicationConfig{ + Name: "test-app", + } + srv, err := NewServer( + SetServerApplication(appCfg), + WithServerGroup("test-group"), + ) + assert.NoError(t, err) + assert.NotNil(t, srv) + assert.Equal(t, appCfg, srv.cfg.Application) + assert.Equal(t, "test-group", srv.cfg.Provider.Group) +} + +// Test GetServiceOptions +func TestGetServiceOptions(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + srv.registerServiceOptions(svcOpts) + + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, "test-service", retrieved.Id) +} + +// Test GetServiceOptions returns nil for non-existent service +func TestGetServiceOptionsNotFound(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + retrieved := srv.GetServiceOptions("non-existent") + assert.Nil(t, retrieved) +} + +// Test GetServiceInfo +func TestGetServiceInfo(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + svcOpts.info = &common.ServiceInfo{ + InterfaceName: "com.example.TestService", + } + srv.registerServiceOptions(svcOpts) + + info := srv.GetServiceInfo("test-service") + assert.NotNil(t, info) + assert.Equal(t, "com.example.TestService", info.InterfaceName) +} + +// Test GetServiceInfo returns nil for non-existent service +func TestGetServiceInfoNotFound(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + info := srv.GetServiceInfo("non-existent") + assert.Nil(t, info) +} + +// Test GetRPCService +func TestGetRPCService(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + mockService := &mockRPCService{} + svcOpts.rpcService = mockService + srv.registerServiceOptions(svcOpts) + + rpcService := srv.GetRPCService("test-service") + assert.NotNil(t, rpcService) + assert.Equal(t, mockService, rpcService) +} + +// Test GetRPCService returns nil for non-existent service +func TestGetRPCServiceNotFound(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + rpcService := srv.GetRPCService("non-existent") + assert.Nil(t, rpcService) +} + +// Test GetServiceOptionsByInterfaceName +func TestGetServiceOptionsByInterfaceName(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + svcOpts.Service.Interface = "com.example.TestService" + srv.registerServiceOptions(svcOpts) + + retrieved := srv.GetServiceOptionsByInterfaceName("com.example.TestService") + assert.NotNil(t, retrieved) + assert.Equal(t, "test-service", retrieved.Id) +} + +// Test GetServiceOptionsByInterfaceName returns nil for non-existent interface +func TestGetServiceOptionsByInterfaceNameNotFound(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + retrieved := srv.GetServiceOptionsByInterfaceName("non.existent.Service") + assert.Nil(t, retrieved) +} + +// Test registerServiceOptions +func TestRegisterServiceOptions(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + svcOpts.Service.Interface = "com.example.TestService" + + srv.registerServiceOptions(svcOpts) + + // Verify via maps + assert.Equal(t, svcOpts, srv.svcOptsMap["test-service"]) + assert.Equal(t, svcOpts, srv.interfaceNameServices["com.example.TestService"]) +} + +// Test registerServiceOptions with empty interface +func TestRegisterServiceOptionsEmptyInterface(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + svcOpts.Service.Interface = "" + + srv.registerServiceOptions(svcOpts) + + // Should be in svcOptsMap but not in interfaceNameServices + assert.Equal(t, svcOpts, srv.svcOptsMap["test-service"]) + assert.Nil(t, srv.interfaceNameServices[""]) +} + +// Test SetProviderServices +func TestSetProviderServices(t *testing.T) { + // Reset the internal services slice + internalProServices = make([]*InternalService, 0, 16) + + internalService := &InternalService{ + Name: "test-internal-service", + Priority: 0, + Init: func(options *ServiceOptions) (*ServiceDefinition, bool) { + return &ServiceDefinition{ + Handler: &mockRPCService{}, + Info: nil, + Opts: []ServiceOption{}, + }, true + }, + } + + SetProviderServices(internalService) + + assert.Len(t, internalProServices, 1) + assert.Equal(t, "test-internal-service", internalProServices[0].Name) +} + +// Test SetProviderServices with empty name +func TestSetProviderServicesEmptyName(t *testing.T) { + // Reset the internal services slice + internalProServices = make([]*InternalService, 0, 16) + + internalService := &InternalService{ + Name: "", + } + + SetProviderServices(internalService) + + assert.Len(t, internalProServices, 0) +} + +// Test enhanceServiceInfo with nil info +func TestEnhanceServiceInfoNil(t *testing.T) { + info := enhanceServiceInfo(nil) + assert.Nil(t, info) +} + +// Test enhanceServiceInfo with methods +func TestEnhanceServiceInfo(t *testing.T) { + info := &common.ServiceInfo{ + InterfaceName: "com.example.Service", + Methods: []common.MethodInfo{ + { + Name: "sayHello", + }, + }, + } + + result := enhanceServiceInfo(info) + assert.NotNil(t, result) + // Should have doubled methods (original + case-swapped) + assert.Equal(t, 2, len(result.Methods)) + assert.Equal(t, "sayHello", result.Methods[0].Name) + // The swapped version should have capitalized first letter + assert.NotEqual(t, "sayHello", result.Methods[1].Name) +} + +// Test getMetadataPort with default protocol +func TestGetMetadataPortWithDefaultProtocol(t *testing.T) { + opts := defaultServerOptions() + opts.Application.MetadataServicePort = "" + opts.Protocols = map[string]*global.ProtocolConfig{ + "dubbo": { + Port: "20880", + }, + } + + port := getMetadataPort(opts) + assert.Equal(t, 20880, port) +} + +// Test getMetadataPort with explicit port +func TestGetMetadataPortExplicit(t *testing.T) { + opts := defaultServerOptions() + opts.Application.MetadataServicePort = "30880" + + port := getMetadataPort(opts) + assert.Equal(t, 30880, port) +} + +// Test getMetadataPort with no port +func TestGetMetadataPortNoPort(t *testing.T) { + opts := defaultServerOptions() + opts.Application.MetadataServicePort = "" + opts.Protocols = map[string]*global.ProtocolConfig{} + + port := getMetadataPort(opts) + assert.Equal(t, 0, port) +} + +// Test getMetadataPort with invalid port +func TestGetMetadataPortInvalid(t *testing.T) { + opts := defaultServerOptions() + opts.Application.MetadataServicePort = "invalid" + + port := getMetadataPort(opts) + assert.Equal(t, 0, port) +} + +// Mock RPCService for testing +type mockRPCService struct{} + +func (m *mockRPCService) Invoke(methodName string, params []any, results []any) error { + return nil +} + +func (m *mockRPCService) Reference() string { + return "com.example.MockService" +} + +// Test concurrency: multiple goroutines registering services +func TestConcurrentServiceRegistration(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + done := make(chan bool) + for i := 0; i < 10; i++ { + go func(idx int) { + svcOpts := defaultServiceOptions() + svcOpts.Id = "service-" + string(rune(idx)) Review Comment: The conversion `string(rune(idx))` produces incorrect results. For idx values 0-9, this converts them to unprintable control characters (Unicode codepoints 0-9), not the string representations "0"-"9". This means all service IDs will be like "service-" followed by a control character, not human-readable IDs. Use `strconv.Itoa(idx)` or `fmt.Sprintf("service-%d", idx)` to convert the integer to a proper string representation. ########## server/server_test.go: ########## @@ -0,0 +1,417 @@ +/* + * 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 server + +import ( + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common" + "dubbo.apache.org/dubbo-go/v3/global" +) + +// Test NewServer creates a server successfully +func TestNewServer(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + assert.NotNil(t, srv) + assert.NotNil(t, srv.cfg) + assert.NotNil(t, srv.svcOptsMap) + assert.NotNil(t, srv.interfaceNameServices) +} + +// Test NewServer with options +func TestNewServerWithOptions(t *testing.T) { + appCfg := &global.ApplicationConfig{ + Name: "test-app", + } + srv, err := NewServer( + SetServerApplication(appCfg), + WithServerGroup("test-group"), + ) + assert.NoError(t, err) + assert.NotNil(t, srv) + assert.Equal(t, appCfg, srv.cfg.Application) + assert.Equal(t, "test-group", srv.cfg.Provider.Group) +} + +// Test GetServiceOptions +func TestGetServiceOptions(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + srv.registerServiceOptions(svcOpts) + + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, "test-service", retrieved.Id) +} + +// Test GetServiceOptions returns nil for non-existent service +func TestGetServiceOptionsNotFound(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + retrieved := srv.GetServiceOptions("non-existent") + assert.Nil(t, retrieved) +} + +// Test GetServiceInfo +func TestGetServiceInfo(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + svcOpts.info = &common.ServiceInfo{ + InterfaceName: "com.example.TestService", + } + srv.registerServiceOptions(svcOpts) + + info := srv.GetServiceInfo("test-service") + assert.NotNil(t, info) + assert.Equal(t, "com.example.TestService", info.InterfaceName) +} + +// Test GetServiceInfo returns nil for non-existent service +func TestGetServiceInfoNotFound(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + info := srv.GetServiceInfo("non-existent") + assert.Nil(t, info) +} + +// Test GetRPCService +func TestGetRPCService(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + mockService := &mockRPCService{} + svcOpts.rpcService = mockService + srv.registerServiceOptions(svcOpts) + + rpcService := srv.GetRPCService("test-service") + assert.NotNil(t, rpcService) + assert.Equal(t, mockService, rpcService) +} + +// Test GetRPCService returns nil for non-existent service +func TestGetRPCServiceNotFound(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + rpcService := srv.GetRPCService("non-existent") + assert.Nil(t, rpcService) +} + +// Test GetServiceOptionsByInterfaceName +func TestGetServiceOptionsByInterfaceName(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + svcOpts.Service.Interface = "com.example.TestService" + srv.registerServiceOptions(svcOpts) + + retrieved := srv.GetServiceOptionsByInterfaceName("com.example.TestService") + assert.NotNil(t, retrieved) + assert.Equal(t, "test-service", retrieved.Id) +} + +// Test GetServiceOptionsByInterfaceName returns nil for non-existent interface +func TestGetServiceOptionsByInterfaceNameNotFound(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + retrieved := srv.GetServiceOptionsByInterfaceName("non.existent.Service") + assert.Nil(t, retrieved) +} + +// Test registerServiceOptions +func TestRegisterServiceOptions(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + svcOpts.Service.Interface = "com.example.TestService" + + srv.registerServiceOptions(svcOpts) + + // Verify via maps + assert.Equal(t, svcOpts, srv.svcOptsMap["test-service"]) + assert.Equal(t, svcOpts, srv.interfaceNameServices["com.example.TestService"]) +} + +// Test registerServiceOptions with empty interface +func TestRegisterServiceOptionsEmptyInterface(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + svcOpts.Service.Interface = "" + + srv.registerServiceOptions(svcOpts) + + // Should be in svcOptsMap but not in interfaceNameServices + assert.Equal(t, svcOpts, srv.svcOptsMap["test-service"]) + assert.Nil(t, srv.interfaceNameServices[""]) +} + +// Test SetProviderServices +func TestSetProviderServices(t *testing.T) { + // Reset the internal services slice + internalProServices = make([]*InternalService, 0, 16) + + internalService := &InternalService{ + Name: "test-internal-service", + Priority: 0, + Init: func(options *ServiceOptions) (*ServiceDefinition, bool) { + return &ServiceDefinition{ + Handler: &mockRPCService{}, + Info: nil, + Opts: []ServiceOption{}, + }, true + }, + } + + SetProviderServices(internalService) + + assert.Len(t, internalProServices, 1) + assert.Equal(t, "test-internal-service", internalProServices[0].Name) +} + +// Test SetProviderServices with empty name +func TestSetProviderServicesEmptyName(t *testing.T) { + // Reset the internal services slice + internalProServices = make([]*InternalService, 0, 16) Review Comment: Directly modifying the global `internalProServices` variable bypasses the `internalProLock` mutex that protects it in production code. This creates a race condition and violates the synchronization contract. Instead of directly assigning to `internalProServices`, either: 1. Use `SetProviderServices` to properly add services (though you'd still need to clear it safely), or 2. Lock `internalProLock` before modifying `internalProServices`, or 3. Use `t.Cleanup()` to restore the original state after the test Additionally, tests that modify global state like this can interfere with each other when run in parallel. ########## server/server_test.go: ########## @@ -0,0 +1,417 @@ +/* + * 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 server + +import ( + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common" + "dubbo.apache.org/dubbo-go/v3/global" +) + +// Test NewServer creates a server successfully +func TestNewServer(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + assert.NotNil(t, srv) + assert.NotNil(t, srv.cfg) + assert.NotNil(t, srv.svcOptsMap) + assert.NotNil(t, srv.interfaceNameServices) +} + +// Test NewServer with options +func TestNewServerWithOptions(t *testing.T) { + appCfg := &global.ApplicationConfig{ + Name: "test-app", + } + srv, err := NewServer( + SetServerApplication(appCfg), + WithServerGroup("test-group"), + ) + assert.NoError(t, err) + assert.NotNil(t, srv) + assert.Equal(t, appCfg, srv.cfg.Application) + assert.Equal(t, "test-group", srv.cfg.Provider.Group) +} + +// Test GetServiceOptions +func TestGetServiceOptions(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + srv.registerServiceOptions(svcOpts) + + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, "test-service", retrieved.Id) +} + +// Test GetServiceOptions returns nil for non-existent service +func TestGetServiceOptionsNotFound(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + retrieved := srv.GetServiceOptions("non-existent") + assert.Nil(t, retrieved) +} + +// Test GetServiceInfo +func TestGetServiceInfo(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + svcOpts.info = &common.ServiceInfo{ + InterfaceName: "com.example.TestService", + } + srv.registerServiceOptions(svcOpts) + + info := srv.GetServiceInfo("test-service") + assert.NotNil(t, info) + assert.Equal(t, "com.example.TestService", info.InterfaceName) +} + +// Test GetServiceInfo returns nil for non-existent service +func TestGetServiceInfoNotFound(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + info := srv.GetServiceInfo("non-existent") + assert.Nil(t, info) +} + +// Test GetRPCService +func TestGetRPCService(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + mockService := &mockRPCService{} + svcOpts.rpcService = mockService + srv.registerServiceOptions(svcOpts) + + rpcService := srv.GetRPCService("test-service") + assert.NotNil(t, rpcService) + assert.Equal(t, mockService, rpcService) +} + +// Test GetRPCService returns nil for non-existent service +func TestGetRPCServiceNotFound(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + rpcService := srv.GetRPCService("non-existent") + assert.Nil(t, rpcService) +} + +// Test GetServiceOptionsByInterfaceName +func TestGetServiceOptionsByInterfaceName(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + svcOpts.Service.Interface = "com.example.TestService" + srv.registerServiceOptions(svcOpts) + + retrieved := srv.GetServiceOptionsByInterfaceName("com.example.TestService") + assert.NotNil(t, retrieved) + assert.Equal(t, "test-service", retrieved.Id) +} + +// Test GetServiceOptionsByInterfaceName returns nil for non-existent interface +func TestGetServiceOptionsByInterfaceNameNotFound(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + retrieved := srv.GetServiceOptionsByInterfaceName("non.existent.Service") + assert.Nil(t, retrieved) +} + +// Test registerServiceOptions +func TestRegisterServiceOptions(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + svcOpts.Service.Interface = "com.example.TestService" + + srv.registerServiceOptions(svcOpts) + + // Verify via maps + assert.Equal(t, svcOpts, srv.svcOptsMap["test-service"]) + assert.Equal(t, svcOpts, srv.interfaceNameServices["com.example.TestService"]) +} + +// Test registerServiceOptions with empty interface +func TestRegisterServiceOptionsEmptyInterface(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + svcOpts := defaultServiceOptions() + svcOpts.Id = "test-service" + svcOpts.Service.Interface = "" + + srv.registerServiceOptions(svcOpts) + + // Should be in svcOptsMap but not in interfaceNameServices + assert.Equal(t, svcOpts, srv.svcOptsMap["test-service"]) + assert.Nil(t, srv.interfaceNameServices[""]) +} + +// Test SetProviderServices +func TestSetProviderServices(t *testing.T) { + // Reset the internal services slice + internalProServices = make([]*InternalService, 0, 16) + Review Comment: Directly modifying the global `internalProServices` variable bypasses the `internalProLock` mutex that protects it in production code. This creates a race condition and violates the synchronization contract. Instead of directly assigning to `internalProServices`, either: 1. Use `SetProviderServices` to properly add services (though you'd still need to clear it safely), or 2. Lock `internalProLock` before modifying `internalProServices`, or 3. Use `t.Cleanup()` to restore the original state after the test Additionally, tests that modify global state like this can interfere with each other when run in parallel. ```suggestion // Reset the internal services slice in a thread-safe manner and restore it after the test. internalProLock.Lock() origInternalProServices := internalProServices internalProServices = make([]*InternalService, 0, 16) internalProLock.Unlock() t.Cleanup(func() { internalProLock.Lock() defer internalProLock.Unlock() internalProServices = origInternalProServices }) ``` -- 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]
