Copilot commented on code in PR #3128: URL: https://github.com/apache/dubbo-go/pull/3128#discussion_r2638200607
########## graceful_shutdown/shutdown_test.go: ########## @@ -0,0 +1,192 @@ +/* + * 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 graceful_shutdown + +import ( + "context" + "sync" + "testing" + "time" +) + +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common/constant" + "dubbo.apache.org/dubbo-go/v3/common/extension" + "dubbo.apache.org/dubbo-go/v3/filter" + "dubbo.apache.org/dubbo-go/v3/global" + "dubbo.apache.org/dubbo-go/v3/protocol/base" + "dubbo.apache.org/dubbo-go/v3/protocol/result" +) + +// MockFilter implements filter.Filter and config.Setter for testing +type MockFilter struct { + mock.Mock +} + +func (m *MockFilter) Set(key string, value any) { + m.Called(key, value) +} + +func (m *MockFilter) Invoke(ctx context.Context, invoker base.Invoker, invocation base.Invocation) result.Result { + return nil +} + +func (m *MockFilter) OnResponse(ctx context.Context, result result.Result, invoker base.Invoker, invocation base.Invocation) result.Result { + return nil +} + +func TestInit(t *testing.T) { + // Reset initOnce and protocols for testing + initOnce = sync.Once{} Review Comment: Resetting the global variable initOnce using sync.Once{} can lead to race conditions if tests run in parallel. The initOnce variable is used by the Init function to ensure it only runs once across the entire application lifecycle. A better approach would be to avoid resetting this variable or to ensure tests that modify global state are properly isolated and not run in parallel using t.Parallel(). ########## graceful_shutdown/shutdown_test.go: ########## @@ -0,0 +1,192 @@ +/* + * 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 graceful_shutdown + +import ( + "context" + "sync" + "testing" + "time" +) + +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common/constant" + "dubbo.apache.org/dubbo-go/v3/common/extension" + "dubbo.apache.org/dubbo-go/v3/filter" + "dubbo.apache.org/dubbo-go/v3/global" + "dubbo.apache.org/dubbo-go/v3/protocol/base" + "dubbo.apache.org/dubbo-go/v3/protocol/result" +) + +// MockFilter implements filter.Filter and config.Setter for testing +type MockFilter struct { + mock.Mock +} + +func (m *MockFilter) Set(key string, value any) { + m.Called(key, value) +} + +func (m *MockFilter) Invoke(ctx context.Context, invoker base.Invoker, invocation base.Invocation) result.Result { + return nil +} + +func (m *MockFilter) OnResponse(ctx context.Context, result result.Result, invoker base.Invoker, invocation base.Invocation) result.Result { + return nil +} + +func TestInit(t *testing.T) { + // Reset initOnce and protocols for testing + initOnce = sync.Once{} + protocols = nil + proMu = sync.Mutex{} + + // Register mock filters + mockConsumerFilter := &MockFilter{} + mockProviderFilter := &MockFilter{} + + // Expect Set method calls + mockConsumerFilter.On("Set", mock.Anything, mock.Anything).Return() + mockProviderFilter.On("Set", mock.Anything, mock.Anything).Return() + + // Register mock filters + extension.SetFilter(constant.GracefulShutdownConsumerFilterKey, func() filter.Filter { + return mockConsumerFilter + }) + extension.SetFilter(constant.GracefulShutdownProviderFilterKey, func() filter.Filter { + return mockProviderFilter + }) + + // Test with default options + Init() + + // Test with custom options + customTimeout := 120 * time.Second + Init(WithTimeout(customTimeout)) + + // Remove mock filters + extension.UnregisterFilter(constant.GracefulShutdownConsumerFilterKey) + extension.UnregisterFilter(constant.GracefulShutdownProviderFilterKey) +} Review Comment: The test doesn't verify that Init was actually called or that the filters were properly configured. While the mock expectations are set up, there are no assertions to verify that the mock methods were actually called with the expected parameters. Consider adding assertions like mockConsumerFilter.AssertExpectations(t) and mockProviderFilter.AssertExpectations(t) after the Init calls. ########## graceful_shutdown/shutdown_test.go: ########## @@ -0,0 +1,192 @@ +/* + * 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 graceful_shutdown + +import ( + "context" + "sync" + "testing" + "time" +) + +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common/constant" + "dubbo.apache.org/dubbo-go/v3/common/extension" + "dubbo.apache.org/dubbo-go/v3/filter" + "dubbo.apache.org/dubbo-go/v3/global" + "dubbo.apache.org/dubbo-go/v3/protocol/base" + "dubbo.apache.org/dubbo-go/v3/protocol/result" +) + +// MockFilter implements filter.Filter and config.Setter for testing +type MockFilter struct { + mock.Mock +} + +func (m *MockFilter) Set(key string, value any) { + m.Called(key, value) +} + +func (m *MockFilter) Invoke(ctx context.Context, invoker base.Invoker, invocation base.Invocation) result.Result { + return nil +} + +func (m *MockFilter) OnResponse(ctx context.Context, result result.Result, invoker base.Invoker, invocation base.Invocation) result.Result { + return nil +} + +func TestInit(t *testing.T) { + // Reset initOnce and protocols for testing + initOnce = sync.Once{} + protocols = nil + proMu = sync.Mutex{} + + // Register mock filters + mockConsumerFilter := &MockFilter{} + mockProviderFilter := &MockFilter{} + + // Expect Set method calls + mockConsumerFilter.On("Set", mock.Anything, mock.Anything).Return() + mockProviderFilter.On("Set", mock.Anything, mock.Anything).Return() + + // Register mock filters + extension.SetFilter(constant.GracefulShutdownConsumerFilterKey, func() filter.Filter { + return mockConsumerFilter + }) + extension.SetFilter(constant.GracefulShutdownProviderFilterKey, func() filter.Filter { + return mockProviderFilter + }) + + // Test with default options + Init() + + // Test with custom options + customTimeout := 120 * time.Second + Init(WithTimeout(customTimeout)) + + // Remove mock filters + extension.UnregisterFilter(constant.GracefulShutdownConsumerFilterKey) + extension.UnregisterFilter(constant.GracefulShutdownProviderFilterKey) +} + +func TestRegisterProtocol(t *testing.T) { + // Reset protocols for testing + protocols = make(map[string]struct{}) + proMu = sync.Mutex{} Review Comment: The test is resetting global state (protocols, proMu) which can cause race conditions if tests run in parallel. This test modifies shared global state without proper isolation. Consider adding a cleanup function using t.Cleanup() to restore the original state after the test, or ensure this test doesn't run in parallel with other tests. ########## graceful_shutdown/shutdown_test.go: ########## @@ -0,0 +1,192 @@ +/* + * 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 graceful_shutdown + +import ( + "context" + "sync" + "testing" + "time" +) + +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common/constant" + "dubbo.apache.org/dubbo-go/v3/common/extension" + "dubbo.apache.org/dubbo-go/v3/filter" + "dubbo.apache.org/dubbo-go/v3/global" + "dubbo.apache.org/dubbo-go/v3/protocol/base" + "dubbo.apache.org/dubbo-go/v3/protocol/result" +) + +// MockFilter implements filter.Filter and config.Setter for testing +type MockFilter struct { + mock.Mock +} + +func (m *MockFilter) Set(key string, value any) { + m.Called(key, value) +} + +func (m *MockFilter) Invoke(ctx context.Context, invoker base.Invoker, invocation base.Invocation) result.Result { + return nil +} + +func (m *MockFilter) OnResponse(ctx context.Context, result result.Result, invoker base.Invoker, invocation base.Invocation) result.Result { + return nil +} + +func TestInit(t *testing.T) { + // Reset initOnce and protocols for testing + initOnce = sync.Once{} + protocols = nil + proMu = sync.Mutex{} + + // Register mock filters + mockConsumerFilter := &MockFilter{} + mockProviderFilter := &MockFilter{} + + // Expect Set method calls + mockConsumerFilter.On("Set", mock.Anything, mock.Anything).Return() + mockProviderFilter.On("Set", mock.Anything, mock.Anything).Return() + + // Register mock filters + extension.SetFilter(constant.GracefulShutdownConsumerFilterKey, func() filter.Filter { + return mockConsumerFilter + }) + extension.SetFilter(constant.GracefulShutdownProviderFilterKey, func() filter.Filter { + return mockProviderFilter + }) + + // Test with default options + Init() + + // Test with custom options + customTimeout := 120 * time.Second + Init(WithTimeout(customTimeout)) + + // Remove mock filters + extension.UnregisterFilter(constant.GracefulShutdownConsumerFilterKey) + extension.UnregisterFilter(constant.GracefulShutdownProviderFilterKey) +} + +func TestRegisterProtocol(t *testing.T) { + // Reset protocols for testing + protocols = make(map[string]struct{}) + proMu = sync.Mutex{} + + // Register some protocols + RegisterProtocol("dubbo") + RegisterProtocol("rest") + RegisterProtocol("tri") + + // Check if protocols are registered correctly + proMu.Lock() + defer proMu.Unlock() + + assert.Contains(t, protocols, "dubbo") + assert.Contains(t, protocols, "rest") + assert.Contains(t, protocols, "tri") + assert.Len(t, protocols, 3) +} + +func TestTotalTimeout(t *testing.T) { + // Test with default timeout + config := global.DefaultShutdownConfig() + timeout := totalTimeout(config) + assert.Equal(t, defaultTimeout, timeout) + + // Test with custom timeout + config.Timeout = "120s" + timeout = totalTimeout(config) + assert.Equal(t, 120*time.Second, timeout) + + // Test with invalid timeout + config.Timeout = "invalid" + timeout = totalTimeout(config) + assert.Equal(t, defaultTimeout, timeout) + + // Test with timeout less than default + config.Timeout = "30s" + timeout = totalTimeout(config) + assert.Equal(t, defaultTimeout, timeout) // Should use default if less than default +} + +func TestParseDuration(t *testing.T) { + // Test with valid duration + res := parseDuration("10s", "test", 5*time.Second) + assert.Equal(t, 10*time.Second, res) + + // Test with invalid duration + res = parseDuration("invalid", "test", 5*time.Second) + assert.Equal(t, 5*time.Second, res) + + // Test with empty string + res = parseDuration("", "test", 5*time.Second) + assert.Equal(t, 5*time.Second, res) +} + +func TestWaitAndAcceptNewRequests(t *testing.T) { + // Test with positive step timeout + config := global.DefaultShutdownConfig() + config.StepTimeout = "100ms" + config.ProviderActiveCount.Store(0) + + start := time.Now() + waitAndAcceptNewRequests(config) + elapsed := time.Since(start) + + // Should wait for ConsumerUpdateWaitTime (default 3s) plus a little extra for processing + assert.GreaterOrEqual(t, elapsed, 3*time.Second) + + // Test with negative step timeout (should skip waiting) + config.StepTimeout = "-1s" + start = time.Now() + waitAndAcceptNewRequests(config) + elapsed = time.Since(start) + + // Should only wait for ConsumerUpdateWaitTime + assert.Less(t, elapsed, 3*time.Second+100*time.Millisecond) Review Comment: The comment states "Should only wait for ConsumerUpdateWaitTime" which is accurate. However, the ConsumerUpdateWaitTime defaults to 3s, so the elapsed time should be at least 3s, not less than 3s + 100ms. The current assertion is checking the wrong condition - it's checking that the time is less than a threshold when it should be checking that it's at least 3s (since ConsumerUpdateWaitTime sleep happens unconditionally before the stepTimeout check). ```suggestion // Test with negative step timeout (should skip additional waiting beyond ConsumerUpdateWaitTime) config.StepTimeout = "-1s" start = time.Now() waitAndAcceptNewRequests(config) elapsed = time.Since(start) // Should wait at least for ConsumerUpdateWaitTime (default 3s) assert.GreaterOrEqual(t, elapsed, 3*time.Second) ``` ########## graceful_shutdown/options_test.go: ########## @@ -0,0 +1,102 @@ +/* + * 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 graceful_shutdown + +import ( + "testing" + "time" +) + +import ( + "github.com/stretchr/testify/assert" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/global" +) + +func TestDefaultOptions(t *testing.T) { + opts := defaultOptions() + assert.NotNil(t, opts) + assert.NotNil(t, opts.Shutdown) + assert.Equal(t, "60s", opts.Shutdown.Timeout) + assert.Equal(t, "3s", opts.Shutdown.StepTimeout) + assert.Equal(t, "3s", opts.Shutdown.ConsumerUpdateWaitTime) + assert.Equal(t, "", opts.Shutdown.OfflineRequestWindowTimeout) // No default value Review Comment: The comment says "No default value" but according to the DefaultShutdownConfig() function, OfflineRequestWindowTimeout has no default tag in the struct definition, meaning it will be an empty string by default. This comment is accurate but could be clearer by stating "Empty string by default" or "No default timeout configured" to better describe the actual default value rather than implying there's no value at all. ```suggestion assert.Equal(t, "", opts.Shutdown.OfflineRequestWindowTimeout) // Empty string by default (no timeout configured) ``` ########## graceful_shutdown/shutdown_test.go: ########## @@ -0,0 +1,192 @@ +/* + * 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 graceful_shutdown + +import ( + "context" + "sync" + "testing" + "time" +) + +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common/constant" + "dubbo.apache.org/dubbo-go/v3/common/extension" + "dubbo.apache.org/dubbo-go/v3/filter" + "dubbo.apache.org/dubbo-go/v3/global" + "dubbo.apache.org/dubbo-go/v3/protocol/base" + "dubbo.apache.org/dubbo-go/v3/protocol/result" +) + +// MockFilter implements filter.Filter and config.Setter for testing +type MockFilter struct { + mock.Mock +} + +func (m *MockFilter) Set(key string, value any) { + m.Called(key, value) +} + +func (m *MockFilter) Invoke(ctx context.Context, invoker base.Invoker, invocation base.Invocation) result.Result { + return nil +} + +func (m *MockFilter) OnResponse(ctx context.Context, result result.Result, invoker base.Invoker, invocation base.Invocation) result.Result { + return nil +} + +func TestInit(t *testing.T) { + // Reset initOnce and protocols for testing + initOnce = sync.Once{} + protocols = nil + proMu = sync.Mutex{} + + // Register mock filters + mockConsumerFilter := &MockFilter{} + mockProviderFilter := &MockFilter{} + + // Expect Set method calls + mockConsumerFilter.On("Set", mock.Anything, mock.Anything).Return() + mockProviderFilter.On("Set", mock.Anything, mock.Anything).Return() + + // Register mock filters + extension.SetFilter(constant.GracefulShutdownConsumerFilterKey, func() filter.Filter { + return mockConsumerFilter + }) + extension.SetFilter(constant.GracefulShutdownProviderFilterKey, func() filter.Filter { + return mockProviderFilter + }) + + // Test with default options + Init() + + // Test with custom options + customTimeout := 120 * time.Second + Init(WithTimeout(customTimeout)) + + // Remove mock filters + extension.UnregisterFilter(constant.GracefulShutdownConsumerFilterKey) + extension.UnregisterFilter(constant.GracefulShutdownProviderFilterKey) +} + +func TestRegisterProtocol(t *testing.T) { + // Reset protocols for testing + protocols = make(map[string]struct{}) + proMu = sync.Mutex{} + + // Register some protocols + RegisterProtocol("dubbo") + RegisterProtocol("rest") + RegisterProtocol("tri") + + // Check if protocols are registered correctly + proMu.Lock() + defer proMu.Unlock() + + assert.Contains(t, protocols, "dubbo") + assert.Contains(t, protocols, "rest") + assert.Contains(t, protocols, "tri") + assert.Len(t, protocols, 3) +} + +func TestTotalTimeout(t *testing.T) { + // Test with default timeout + config := global.DefaultShutdownConfig() + timeout := totalTimeout(config) + assert.Equal(t, defaultTimeout, timeout) + + // Test with custom timeout + config.Timeout = "120s" + timeout = totalTimeout(config) + assert.Equal(t, 120*time.Second, timeout) + + // Test with invalid timeout + config.Timeout = "invalid" + timeout = totalTimeout(config) + assert.Equal(t, defaultTimeout, timeout) + + // Test with timeout less than default + config.Timeout = "30s" + timeout = totalTimeout(config) + assert.Equal(t, defaultTimeout, timeout) // Should use default if less than default +} + +func TestParseDuration(t *testing.T) { + // Test with valid duration + res := parseDuration("10s", "test", 5*time.Second) + assert.Equal(t, 10*time.Second, res) + + // Test with invalid duration + res = parseDuration("invalid", "test", 5*time.Second) + assert.Equal(t, 5*time.Second, res) + + // Test with empty string + res = parseDuration("", "test", 5*time.Second) + assert.Equal(t, 5*time.Second, res) +} + +func TestWaitAndAcceptNewRequests(t *testing.T) { + // Test with positive step timeout + config := global.DefaultShutdownConfig() + config.StepTimeout = "100ms" + config.ProviderActiveCount.Store(0) + + start := time.Now() + waitAndAcceptNewRequests(config) + elapsed := time.Since(start) + + // Should wait for ConsumerUpdateWaitTime (default 3s) plus a little extra for processing + assert.GreaterOrEqual(t, elapsed, 3*time.Second) + + // Test with negative step timeout (should skip waiting) + config.StepTimeout = "-1s" + start = time.Now() + waitAndAcceptNewRequests(config) + elapsed = time.Since(start) + + // Should only wait for ConsumerUpdateWaitTime Review Comment: The comment states "Should wait for ConsumerUpdateWaitTime (default 3s) plus a little extra for processing" but this is inaccurate. The waitAndAcceptNewRequests function waits for ConsumerUpdateWaitTime (3s), then calls waitingProviderProcessedTimeout which will wait up to stepTimeout (100ms in this test) while checking ProviderActiveCount. Since ProviderActiveCount is 0, it should return quickly, but the comment should clarify what the function actually does rather than describing expected behavior imprecisely. ```suggestion // waitAndAcceptNewRequests first waits for ConsumerUpdateWaitTime (default 3s), then // calls waitingProviderProcessedTimeout which may wait up to StepTimeout while // ProviderActiveCount > 0. Since ProviderActiveCount is 0 in this test, the second // phase returns quickly, so elapsed should be at least ConsumerUpdateWaitTime. assert.GreaterOrEqual(t, elapsed, 3*time.Second) // Test with negative step timeout (should skip the provider processing wait) config.StepTimeout = "-1s" start = time.Now() waitAndAcceptNewRequests(config) elapsed = time.Since(start) // Should effectively only wait for ConsumerUpdateWaitTime ``` ########## graceful_shutdown/shutdown_test.go: ########## @@ -0,0 +1,192 @@ +/* + * 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 graceful_shutdown + +import ( + "context" + "sync" + "testing" + "time" +) + +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common/constant" + "dubbo.apache.org/dubbo-go/v3/common/extension" + "dubbo.apache.org/dubbo-go/v3/filter" + "dubbo.apache.org/dubbo-go/v3/global" + "dubbo.apache.org/dubbo-go/v3/protocol/base" + "dubbo.apache.org/dubbo-go/v3/protocol/result" +) + +// MockFilter implements filter.Filter and config.Setter for testing +type MockFilter struct { + mock.Mock +} + +func (m *MockFilter) Set(key string, value any) { + m.Called(key, value) +} + +func (m *MockFilter) Invoke(ctx context.Context, invoker base.Invoker, invocation base.Invocation) result.Result { + return nil +} + +func (m *MockFilter) OnResponse(ctx context.Context, result result.Result, invoker base.Invoker, invocation base.Invocation) result.Result { + return nil +} + +func TestInit(t *testing.T) { + // Reset initOnce and protocols for testing + initOnce = sync.Once{} + protocols = nil + proMu = sync.Mutex{} + + // Register mock filters + mockConsumerFilter := &MockFilter{} + mockProviderFilter := &MockFilter{} + + // Expect Set method calls + mockConsumerFilter.On("Set", mock.Anything, mock.Anything).Return() + mockProviderFilter.On("Set", mock.Anything, mock.Anything).Return() + + // Register mock filters + extension.SetFilter(constant.GracefulShutdownConsumerFilterKey, func() filter.Filter { + return mockConsumerFilter + }) + extension.SetFilter(constant.GracefulShutdownProviderFilterKey, func() filter.Filter { + return mockProviderFilter + }) + + // Test with default options + Init() + + // Test with custom options + customTimeout := 120 * time.Second + Init(WithTimeout(customTimeout)) + + // Remove mock filters + extension.UnregisterFilter(constant.GracefulShutdownConsumerFilterKey) + extension.UnregisterFilter(constant.GracefulShutdownProviderFilterKey) +} + +func TestRegisterProtocol(t *testing.T) { + // Reset protocols for testing + protocols = make(map[string]struct{}) + proMu = sync.Mutex{} + + // Register some protocols + RegisterProtocol("dubbo") + RegisterProtocol("rest") + RegisterProtocol("tri") + + // Check if protocols are registered correctly + proMu.Lock() + defer proMu.Unlock() + + assert.Contains(t, protocols, "dubbo") + assert.Contains(t, protocols, "rest") + assert.Contains(t, protocols, "tri") + assert.Len(t, protocols, 3) +} + +func TestTotalTimeout(t *testing.T) { + // Test with default timeout + config := global.DefaultShutdownConfig() + timeout := totalTimeout(config) + assert.Equal(t, defaultTimeout, timeout) + + // Test with custom timeout + config.Timeout = "120s" + timeout = totalTimeout(config) + assert.Equal(t, 120*time.Second, timeout) + + // Test with invalid timeout + config.Timeout = "invalid" + timeout = totalTimeout(config) + assert.Equal(t, defaultTimeout, timeout) + + // Test with timeout less than default + config.Timeout = "30s" + timeout = totalTimeout(config) + assert.Equal(t, defaultTimeout, timeout) // Should use default if less than default +} + +func TestParseDuration(t *testing.T) { + // Test with valid duration + res := parseDuration("10s", "test", 5*time.Second) + assert.Equal(t, 10*time.Second, res) + + // Test with invalid duration + res = parseDuration("invalid", "test", 5*time.Second) + assert.Equal(t, 5*time.Second, res) + + // Test with empty string + res = parseDuration("", "test", 5*time.Second) + assert.Equal(t, 5*time.Second, res) +} + +func TestWaitAndAcceptNewRequests(t *testing.T) { + // Test with positive step timeout + config := global.DefaultShutdownConfig() + config.StepTimeout = "100ms" + config.ProviderActiveCount.Store(0) + + start := time.Now() + waitAndAcceptNewRequests(config) + elapsed := time.Since(start) + + // Should wait for ConsumerUpdateWaitTime (default 3s) plus a little extra for processing + assert.GreaterOrEqual(t, elapsed, 3*time.Second) + + // Test with negative step timeout (should skip waiting) + config.StepTimeout = "-1s" + start = time.Now() + waitAndAcceptNewRequests(config) + elapsed = time.Since(start) + + // Should only wait for ConsumerUpdateWaitTime + assert.Less(t, elapsed, 3*time.Second+100*time.Millisecond) Review Comment: The assertion logic is incorrect. When StepTimeout is negative (-1s), the function waitAndAcceptNewRequests still waits for ConsumerUpdateWaitTime (3s) before checking if stepTimeout is negative and returning. The assertion expects the elapsed time to be less than 3s + 100ms, but it should actually be greater than or equal to 3s since ConsumerUpdateWaitTime sleep happens before the stepTimeout check. ```suggestion // Test with negative step timeout (should not add extra wait beyond ConsumerUpdateWaitTime) config.StepTimeout = "-1s" start = time.Now() waitAndAcceptNewRequests(config) elapsed = time.Since(start) // Should still wait for at least ConsumerUpdateWaitTime assert.GreaterOrEqual(t, elapsed, 3*time.Second) ``` -- 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]
