Copilot commented on code in PR #3120: URL: https://github.com/apache/dubbo-go/pull/3120#discussion_r2648992599
########## remoting/zookeeper/curator_discovery/service_discovery_test.go: ########## @@ -0,0 +1,201 @@ +/* + * 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 curator_discovery + +import ( + "sync" + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/remoting" +) + +const ( + testBasePath = "/dubbo/services" + testServiceName = "test-service" + testInstanceID = "instance-1" +) + +func TestEntry(t *testing.T) { + t.Run("basic operations", func(t *testing.T) { + entry := &Entry{} + assert.Nil(t, entry.instance) + + entry.Lock() + entry.instance = &ServiceInstance{Name: testServiceName} + entry.Unlock() + assert.Equal(t, testServiceName, entry.instance.Name) + }) + + t.Run("concurrent access", func(t *testing.T) { + entry := &Entry{instance: &ServiceInstance{Name: testServiceName}} + var wg sync.WaitGroup + + for i := 0; i < 100; i++ { + wg.Add(1) + go func() { + defer wg.Done() + entry.Lock() + entry.instance.Name = "updated" + entry.Unlock() + }() + } + wg.Wait() + assert.Equal(t, "updated", entry.instance.Name) + }) +} + +func TestNewServiceDiscovery(t *testing.T) { + for _, basePath := range []string{testBasePath, "/", ""} { + sd := NewServiceDiscovery(nil, basePath) + assert.NotNil(t, sd) + assert.Equal(t, basePath, sd.basePath) + assert.NotNil(t, sd.mutex) + assert.NotNil(t, sd.services) + } +} + +func TestServiceDiscoveryPathForInstance(t *testing.T) { + tests := []struct { + basePath, serviceName, instanceID, expected string + }{ + {testBasePath, testServiceName, testInstanceID, testBasePath + "/" + testServiceName + "/" + testInstanceID}, + {"/", "service", "id", "/service/id"}, + {"", "service", "id", "service/id"}, + } + + for _, tt := range tests { + sd := NewServiceDiscovery(nil, tt.basePath) + assert.Equal(t, tt.expected, sd.pathForInstance(tt.serviceName, tt.instanceID)) + } +} + +func TestServiceDiscoveryPathForName(t *testing.T) { + tests := []struct { + basePath, serviceName, expected string + }{ + {testBasePath, testServiceName, testBasePath + "/" + testServiceName}, + {"/", "service", "/service"}, + {"", "service", "service"}, + } + + for _, tt := range tests { + sd := NewServiceDiscovery(nil, tt.basePath) + assert.Equal(t, tt.expected, sd.pathForName(tt.serviceName)) + } +} + +func TestServiceDiscoveryGetNameAndID(t *testing.T) { + tests := []struct { + name, basePath, path, expectedName, expectedID string + wantErr bool + }{ + {"valid path", testBasePath, testBasePath + "/" + testServiceName + "/" + testInstanceID, testServiceName, testInstanceID, false}, + {"missing id", "/dubbo", "/dubbo/service", "", "", true}, + {"empty path", "/dubbo", "", "", "", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sd := NewServiceDiscovery(nil, tt.basePath) + name, id, err := sd.getNameAndID(tt.path) + if tt.wantErr { + assert.NotNil(t, err) + } else { + assert.Nil(t, err) + assert.Equal(t, tt.expectedName, name) + assert.Equal(t, tt.expectedID, id) + } + }) + } +} + +func TestServiceDiscoveryDataChange(t *testing.T) { + sd := NewServiceDiscovery(nil, testBasePath) + testPath := testBasePath + "/test/" + testInstanceID + + for _, eventType := range []remoting.EventType{remoting.EventTypeAdd, remoting.EventTypeUpdate, remoting.EventTypeDel} { + event := remoting.Event{Path: testPath, Action: eventType, Content: "content"} + assert.True(t, sd.DataChange(event)) + } + + // Invalid path + assert.True(t, sd.DataChange(remoting.Event{Path: testBasePath + "/only-name"})) +} + +func TestServiceDiscoveryClose(t *testing.T) { + sd := &ServiceDiscovery{client: nil, listener: nil, services: &sync.Map{}, mutex: &sync.Mutex{}} + sd.Close() // Should not panic +} + +func TestServiceDiscoveryServicesMap(t *testing.T) { + sd := NewServiceDiscovery(nil, testBasePath) + + entry := &Entry{instance: &ServiceInstance{Name: testServiceName, ID: testInstanceID}} + sd.services.Store(testInstanceID, entry) + + value, ok := sd.services.Load(testInstanceID) + assert.True(t, ok) + assert.Equal(t, testServiceName, value.(*Entry).instance.Name) + + sd.services.Delete(testInstanceID) + _, ok = sd.services.Load(testInstanceID) + assert.False(t, ok) +} + +func TestServiceDiscoveryUpdateService(t *testing.T) { + sd := NewServiceDiscovery(nil, testBasePath) + + // Update non-existent + err := sd.UpdateService(&ServiceInstance{Name: testServiceName, ID: "non-existent"}) + assert.NotNil(t, err) + assert.Contains(t, err.Error(), "not registered") + + // Update with invalid entry type + sd.services.Store("invalid-id", "not-an-entry") + err = sd.UpdateService(&ServiceInstance{Name: testServiceName, ID: "invalid-id"}) + assert.NotNil(t, err) + assert.Contains(t, err.Error(), "not entry") +} + +func TestServiceDiscoveryUnregisterService(t *testing.T) { + sd := NewServiceDiscovery(nil, testBasePath) + + err := sd.UnregisterService(&ServiceInstance{Name: testServiceName, ID: "non-existent"}) + assert.Nil(t, err) +} + +func TestServiceDiscoveryConcurrentAccess(t *testing.T) { + sd := NewServiceDiscovery(nil, testBasePath) + var wg sync.WaitGroup + + for i := 0; i < 100; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + entry := &Entry{instance: &ServiceInstance{Name: testServiceName, ID: string(rune('a' + idx%26))}} Review Comment: Using string(rune('a' + idx%26)) to generate IDs is problematic for concurrent testing. Multiple goroutines with different idx values that have the same idx%26 result will generate the same ID, causing potential conflicts in the concurrent map operations. Consider using a more unique identifier such as fmt.Sprintf("id-%d", idx) to ensure each goroutine has a unique ID. ########## remoting/getty/pool_test.go: ########## @@ -0,0 +1,133 @@ +/* + * 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 getty + +import ( + "sync" + "sync/atomic" + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +func TestGettyRPCClientUpdateActive(t *testing.T) { + client := &gettyRPCClient{} + client.updateActive(1234567890) + assert.Equal(t, int64(1234567890), atomic.LoadInt64(&client.active)) + + client.updateActive(0) + assert.Equal(t, int64(0), atomic.LoadInt64(&client.active)) +} + +func TestGettyRPCClientSelectSession(t *testing.T) { + client := &gettyRPCClient{sessions: nil} + assert.Nil(t, client.selectSession()) + + client.sessions = []*rpcSession{} + assert.Nil(t, client.selectSession()) +} + +func TestGettyRPCClientSessionOperations(t *testing.T) { + client := &gettyRPCClient{} + + // Remove/update nil session should not panic + client.removeSession(nil) + client.updateSession(nil) + + // Get from nil sessions + _, err := client.getClientRpcSession(nil) + assert.Equal(t, errClientClosed, err) + + // Session not found + client.sessions = []*rpcSession{} + _, err = client.getClientRpcSession(nil) + assert.Contains(t, err.Error(), "session not exist") +} + +func TestGettyRPCClientIsAvailable(t *testing.T) { + client := &gettyRPCClient{sessions: nil} + assert.False(t, client.isAvailable()) + + client.sessions = []*rpcSession{} + assert.False(t, client.isAvailable()) +} + +func TestGettyRPCClientClose(t *testing.T) { + client := &gettyRPCClient{sessions: []*rpcSession{}} + assert.Nil(t, client.close()) + assert.NotNil(t, client.close()) // Second close returns error Review Comment: Calling close() twice on the same client may not properly test the intended behavior. The second close() call is expected to return an error, but the test doesn't verify what specific error is returned. Consider asserting the specific error type or message to ensure the function correctly handles double-close scenarios. ########## remoting/etcdv3/client_test.go: ########## @@ -0,0 +1,71 @@ +/* + * 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 etcdv3 + +import ( + "sync" + "testing" + "time" +) + +import ( + gxetcd "github.com/dubbogo/gost/database/kv/etcd/v3" + + "github.com/stretchr/testify/assert" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common" +) + +type mockClientFacade struct { + client *gxetcd.Client + lock sync.Mutex + url *common.URL +} + +func (m *mockClientFacade) Client() *gxetcd.Client { return m.client } +func (m *mockClientFacade) SetClient(c *gxetcd.Client) { m.client = c } +func (m *mockClientFacade) ClientLock() *sync.Mutex { return &m.lock } +func (m *mockClientFacade) WaitGroup() *sync.WaitGroup { return &sync.WaitGroup{} } +func (m *mockClientFacade) Done() chan struct{} { return make(chan struct{}) } +func (m *mockClientFacade) RestartCallBack() bool { return true } +func (m *mockClientFacade) GetURL() *common.URL { return m.url } +func (m *mockClientFacade) IsAvailable() bool { return true } +func (m *mockClientFacade) Destroy() {} Review Comment: The Done channel is created on each call rather than being stored and reused. This means different calls to Done() will return different channels, which could lead to synchronization issues in actual usage. The mock should store a single channel instance and return it consistently. ```suggestion wg sync.WaitGroup done chan struct{} } func (m *mockClientFacade) Client() *gxetcd.Client { return m.client } func (m *mockClientFacade) SetClient(c *gxetcd.Client) { m.client = c } func (m *mockClientFacade) ClientLock() *sync.Mutex { return &m.lock } func (m *mockClientFacade) WaitGroup() *sync.WaitGroup { return &m.wg } func (m *mockClientFacade) Done() chan struct{} { m.lock.Lock() defer m.lock.Unlock() if m.done == nil { m.done = make(chan struct{}) } return m.done } func (m *mockClientFacade) RestartCallBack() bool { return true } func (m *mockClientFacade) GetURL() *common.URL { return m.url } func (m *mockClientFacade) IsAvailable() bool { return true } func (m *mockClientFacade) Destroy() {} ``` ########## remoting/zookeeper/curator_discovery/service_discovery_test.go: ########## @@ -0,0 +1,201 @@ +/* + * 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 curator_discovery + +import ( + "sync" + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/remoting" +) + +const ( + testBasePath = "/dubbo/services" + testServiceName = "test-service" + testInstanceID = "instance-1" +) + +func TestEntry(t *testing.T) { + t.Run("basic operations", func(t *testing.T) { Review Comment: The test name "basic operations" is too generic and doesn't describe what specific operations are being tested. Consider renaming to something more descriptive like "test entry lock and instance access" to better document what the test verifies. ```suggestion t.Run("entry lock and instance access", func(t *testing.T) { ``` ########## remoting/zookeeper/client_test.go: ########## @@ -0,0 +1,103 @@ +/* + * 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 zookeeper + +import ( + "sync" + "testing" + "time" +) + +import ( + gxzookeeper "github.com/dubbogo/gost/database/kv/zk" + + "github.com/stretchr/testify/assert" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common" +) + +func TestConstants(t *testing.T) { + assert.Equal(t, 3, ConnDelay) + assert.Equal(t, 3, MaxFailTimes) +} + +// Mock for ValidateZookeeperClient test +type mockZkClientFacade struct { + client *gxzookeeper.ZookeeperClient + lock sync.Mutex + url *common.URL +} + +func (m *mockZkClientFacade) ZkClient() *gxzookeeper.ZookeeperClient { return m.client } +func (m *mockZkClientFacade) SetZkClient(c *gxzookeeper.ZookeeperClient) { m.client = c } +func (m *mockZkClientFacade) ZkClientLock() *sync.Mutex { return &m.lock } +func (m *mockZkClientFacade) WaitGroup() *sync.WaitGroup { return &sync.WaitGroup{} } +func (m *mockZkClientFacade) Done() chan struct{} { return make(chan struct{}) } +func (m *mockZkClientFacade) RestartCallBack() bool { return true } +func (m *mockZkClientFacade) GetURL() *common.URL { return m.url } + Review Comment: The Done channel is created in each call to Done() rather than being reused. This means different calls to Done() will return different channels, which could lead to incorrect synchronization behavior. The test should verify that Done() returns a consistent channel or the mock should store and reuse a single channel instance. ```suggestion done chan struct{} } func (m *mockZkClientFacade) ZkClient() *gxzookeeper.ZookeeperClient { return m.client } func (m *mockZkClientFacade) SetZkClient(c *gxzookeeper.ZookeeperClient) { m.client = c } func (m *mockZkClientFacade) ZkClientLock() *sync.Mutex { return &m.lock } func (m *mockZkClientFacade) WaitGroup() *sync.WaitGroup { return &sync.WaitGroup{} } func (m *mockZkClientFacade) Done() chan struct{} { m.lock.Lock() defer m.lock.Unlock() if m.done == nil { m.done = make(chan struct{}) } return m.done } func (m *mockZkClientFacade) RestartCallBack() bool { return true } func (m *mockZkClientFacade) GetURL() *common.URL { return m.url } ``` ########## remoting/zookeeper/client_test.go: ########## @@ -0,0 +1,103 @@ +/* + * 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 zookeeper + +import ( + "sync" + "testing" + "time" +) + +import ( + gxzookeeper "github.com/dubbogo/gost/database/kv/zk" + + "github.com/stretchr/testify/assert" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common" +) + +func TestConstants(t *testing.T) { + assert.Equal(t, 3, ConnDelay) + assert.Equal(t, 3, MaxFailTimes) +} + +// Mock for ValidateZookeeperClient test +type mockZkClientFacade struct { + client *gxzookeeper.ZookeeperClient + lock sync.Mutex + url *common.URL +} + +func (m *mockZkClientFacade) ZkClient() *gxzookeeper.ZookeeperClient { return m.client } +func (m *mockZkClientFacade) SetZkClient(c *gxzookeeper.ZookeeperClient) { m.client = c } +func (m *mockZkClientFacade) ZkClientLock() *sync.Mutex { return &m.lock } +func (m *mockZkClientFacade) WaitGroup() *sync.WaitGroup { return &sync.WaitGroup{} } +func (m *mockZkClientFacade) Done() chan struct{} { return make(chan struct{}) } +func (m *mockZkClientFacade) RestartCallBack() bool { return true } +func (m *mockZkClientFacade) GetURL() *common.URL { return m.url } + +func TestValidateZookeeperClient(t *testing.T) { + // Test with invalid address (will fail to connect but exercises the code path) + facade := &mockZkClientFacade{ + url: common.NewURLWithOptions( + common.WithParamsValue("config.timeout", "100ms"), + ), + } + + err := ValidateZookeeperClient(facade, "test") + assert.NotNil(t, err) // Expected to fail without real zk + + // Test with existing client (should skip creation) + facade2 := &mockZkClientFacade{ + client: &gxzookeeper.ZookeeperClient{}, + url: common.NewURLWithOptions(), + } + err = ValidateZookeeperClient(facade2, "test") Review Comment: The test comment states "will fail without real zk" but the variable name uses "facade2" which is not descriptive. Consider renaming to something like "facadeWithExistingClient" to better document the test scenario being verified. ```suggestion facadeWithExistingClient := &mockZkClientFacade{ client: &gxzookeeper.ZookeeperClient{}, url: common.NewURLWithOptions(), } err = ValidateZookeeperClient(facadeWithExistingClient, "test") ``` -- 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]
