Copilot commented on code in PR #3129: URL: https://github.com/apache/dubbo-go/pull/3129#discussion_r2637798995
########## server/server_test.go: ########## @@ -0,0 +1,477 @@ +/* + * 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 ( + "strconv" + "sync" + "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) Review Comment: The test is accessing unexported field `srv.cfg` directly. While this is technically allowed within the same package, it would be better to verify the server initialization through public API behavior rather than accessing internal state. Consider removing this assertion or verifying initialization indirectly through the public API (which the test already does in lines 42-49). ```suggestion ``` ########## server/server_test.go: ########## @@ -0,0 +1,477 @@ +/* + * 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 ( + "strconv" + "sync" + "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) + + // Verify server is properly initialized by using public API + // Try to register and retrieve a service to verify internal maps work + svcOpts := defaultServiceOptions() + svcOpts.Id = "init-test-service" + srv.registerServiceOptions(svcOpts) + + retrieved := srv.GetServiceOptions("init-test-service") + assert.NotNil(t, retrieved, "Server should have properly initialized service maps") +} + +// 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 public API methods + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, svcOpts, retrieved) + + retrievedByInterface := srv.GetServiceOptionsByInterfaceName("com.example.TestService") + assert.NotNil(t, retrievedByInterface) + assert.Equal(t, svcOpts, retrievedByInterface) +} + +// 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 retrievable by service ID but not by interface name + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, svcOpts, retrieved) + + // Empty interface should not be retrievable by interface name + retrievedByInterface := srv.GetServiceOptionsByInterfaceName("") + assert.Nil(t, retrievedByInterface) +} + +// Test SetProviderServices +func TestSetProviderServices(t *testing.T) { + // Preserve original internal services and restore after test + origInternalProServices := internalProServices + t.Cleanup(func() { + internalProServices = origInternalProServices + }) + // Lock and backup original state + internalProLock.Lock() + originalServices := internalProServices + internalProServices = make([]*InternalService, 0, 16) + + // Unlock before calling SetProviderServices (which needs the lock) + internalProLock.Unlock() + + // Register cleanup to restore original state + t.Cleanup(func() { + internalProLock.Lock() + defer internalProLock.Unlock() + internalProServices = originalServices + }) + + internalService := &InternalService{ + Name: "test-internal-service", + Priority: 0, + Init: func(options *ServiceOptions) (*ServiceDefinition, bool) { + return &ServiceDefinition{ + Handler: &mockRPCService{}, + Info: nil, + Opts: []ServiceOption{}, + }, true + }, + } + + // This function will acquire the lock internally + SetProviderServices(internalService) + + // Access the global variable safely with lock + internalProLock.Lock() + defer internalProLock.Unlock() + assert.Len(t, internalProServices, 1) + assert.Equal(t, "test-internal-service", internalProServices[0].Name) +} + +// Test SetProviderServices with empty name +func TestSetProviderServicesEmptyName(t *testing.T) { + // Lock and backup original state + internalProLock.Lock() + originalServices := internalProServices + internalProServices = make([]*InternalService, 0, 16) + + // Unlock before calling SetProviderServices (which needs the lock) + internalProLock.Unlock() + + // Register cleanup to restore original state + t.Cleanup(func() { + internalProLock.Lock() + defer internalProLock.Unlock() + internalProServices = originalServices + }) + + internalService := &InternalService{ + Name: "", + } + + // This function will acquire the lock internally + SetProviderServices(internalService) + + // Access the global variable safely with lock + internalProLock.Lock() + defer internalProLock.Unlock() + 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) + + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + svcOpts := defaultServiceOptions() + svcOpts.Id = "service-" + strconv.Itoa(idx) + srv.registerServiceOptions(svcOpts) + }(i) + } + + wg.Wait() + + // Verify all services were registered using public API + for i := 0; i < 10; i++ { + svcID := "service-" + strconv.Itoa(i) + retrieved := srv.GetServiceOptions(svcID) + assert.NotNil(t, retrieved, "Service %s should be registered", svcID) + assert.Equal(t, svcID, retrieved.Id) + } +} + +// Test Register with ServiceInfo +func TestRegisterWithServiceInfo(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + handler := &mockRPCService{} + info := &common.ServiceInfo{ + InterfaceName: "com.example.Service", + Methods: []common.MethodInfo{ + {Name: "Method1"}, + }, + } + + err = srv.Register(handler, info) + assert.NoError(t, err) + + // Service should be registered + svcOpts := srv.GetServiceOptions(handler.Reference()) + assert.NotNil(t, svcOpts) +} + +// Test RegisterService without ServiceInfo +func TestRegisterService(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + handler := &mockRPCService{} + err = srv.RegisterService(handler) + assert.NoError(t, err) + + // Service should be registered with handler reference name + svcOpts := srv.GetServiceOptions(handler.Reference()) + assert.NotNil(t, svcOpts) +} + +// Test Register with handler that has method config +func TestRegisterWithMethodConfig(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + handler := &mockRPCService{} + info := &common.ServiceInfo{ + InterfaceName: "com.example.TestService", + } + + err = srv.Register( + handler, + info, + WithInterface("com.example.TestService"), + WithGroup("test-group"), + WithVersion("1.0.0"), + ) + assert.NoError(t, err) + + svcOpts := srv.GetServiceOptions(handler.Reference()) + assert.NotNil(t, svcOpts) + if svcOpts != nil { + assert.Equal(t, "test-group", svcOpts.Service.Group) + assert.Equal(t, "1.0.0", svcOpts.Service.Version) + } +} + +// Test genSvcOpts with missing server config +func TestGenSvcOptsWithMissingServerConfig(t *testing.T) { + srv := &Server{ + cfg: nil, + } + + handler := &mockRPCService{} + _, err := srv.genSvcOpts(handler, nil) + assert.Error(t, err) + assert.Contains(t, err.Error(), "has not been initialized") +} + +// Test exportServices with empty service map +func TestExportServicesEmpty(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + err = srv.exportServices() + assert.NoError(t, err) +} + +// Test NewServer with invalid config +func TestNewServerWithInvalidConfig(t *testing.T) { + // This should not error during creation, but config validation might fail on use + srv, err := NewServer(WithServerGroup("test")) + assert.NoError(t, err) + assert.NotNil(t, srv) + assert.Equal(t, "test", srv.cfg.Provider.Group) Review Comment: The test is accessing unexported field `srv.cfg` to verify server options. While this is allowed within the same package, it would be better to verify configuration through observable behavior rather than accessing internal state. ```suggestion // Verify server group configuration via observable behavior: // register a service without an explicit group and ensure it inherits the server group. handler := &mockRPCService{} info := &common.ServiceInfo{ InterfaceName: "com.example.TestService", } err = srv.Register( handler, info, WithInterface("com.example.TestService"), ) assert.NoError(t, err) svcOpts := srv.GetServiceOptions(handler.Reference()) assert.NotNil(t, svcOpts) if svcOpts != nil { assert.Equal(t, "test", svcOpts.Service.Group) } ``` ########## server/server_test.go: ########## @@ -0,0 +1,477 @@ +/* + * 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 ( + "strconv" + "sync" + "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) + + // Verify server is properly initialized by using public API + // Try to register and retrieve a service to verify internal maps work + svcOpts := defaultServiceOptions() + svcOpts.Id = "init-test-service" + srv.registerServiceOptions(svcOpts) + + retrieved := srv.GetServiceOptions("init-test-service") + assert.NotNil(t, retrieved, "Server should have properly initialized service maps") +} + +// 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 public API methods + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, svcOpts, retrieved) + + retrievedByInterface := srv.GetServiceOptionsByInterfaceName("com.example.TestService") + assert.NotNil(t, retrievedByInterface) + assert.Equal(t, svcOpts, retrievedByInterface) +} + +// 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 retrievable by service ID but not by interface name + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, svcOpts, retrieved) + + // Empty interface should not be retrievable by interface name + retrievedByInterface := srv.GetServiceOptionsByInterfaceName("") + assert.Nil(t, retrievedByInterface) +} + +// Test SetProviderServices +func TestSetProviderServices(t *testing.T) { + // Preserve original internal services and restore after test + origInternalProServices := internalProServices + t.Cleanup(func() { + internalProServices = origInternalProServices + }) + // Lock and backup original state + internalProLock.Lock() + originalServices := internalProServices + internalProServices = make([]*InternalService, 0, 16) + + // Unlock before calling SetProviderServices (which needs the lock) + internalProLock.Unlock() + + // Register cleanup to restore original state + t.Cleanup(func() { + internalProLock.Lock() + defer internalProLock.Unlock() + internalProServices = originalServices + }) + + internalService := &InternalService{ + Name: "test-internal-service", + Priority: 0, + Init: func(options *ServiceOptions) (*ServiceDefinition, bool) { + return &ServiceDefinition{ + Handler: &mockRPCService{}, + Info: nil, + Opts: []ServiceOption{}, + }, true + }, + } + + // This function will acquire the lock internally + SetProviderServices(internalService) + + // Access the global variable safely with lock + internalProLock.Lock() + defer internalProLock.Unlock() + assert.Len(t, internalProServices, 1) + assert.Equal(t, "test-internal-service", internalProServices[0].Name) +} + +// Test SetProviderServices with empty name +func TestSetProviderServicesEmptyName(t *testing.T) { + // Lock and backup original state + internalProLock.Lock() + originalServices := internalProServices + internalProServices = make([]*InternalService, 0, 16) + + // Unlock before calling SetProviderServices (which needs the lock) + internalProLock.Unlock() + + // Register cleanup to restore original state + t.Cleanup(func() { + internalProLock.Lock() + defer internalProLock.Unlock() + internalProServices = originalServices + }) + + internalService := &InternalService{ + Name: "", + } + + // This function will acquire the lock internally + SetProviderServices(internalService) + + // Access the global variable safely with lock + internalProLock.Lock() + defer internalProLock.Unlock() + 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) Review Comment: The assertion on line 304 only checks that the swapped method name is different from the original, but doesn't verify the actual expected value. Consider adding a more specific assertion that checks the exact expected name (e.g., assert.Equal(t, "SayHello", result.Methods[1].Name)) to ensure the case-swapping behavior is working correctly. ########## server/server_test.go: ########## @@ -0,0 +1,477 @@ +/* + * 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 ( + "strconv" + "sync" + "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) + + // Verify server is properly initialized by using public API + // Try to register and retrieve a service to verify internal maps work + svcOpts := defaultServiceOptions() + svcOpts.Id = "init-test-service" + srv.registerServiceOptions(svcOpts) + + retrieved := srv.GetServiceOptions("init-test-service") + assert.NotNil(t, retrieved, "Server should have properly initialized service maps") +} + +// 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 public API methods + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, svcOpts, retrieved) + + retrievedByInterface := srv.GetServiceOptionsByInterfaceName("com.example.TestService") + assert.NotNil(t, retrievedByInterface) + assert.Equal(t, svcOpts, retrievedByInterface) +} + +// 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 retrievable by service ID but not by interface name + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, svcOpts, retrieved) + + // Empty interface should not be retrievable by interface name + retrievedByInterface := srv.GetServiceOptionsByInterfaceName("") + assert.Nil(t, retrievedByInterface) +} + +// Test SetProviderServices +func TestSetProviderServices(t *testing.T) { + // Preserve original internal services and restore after test + origInternalProServices := internalProServices + t.Cleanup(func() { + internalProServices = origInternalProServices + }) + // Lock and backup original state + internalProLock.Lock() + originalServices := internalProServices + internalProServices = make([]*InternalService, 0, 16) + + // Unlock before calling SetProviderServices (which needs the lock) + internalProLock.Unlock() + + // Register cleanup to restore original state + t.Cleanup(func() { + internalProLock.Lock() + defer internalProLock.Unlock() + internalProServices = originalServices + }) Review Comment: There is redundant cleanup logic in this test. Lines 210-213 preserve the original services in `origInternalProServices` and register a cleanup. Then lines 214-227 do essentially the same thing again with `originalServices`. The second cleanup (lines 223-227) will restore the state that was already modified by the test setup (line 217), rather than the true original state. You should remove the duplicate logic and keep only one cleanup approach. ########## server/server_test.go: ########## @@ -0,0 +1,477 @@ +/* + * 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 ( + "strconv" + "sync" + "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) + + // Verify server is properly initialized by using public API + // Try to register and retrieve a service to verify internal maps work + svcOpts := defaultServiceOptions() + svcOpts.Id = "init-test-service" + srv.registerServiceOptions(svcOpts) + + retrieved := srv.GetServiceOptions("init-test-service") + assert.NotNil(t, retrieved, "Server should have properly initialized service maps") +} + +// 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 public API methods + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, svcOpts, retrieved) + + retrievedByInterface := srv.GetServiceOptionsByInterfaceName("com.example.TestService") + assert.NotNil(t, retrievedByInterface) + assert.Equal(t, svcOpts, retrievedByInterface) +} + +// 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 retrievable by service ID but not by interface name + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, svcOpts, retrieved) + + // Empty interface should not be retrievable by interface name + retrievedByInterface := srv.GetServiceOptionsByInterfaceName("") + assert.Nil(t, retrievedByInterface) +} + +// Test SetProviderServices +func TestSetProviderServices(t *testing.T) { + // Preserve original internal services and restore after test + origInternalProServices := internalProServices + t.Cleanup(func() { + internalProServices = origInternalProServices + }) + // Lock and backup original state + internalProLock.Lock() + originalServices := internalProServices + internalProServices = make([]*InternalService, 0, 16) + + // Unlock before calling SetProviderServices (which needs the lock) + internalProLock.Unlock() + + // Register cleanup to restore original state + t.Cleanup(func() { + internalProLock.Lock() + defer internalProLock.Unlock() + internalProServices = originalServices + }) + + internalService := &InternalService{ + Name: "test-internal-service", + Priority: 0, + Init: func(options *ServiceOptions) (*ServiceDefinition, bool) { + return &ServiceDefinition{ + Handler: &mockRPCService{}, + Info: nil, + Opts: []ServiceOption{}, + }, true + }, + } + + // This function will acquire the lock internally + SetProviderServices(internalService) + + // Access the global variable safely with lock + internalProLock.Lock() + defer internalProLock.Unlock() + assert.Len(t, internalProServices, 1) + assert.Equal(t, "test-internal-service", internalProServices[0].Name) +} + +// Test SetProviderServices with empty name +func TestSetProviderServicesEmptyName(t *testing.T) { + // Lock and backup original state + internalProLock.Lock() + originalServices := internalProServices + internalProServices = make([]*InternalService, 0, 16) + + // Unlock before calling SetProviderServices (which needs the lock) + internalProLock.Unlock() + + // Register cleanup to restore original state + t.Cleanup(func() { + internalProLock.Lock() + defer internalProLock.Unlock() + internalProServices = originalServices + }) + + internalService := &InternalService{ + Name: "", + } + + // This function will acquire the lock internally + SetProviderServices(internalService) + + // Access the global variable safely with lock + internalProLock.Lock() + defer internalProLock.Unlock() + 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) + + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + svcOpts := defaultServiceOptions() + svcOpts.Id = "service-" + strconv.Itoa(idx) + srv.registerServiceOptions(svcOpts) + }(i) + } + + wg.Wait() + + // Verify all services were registered using public API + for i := 0; i < 10; i++ { + svcID := "service-" + strconv.Itoa(i) + retrieved := srv.GetServiceOptions(svcID) + assert.NotNil(t, retrieved, "Service %s should be registered", svcID) + assert.Equal(t, svcID, retrieved.Id) + } +} + +// Test Register with ServiceInfo +func TestRegisterWithServiceInfo(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + handler := &mockRPCService{} + info := &common.ServiceInfo{ + InterfaceName: "com.example.Service", + Methods: []common.MethodInfo{ + {Name: "Method1"}, + }, + } + + err = srv.Register(handler, info) + assert.NoError(t, err) + + // Service should be registered + svcOpts := srv.GetServiceOptions(handler.Reference()) + assert.NotNil(t, svcOpts) +} + +// Test RegisterService without ServiceInfo +func TestRegisterService(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + handler := &mockRPCService{} + err = srv.RegisterService(handler) + assert.NoError(t, err) + + // Service should be registered with handler reference name + svcOpts := srv.GetServiceOptions(handler.Reference()) + assert.NotNil(t, svcOpts) +} + +// Test Register with handler that has method config +func TestRegisterWithMethodConfig(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + handler := &mockRPCService{} + info := &common.ServiceInfo{ + InterfaceName: "com.example.TestService", + } + + err = srv.Register( + handler, + info, + WithInterface("com.example.TestService"), + WithGroup("test-group"), + WithVersion("1.0.0"), + ) + assert.NoError(t, err) + + svcOpts := srv.GetServiceOptions(handler.Reference()) + assert.NotNil(t, svcOpts) + if svcOpts != nil { + assert.Equal(t, "test-group", svcOpts.Service.Group) + assert.Equal(t, "1.0.0", svcOpts.Service.Version) + } +} + +// Test genSvcOpts with missing server config +func TestGenSvcOptsWithMissingServerConfig(t *testing.T) { + srv := &Server{ + cfg: nil, + } + + handler := &mockRPCService{} + _, err := srv.genSvcOpts(handler, nil) + assert.Error(t, err) + assert.Contains(t, err.Error(), "has not been initialized") +} + +// Test exportServices with empty service map +func TestExportServicesEmpty(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + err = srv.exportServices() + assert.NoError(t, err) +} + +// Test NewServer with invalid config +func TestNewServerWithInvalidConfig(t *testing.T) { + // This should not error during creation, but config validation might fail on use + srv, err := NewServer(WithServerGroup("test")) + assert.NoError(t, err) + assert.NotNil(t, srv) + assert.Equal(t, "test", srv.cfg.Provider.Group) +} Review Comment: The test name "TestNewServerWithInvalidConfig" and the comment on line 472 are misleading. The test is actually using a valid configuration (WithServerGroup is a legitimate option). Consider renaming the test to something like "TestNewServerWithCustomGroup" or "TestNewServerWithServerOption" to accurately reflect what it's testing. ########## server/server_test.go: ########## @@ -0,0 +1,477 @@ +/* + * 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 ( + "strconv" + "sync" + "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) + + // Verify server is properly initialized by using public API + // Try to register and retrieve a service to verify internal maps work + svcOpts := defaultServiceOptions() + svcOpts.Id = "init-test-service" + srv.registerServiceOptions(svcOpts) + + retrieved := srv.GetServiceOptions("init-test-service") + assert.NotNil(t, retrieved, "Server should have properly initialized service maps") +} + +// 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 public API methods + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, svcOpts, retrieved) + + retrievedByInterface := srv.GetServiceOptionsByInterfaceName("com.example.TestService") + assert.NotNil(t, retrievedByInterface) + assert.Equal(t, svcOpts, retrievedByInterface) +} + +// 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 retrievable by service ID but not by interface name + retrieved := srv.GetServiceOptions("test-service") + assert.NotNil(t, retrieved) + assert.Equal(t, svcOpts, retrieved) + + // Empty interface should not be retrievable by interface name + retrievedByInterface := srv.GetServiceOptionsByInterfaceName("") + assert.Nil(t, retrievedByInterface) +} + +// Test SetProviderServices +func TestSetProviderServices(t *testing.T) { + // Preserve original internal services and restore after test + origInternalProServices := internalProServices + t.Cleanup(func() { + internalProServices = origInternalProServices + }) + // Lock and backup original state + internalProLock.Lock() + originalServices := internalProServices + internalProServices = make([]*InternalService, 0, 16) + + // Unlock before calling SetProviderServices (which needs the lock) + internalProLock.Unlock() + + // Register cleanup to restore original state + t.Cleanup(func() { + internalProLock.Lock() + defer internalProLock.Unlock() + internalProServices = originalServices + }) + + internalService := &InternalService{ + Name: "test-internal-service", + Priority: 0, + Init: func(options *ServiceOptions) (*ServiceDefinition, bool) { + return &ServiceDefinition{ + Handler: &mockRPCService{}, + Info: nil, + Opts: []ServiceOption{}, + }, true + }, + } + + // This function will acquire the lock internally + SetProviderServices(internalService) + + // Access the global variable safely with lock + internalProLock.Lock() + defer internalProLock.Unlock() + assert.Len(t, internalProServices, 1) + assert.Equal(t, "test-internal-service", internalProServices[0].Name) +} + +// Test SetProviderServices with empty name +func TestSetProviderServicesEmptyName(t *testing.T) { + // Lock and backup original state + internalProLock.Lock() + originalServices := internalProServices + internalProServices = make([]*InternalService, 0, 16) + + // Unlock before calling SetProviderServices (which needs the lock) + internalProLock.Unlock() + + // Register cleanup to restore original state + t.Cleanup(func() { + internalProLock.Lock() + defer internalProLock.Unlock() + internalProServices = originalServices + }) + + internalService := &InternalService{ + Name: "", + } + + // This function will acquire the lock internally + SetProviderServices(internalService) + + // Access the global variable safely with lock + internalProLock.Lock() + defer internalProLock.Unlock() + 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) + + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + svcOpts := defaultServiceOptions() + svcOpts.Id = "service-" + strconv.Itoa(idx) + srv.registerServiceOptions(svcOpts) + }(i) + } + + wg.Wait() + + // Verify all services were registered using public API + for i := 0; i < 10; i++ { + svcID := "service-" + strconv.Itoa(i) + retrieved := srv.GetServiceOptions(svcID) + assert.NotNil(t, retrieved, "Service %s should be registered", svcID) + assert.Equal(t, svcID, retrieved.Id) + } +} + +// Test Register with ServiceInfo +func TestRegisterWithServiceInfo(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + handler := &mockRPCService{} + info := &common.ServiceInfo{ + InterfaceName: "com.example.Service", + Methods: []common.MethodInfo{ + {Name: "Method1"}, + }, + } + + err = srv.Register(handler, info) + assert.NoError(t, err) + + // Service should be registered + svcOpts := srv.GetServiceOptions(handler.Reference()) + assert.NotNil(t, svcOpts) +} + +// Test RegisterService without ServiceInfo +func TestRegisterService(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + handler := &mockRPCService{} + err = srv.RegisterService(handler) + assert.NoError(t, err) + + // Service should be registered with handler reference name + svcOpts := srv.GetServiceOptions(handler.Reference()) + assert.NotNil(t, svcOpts) +} + +// Test Register with handler that has method config +func TestRegisterWithMethodConfig(t *testing.T) { + srv, err := NewServer() + assert.NoError(t, err) + + handler := &mockRPCService{} + info := &common.ServiceInfo{ + InterfaceName: "com.example.TestService", + } + + err = srv.Register( + handler, + info, + WithInterface("com.example.TestService"), + WithGroup("test-group"), + WithVersion("1.0.0"), + ) + assert.NoError(t, err) + + svcOpts := srv.GetServiceOptions(handler.Reference()) + assert.NotNil(t, svcOpts) + if svcOpts != nil { + assert.Equal(t, "test-group", svcOpts.Service.Group) + assert.Equal(t, "1.0.0", svcOpts.Service.Version) + } Review Comment: The if condition on line 443 is redundant. Line 442 already asserts that svcOpts is not nil. If that assertion fails, the test will stop. If it passes, svcOpts is guaranteed to be non-nil, making the if statement unnecessary. You can safely remove the if condition and keep only the assertions inside. ```suggestion assert.Equal(t, "test-group", svcOpts.Service.Group) assert.Equal(t, "1.0.0", svcOpts.Service.Version) ``` ########## server/server_test.go: ########## @@ -0,0 +1,477 @@ +/* + * 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 ( + "strconv" + "sync" + "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) + + // Verify server is properly initialized by using public API + // Try to register and retrieve a service to verify internal maps work + svcOpts := defaultServiceOptions() + svcOpts.Id = "init-test-service" + srv.registerServiceOptions(svcOpts) + + retrieved := srv.GetServiceOptions("init-test-service") + assert.NotNil(t, retrieved, "Server should have properly initialized service maps") +} + +// 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) Review Comment: The test is accessing unexported field `srv.cfg` directly to verify the configuration. While this is allowed within the same package, it would be better to verify configuration through observable behavior rather than accessing internal state, especially since this is testing user-facing functionality. ```suggestion // Note: configuration is intentionally not validated via internal fields (srv.cfg) // to avoid coupling tests to implementation details. Further validation should // be done via exported behavior once available. ``` -- 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]
