Copilot commented on code in PR #3136: URL: https://github.com/apache/dubbo-go/pull/3136#discussion_r2638811560
########## protocol/dubbo/impl/serialize_test.go: ########## @@ -0,0 +1,486 @@ +/* + * 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 impl + +import ( + "bytes" + "fmt" + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common/constant" +) + +// TestLoadSerializerWithDefaultHessian2 tests LoadSerializer with default Hessian2 serialization +func TestLoadSerializerWithDefaultHessian2(t *testing.T) { + mockSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockSerializer) + + pkg := NewDubboPackage(nil) + pkg.Header.SerialID = 0 // Default SerialID + + err := LoadSerializer(pkg) + assert.NoError(t, err) + assert.NotNil(t, pkg.Codec) +} + +// TestLoadSerializerWithHessian2 tests LoadSerializer with explicit Hessian2 serialization +func TestLoadSerializerWithHessian2(t *testing.T) { + mockSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockSerializer) + + pkg := NewDubboPackage(nil) + pkg.Header.SerialID = constant.SHessian2 + + err := LoadSerializer(pkg) + assert.NoError(t, err) + assert.NotNil(t, pkg.Codec) +} + +// TestLoadSerializerWithDifferentValidID tests LoadSerializer with different valid serializer ID +func TestLoadSerializerWithDifferentValidID(t *testing.T) { + mockHessianSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockHessianSerializer) + + pkg := NewDubboPackage(nil) + pkg.Header.SerialID = constant.SHessian2 + + err := LoadSerializer(pkg) + assert.NoError(t, err) + assert.NotNil(t, pkg.Codec) +} + +// TestLoadSerializerWithInvalidID tests LoadSerializer with invalid serializer ID panics +func TestLoadSerializerWithInvalidID(t *testing.T) { + mockSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockSerializer) + + pkg := NewDubboPackage(nil) + pkg.Header.SerialID = 255 // Invalid serializer ID + + assert.Panics(t, func() { + LoadSerializer(pkg) + }) +} + +// TestLoadSerializerWithZeroIDDefaultsToHessian2 tests that SerialID 0 defaults to Hessian2 +func TestLoadSerializerWithZeroIDDefaultsToHessian2(t *testing.T) { + mockHessianSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockHessianSerializer) + + pkg := NewDubboPackage(nil) + pkg.Header.SerialID = 0 + + err := LoadSerializer(pkg) + assert.NoError(t, err) + // Verify that the loaded serializer is for Hessian2 + assert.NotNil(t, pkg.Codec) +} + +// TestLoadSerializerWithNilPackage tests LoadSerializer handles package correctly +func TestLoadSerializerWithNilPackage(t *testing.T) { + mockSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockSerializer) + + pkg := NewDubboPackage(nil) + pkg.Header.SerialID = constant.SHessian2 + + assert.NotPanics(t, func() { + err := LoadSerializer(pkg) + assert.NoError(t, err) + }) +} + +// TestLoadSerializerMultipleInstances tests that LoadSerializer works with multiple packages +func TestLoadSerializerMultipleInstances(t *testing.T) { + mockHessianSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockHessianSerializer) + + pkg1 := NewDubboPackage(nil) + pkg1.Header.SerialID = constant.SHessian2 + + pkg2 := NewDubboPackage(nil) + pkg2.Header.SerialID = 0 // Default to Hessian2 + + err1 := LoadSerializer(pkg1) + err2 := LoadSerializer(pkg2) + + assert.NoError(t, err1) + assert.NoError(t, err2) + assert.NotNil(t, pkg1.Codec) + assert.NotNil(t, pkg2.Codec) +} + +// TestLoadSerializerWithBufferedData tests LoadSerializer with buffered data in package +func TestLoadSerializerWithBufferedData(t *testing.T) { + mockSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockSerializer) + + data := bytes.NewBuffer([]byte{0x01, 0x02, 0x03}) + pkg := NewDubboPackage(data) + pkg.Header.SerialID = constant.SHessian2 + + err := LoadSerializer(pkg) + assert.NoError(t, err) + assert.NotNil(t, pkg.Codec) +} + +// TestLoadSerializerDoesNotPanicWithValidID tests LoadSerializer doesn't panic with valid IDs +func TestLoadSerializerDoesNotPanicWithValidID(t *testing.T) { + mockHessianSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockHessianSerializer) + + tests := []struct { + desc string + serialID byte + shouldFail bool + }{ + { + desc: "default Hessian2 (0)", + serialID: 0, + shouldFail: false, + }, + { + desc: "explicit Hessian2", + serialID: constant.SHessian2, + shouldFail: false, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + pkg := NewDubboPackage(nil) + pkg.Header.SerialID = test.serialID + + if test.shouldFail { + assert.Panics(t, func() { + LoadSerializer(pkg) + }) + } else { + assert.NotPanics(t, func() { + err := LoadSerializer(pkg) + assert.NoError(t, err) + }) + } + }) + } +} + +// TestLoadSerializerSetsSerializerInCodec tests that LoadSerializer properly sets the serializer in the codec +func TestLoadSerializerSetsSerializerInCodec(t *testing.T) { + mockSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockSerializer) + + pkg := NewDubboPackage(nil) + pkg.Header.SerialID = constant.SHessian2 + + err := LoadSerializer(pkg) + assert.NoError(t, err) + assert.NotNil(t, pkg.Codec) + assert.NotNil(t, pkg.Codec.serializer) +} + +// TestSerializerInterface tests that Serializer interface is properly defined +func TestSerializerInterface(t *testing.T) { + // Verify that HessianSerializer implements Serializer interface + var _ Serializer = (*HessianSerializer)(nil) + + mockSerializer := &HessianSerializer{} + assert.NotNil(t, mockSerializer) +} + +// TestLoadSerializerConsistency tests that LoadSerializer gives consistent results for same input +func TestLoadSerializerConsistency(t *testing.T) { + mockSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockSerializer) + + pkg1 := NewDubboPackage(nil) + pkg1.Header.SerialID = constant.SHessian2 + + pkg2 := NewDubboPackage(nil) + pkg2.Header.SerialID = constant.SHessian2 + + err1 := LoadSerializer(pkg1) + err2 := LoadSerializer(pkg2) + + assert.NoError(t, err1) + assert.NoError(t, err2) + assert.Equal(t, err1, err2) +} + +// TestLoadSerializerReplacesExistingSerializer tests that LoadSerializer can replace existing serializer +func TestLoadSerializerReplacesExistingSerializer(t *testing.T) { + mockSerializer1 := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockSerializer1) + + pkg := NewDubboPackage(nil) + pkg.Header.SerialID = constant.SHessian2 + + err1 := LoadSerializer(pkg) + assert.NoError(t, err1) + + // Create a new mock serializer and replace + mockSerializer2 := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockSerializer2) + + err2 := LoadSerializer(pkg) + assert.NoError(t, err2) +} + +// TestLoadSerializerWithAllBytesValues tests LoadSerializer with various byte values +func TestLoadSerializerWithAllBytesValues(t *testing.T) { + mockSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockSerializer) + + validValues := []byte{ + 0, // Default - should map to Hessian2 + constant.SHessian2, // Explicit Hessian2 + } + + for _, byteVal := range validValues { + t.Run(fmt.Sprintf("serial_id_%d", byteVal), func(t *testing.T) { + pkg := NewDubboPackage(nil) + pkg.Header.SerialID = byteVal + + if byteVal == 0 || byteVal == constant.SHessian2 { + err := LoadSerializer(pkg) + assert.NoError(t, err) + assert.NotNil(t, pkg.Codec) + } + }) + } +} + +// TestLoadSerializerWithInvalidByteValues tests that invalid byte values panic +func TestLoadSerializerWithInvalidByteValues(t *testing.T) { + mockSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockSerializer) + + // Only these specific values should panic (not in nameMaps) + invalidValues := []byte{1, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 22, 23, 254, 255} Review Comment: The list of invalid byte values appears to be hardcoded and may not accurately represent all invalid serializer IDs. This test could fail if new serializers are added or if the valid ID range changes. Consider making this test more maintainable by determining invalid values programmatically based on what's registered in the serializer map, or by testing against a known invalid value like 255 only. ```suggestion // Use a known invalid value that should not be registered as a serializer ID. invalidValues := []byte{255} ``` ########## protocol/dubbo/impl/hessian_test.go: ########## @@ -59,6 +66,266 @@ func TestGetArgType(t *testing.T) { t.Run("param", func(t *testing.T) { assert.Equal(t, dubboParam, getArgType(&Param{})) }) + + t.Run("nil", func(t *testing.T) { + assert.Equal(t, "V", getArgType(nil)) + }) + + t.Run("bool", func(t *testing.T) { + assert.Equal(t, "Z", getArgType(true)) + }) + + t.Run("bool array", func(t *testing.T) { + assert.Equal(t, "[Z", getArgType([]bool{true, false})) + }) + + t.Run("byte", func(t *testing.T) { + assert.Equal(t, "B", getArgType(byte(1))) + }) + + t.Run("byte array", func(t *testing.T) { + assert.Equal(t, "[B", getArgType([]byte{1, 2, 3})) + }) + + t.Run("int8", func(t *testing.T) { + assert.Equal(t, "B", getArgType(int8(1))) + }) + + t.Run("int8 array", func(t *testing.T) { + assert.Equal(t, "[B", getArgType([]int8{1, 2, 3})) + }) + + t.Run("int16", func(t *testing.T) { + assert.Equal(t, "S", getArgType(int16(1))) + }) + + t.Run("int16 array", func(t *testing.T) { + assert.Equal(t, "[S", getArgType([]int16{1, 2, 3})) + }) + + t.Run("uint16", func(t *testing.T) { + assert.Equal(t, "C", getArgType(uint16(1))) + }) + + t.Run("uint16 array", func(t *testing.T) { + assert.Equal(t, "[C", getArgType([]uint16{1, 2, 3})) + }) + + t.Run("int", func(t *testing.T) { + assert.Equal(t, "J", getArgType(int(1))) + }) + + t.Run("int array", func(t *testing.T) { + assert.Equal(t, "[J", getArgType([]int{1, 2, 3})) + }) + + t.Run("int32", func(t *testing.T) { + assert.Equal(t, "I", getArgType(int32(1))) + }) + + t.Run("int32 array", func(t *testing.T) { + assert.Equal(t, "[I", getArgType([]int32{1, 2, 3})) + }) + + t.Run("int64", func(t *testing.T) { + assert.Equal(t, "J", getArgType(int64(1))) + }) + + t.Run("int64 array", func(t *testing.T) { + assert.Equal(t, "[J", getArgType([]int64{1, 2, 3})) + }) + + t.Run("time.Time", func(t *testing.T) { + assert.Equal(t, "java.util.Date", getArgType(time.Now())) + }) + + t.Run("time.Time array", func(t *testing.T) { + assert.Equal(t, "[Ljava.util.Date", getArgType([]time.Time{time.Now()})) + }) + + t.Run("float32", func(t *testing.T) { + assert.Equal(t, "F", getArgType(float32(1.0))) + }) + + t.Run("float32 array", func(t *testing.T) { + assert.Equal(t, "[F", getArgType([]float32{1.0, 2.0})) + }) + + t.Run("float64", func(t *testing.T) { + assert.Equal(t, "D", getArgType(float64(1.0))) + }) + + t.Run("float64 array", func(t *testing.T) { + assert.Equal(t, "[D", getArgType([]float64{1.0, 2.0})) + }) + + t.Run("string", func(t *testing.T) { + assert.Equal(t, "java.lang.String", getArgType("test")) + }) + + t.Run("string array", func(t *testing.T) { + assert.Equal(t, "[Ljava.lang.String;", getArgType([]string{"test1", "test2"})) + }) + + t.Run("map[any]any", func(t *testing.T) { + assert.Equal(t, "java.util.Map", getArgType(map[any]any{"key": "value"})) + }) + + t.Run("map[string]int", func(t *testing.T) { + assert.Equal(t, "java.util.Map", getArgType(map[string]int{"key": 1})) + }) + + t.Run("pointer types", func(t *testing.T) { + v := int8(1) + assert.Equal(t, "java.lang.Byte", getArgType(&v)) + + v2 := int16(1) + assert.Equal(t, "java.lang.Short", getArgType(&v2)) + + v3 := uint16(1) + assert.Equal(t, "java.lang.Character", getArgType(&v3)) + + v4 := int(1) + assert.Equal(t, "java.lang.Long", getArgType(&v4)) + + v5 := int32(1) + assert.Equal(t, "java.lang.Integer", getArgType(&v5)) + + v6 := int64(1) + assert.Equal(t, "java.lang.Long", getArgType(&v6)) + + v7 := float32(1.0) + assert.Equal(t, "java.lang.Float", getArgType(&v7)) + + v8 := float64(1.0) + assert.Equal(t, "java.lang.Double", getArgType(&v8)) + }) + + t.Run("slice of structs", func(t *testing.T) { + assert.Equal(t, "[Ljava.lang.Object;", getArgType([]Pojo{{}, {}})) + }) + + t.Run("slice of string", func(t *testing.T) { + // Note: []string returns array type, not list Review Comment: The comment "Note: []string returns array type, not list" is misleading. In the test context, it's not returning anything different from what's expected - getArgType consistently returns the array type representation for slices. This comment doesn't add value and may confuse readers. Consider removing it or clarifying what point it's trying to make. ```suggestion ``` ########## protocol/dubbo/dubbo_codec_test.go: ########## @@ -0,0 +1,265 @@ +/* + * 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 dubbo + +import ( + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +// TestIsRequest tests isRequest with various bit patterns +func TestIsRequest(t *testing.T) { + codec := &DubboCodec{} + + tests := []struct { + desc string + data []byte + expected bool + }{ + { + desc: "request with 0x80 at position 2", + data: []byte{0x00, 0x00, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: true, + }, + { + desc: "request with 0xFF at position 2", + data: []byte{0x00, 0x00, 0xFF, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: true, + }, + { + desc: "response with 0x00 at position 2", + data: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + { + desc: "response with 0x7F at position 2", + data: []byte{0x00, 0x00, 0x7F, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + result := codec.isRequest(test.data) + assert.Equal(t, test.expected, result) + }) + } +} + +// TestDecodeWithInsufficientData tests decode with insufficient header data +func TestDecodeWithInsufficientData(t *testing.T) { + codec := &DubboCodec{} + + // Data shorter than header length + data := []byte{0x01, 0x02} + + decodeResult, length, err := codec.Decode(data) + assert.Nil(t, decodeResult) + assert.Equal(t, 0, length) + assert.NoError(t, err) +} + +// TestDecodeWithEmptyData tests decode with empty data +func TestDecodeWithEmptyData(t *testing.T) { + codec := &DubboCodec{} + + data := []byte{} + + decodeResult, length, err := codec.Decode(data) + assert.Nil(t, decodeResult) + assert.Equal(t, 0, length) + assert.NoError(t, err) +} + +// TestCodecType tests that DubboCodec is properly instantiated +func TestCodecType(t *testing.T) { + codec := &DubboCodec{} + + // Verify codec implements the expected interface by checking it has the methods + assert.NotNil(t, codec.EncodeRequest) + assert.NotNil(t, codec.EncodeResponse) + assert.NotNil(t, codec.Decode) + assert.NotNil(t, codec.encodeHeartbeatRequest) +} + +// TestIsRequestEdgeCases tests isRequest with edge case bit patterns +func TestIsRequestEdgeCases(t *testing.T) { + codec := &DubboCodec{} + + tests := []struct { + desc string + data []byte + expected bool + }{ + { + desc: "bit 0 only (not request bit)", + data: []byte{0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + { + desc: "bit 1 only (not request bit)", + data: []byte{0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + { + desc: "bit 2 only (request bit set)", + data: []byte{0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, Review Comment: The description says "multiple bits set including bit 2" but the actual test is checking for the 0x80 bit (bit 7) at position 2 of the data array. The terminology is confusing because "bit 2" could refer to either bit position 2 in a byte or byte position 2 in the array. Consider clarifying the description to say "multiple bytes with 0xFF, and byte position 2 has 0x80 (bit 7 set)" to avoid ambiguity. ```suggestion desc: "multiple bytes with 0xFF, and byte position 2 has 0x80 (bit 7 set)", ``` ########## protocol/dubbo/impl/response_test.go: ########## @@ -0,0 +1,215 @@ +/* + * 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 impl + +import ( + "errors" + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +// TestNewResponsePayload tests creating a new ResponsePayload with various inputs +func TestNewResponsePayload(t *testing.T) { + tests := []struct { + desc string + rspObj any + exception error + attachments map[string]any + expectRspObj any + expectException error + expectAttachments map[string]any + }{ + { + desc: "with all parameters", + rspObj: "response data", + exception: errors.New("test error"), + attachments: map[string]any{"key": "value"}, + expectRspObj: "response data", + expectException: errors.New("test error"), Review Comment: The test creates a new ResponsePayload with errors.New("test error") as the exception, then compares it with errors.New("test error") in the expectException field. However, each call to errors.New creates a new error instance, so these won't be equal when compared with assert.Equal. The assertion at line 86 checks for Error() which only verifies the error is non-nil, but the test structure suggests intent to compare exact error values. Consider using a pre-defined error variable or adjusting the test expectations to match what's actually being validated. ########## protocol/dubbo/dubbo_codec_test.go: ########## @@ -0,0 +1,265 @@ +/* + * 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 dubbo + +import ( + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +// TestIsRequest tests isRequest with various bit patterns +func TestIsRequest(t *testing.T) { + codec := &DubboCodec{} + + tests := []struct { + desc string + data []byte + expected bool + }{ + { + desc: "request with 0x80 at position 2", + data: []byte{0x00, 0x00, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: true, + }, + { + desc: "request with 0xFF at position 2", + data: []byte{0x00, 0x00, 0xFF, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: true, + }, + { + desc: "response with 0x00 at position 2", + data: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + { + desc: "response with 0x7F at position 2", + data: []byte{0x00, 0x00, 0x7F, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + result := codec.isRequest(test.data) + assert.Equal(t, test.expected, result) + }) + } +} + +// TestDecodeWithInsufficientData tests decode with insufficient header data +func TestDecodeWithInsufficientData(t *testing.T) { + codec := &DubboCodec{} + + // Data shorter than header length + data := []byte{0x01, 0x02} + + decodeResult, length, err := codec.Decode(data) + assert.Nil(t, decodeResult) + assert.Equal(t, 0, length) + assert.NoError(t, err) +} + +// TestDecodeWithEmptyData tests decode with empty data +func TestDecodeWithEmptyData(t *testing.T) { + codec := &DubboCodec{} + + data := []byte{} + + decodeResult, length, err := codec.Decode(data) + assert.Nil(t, decodeResult) + assert.Equal(t, 0, length) + assert.NoError(t, err) +} + +// TestCodecType tests that DubboCodec is properly instantiated +func TestCodecType(t *testing.T) { + codec := &DubboCodec{} + + // Verify codec implements the expected interface by checking it has the methods + assert.NotNil(t, codec.EncodeRequest) + assert.NotNil(t, codec.EncodeResponse) + assert.NotNil(t, codec.Decode) + assert.NotNil(t, codec.encodeHeartbeatRequest) +} + +// TestIsRequestEdgeCases tests isRequest with edge case bit patterns +func TestIsRequestEdgeCases(t *testing.T) { + codec := &DubboCodec{} + + tests := []struct { + desc string + data []byte + expected bool + }{ + { + desc: "bit 0 only (not request bit)", + data: []byte{0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + { + desc: "bit 1 only (not request bit)", + data: []byte{0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, Review Comment: The description says "bit 5 only (not bit 2)" but 0x04 is 0b00000100, which is bit 2, not bit 5. The description should be updated to accurately reflect the bit being tested. ```suggestion desc: "bit 2 only (not bit 5)", ``` ########## protocol/dubbo/impl/serialization_test.go: ########## @@ -0,0 +1,157 @@ +/* + * 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 impl + +import ( + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common/constant" +) + +// TestGetSerializerById tests GetSerializerById with valid and invalid serializer IDs +func TestGetSerializerById(t *testing.T) { + // Setup: Create a mock serializer for testing + mockSerializer := &HessianSerializer{} + SetSerializer(constant.Hessian2Serialization, mockSerializer) + + tests := []struct { + desc string + id byte + shouldPanic bool + }{ + { + desc: "valid Hessian2 serializer ID", + id: constant.SHessian2, + shouldPanic: false, + }, + { + desc: "valid Protobuf serializer ID", + id: constant.SProto, + shouldPanic: true, // Will panic because serializer is not registered + }, + { + desc: "invalid serializer ID 255", + id: 255, + shouldPanic: true, // Will panic because ID is not in nameMaps + }, + { + desc: "invalid serializer ID 0", + id: 0, + shouldPanic: true, // Will panic because ID is not in nameMaps Review Comment: This test case description says "invalid serializer ID 0" but indicates shouldPanic is true. However, line 265 in the same file shows that serializer ID 0 should map to Hessian2 (the default). This is inconsistent. The test expectation should be shouldPanic: false for ID 0, or the test description should be updated to clarify that ID 0 panics when it's not mapped in nameMaps. ```suggestion desc: "default Hessian2 serializer ID 0", id: 0, shouldPanic: false, ``` ########## protocol/dubbo/impl/response_test.go: ########## @@ -0,0 +1,215 @@ +/* + * 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 impl + +import ( + "errors" + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +// TestNewResponsePayload tests creating a new ResponsePayload with various inputs +func TestNewResponsePayload(t *testing.T) { + tests := []struct { + desc string + rspObj any + exception error + attachments map[string]any + expectRspObj any + expectException error + expectAttachments map[string]any + }{ + { + desc: "with all parameters", + rspObj: "response data", + exception: errors.New("test error"), + attachments: map[string]any{"key": "value"}, + expectRspObj: "response data", + expectException: errors.New("test error"), + expectAttachments: map[string]any{"key": "value"}, + }, + { + desc: "with nil attachments", + rspObj: "response data", + exception: nil, + attachments: nil, + expectRspObj: "response data", + expectException: nil, + expectAttachments: make(map[string]any), + }, + { + desc: "with nil rspObj and exception", + rspObj: nil, + exception: errors.New("error occurred"), + attachments: map[string]any{"error_code": 500}, + expectRspObj: nil, + expectException: errors.New("error occurred"), Review Comment: Similar to the response test, this test creates expectException with errors.New("error occurred") but doesn't properly validate it since each errors.New call creates a distinct error instance. The test at line 64 also creates a new error with errors.New that won't equal the one created at line 61. Consider defining error variables once and reusing them throughout the test case. ########## protocol/dubbo/dubbo_codec_test.go: ########## @@ -0,0 +1,265 @@ +/* + * 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 dubbo + +import ( + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +// TestIsRequest tests isRequest with various bit patterns +func TestIsRequest(t *testing.T) { + codec := &DubboCodec{} + + tests := []struct { + desc string + data []byte + expected bool + }{ + { + desc: "request with 0x80 at position 2", + data: []byte{0x00, 0x00, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: true, + }, + { + desc: "request with 0xFF at position 2", + data: []byte{0x00, 0x00, 0xFF, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: true, + }, + { + desc: "response with 0x00 at position 2", + data: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + { + desc: "response with 0x7F at position 2", + data: []byte{0x00, 0x00, 0x7F, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + result := codec.isRequest(test.data) + assert.Equal(t, test.expected, result) + }) + } +} + +// TestDecodeWithInsufficientData tests decode with insufficient header data +func TestDecodeWithInsufficientData(t *testing.T) { + codec := &DubboCodec{} + + // Data shorter than header length + data := []byte{0x01, 0x02} + + decodeResult, length, err := codec.Decode(data) + assert.Nil(t, decodeResult) + assert.Equal(t, 0, length) + assert.NoError(t, err) +} + +// TestDecodeWithEmptyData tests decode with empty data +func TestDecodeWithEmptyData(t *testing.T) { + codec := &DubboCodec{} + + data := []byte{} + + decodeResult, length, err := codec.Decode(data) + assert.Nil(t, decodeResult) + assert.Equal(t, 0, length) + assert.NoError(t, err) +} + +// TestCodecType tests that DubboCodec is properly instantiated +func TestCodecType(t *testing.T) { + codec := &DubboCodec{} + + // Verify codec implements the expected interface by checking it has the methods + assert.NotNil(t, codec.EncodeRequest) + assert.NotNil(t, codec.EncodeResponse) + assert.NotNil(t, codec.Decode) + assert.NotNil(t, codec.encodeHeartbeatRequest) +} + +// TestIsRequestEdgeCases tests isRequest with edge case bit patterns +func TestIsRequestEdgeCases(t *testing.T) { + codec := &DubboCodec{} + + tests := []struct { + desc string + data []byte + expected bool + }{ + { + desc: "bit 0 only (not request bit)", + data: []byte{0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + { + desc: "bit 1 only (not request bit)", + data: []byte{0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + { + desc: "bit 2 only (request bit set)", + data: []byte{0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + { + desc: "bit 7 set with other bits (includes request bit)", + data: []byte{0xFF, 0xFF, 0x80, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}, + expected: true, + }, Review Comment: The comment says "bit 7 only (not bit 2)" but the test data 0x01 represents bit 0, not bit 7. In binary, 0x01 is 0b00000001 (bit 0 set). The description should say "bit 0 only" to match the actual test data. Similar issues exist in the following test cases where the bit numbers in descriptions don't match the hex values. ```suggestion desc: "bit 0 only", data: []byte{0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00}, expected: false, }, { desc: "bit 1 only", data: []byte{0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00}, expected: false, }, { desc: "bit 2 only", data: []byte{0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00}, expected: false, }, { desc: "multiple bits set including bit 7", data: []byte{0xFF, 0xFF, 0x80, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}, expected: true, }, { desc: "all bits set except bit 7", ``` ########## protocol/dubbo/dubbo_codec_test.go: ########## @@ -0,0 +1,265 @@ +/* + * 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 dubbo + +import ( + "testing" +) + +import ( + "github.com/stretchr/testify/assert" +) + +// TestIsRequest tests isRequest with various bit patterns +func TestIsRequest(t *testing.T) { + codec := &DubboCodec{} + + tests := []struct { + desc string + data []byte + expected bool + }{ + { + desc: "request with 0x80 at position 2", + data: []byte{0x00, 0x00, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: true, + }, + { + desc: "request with 0xFF at position 2", + data: []byte{0x00, 0x00, 0xFF, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: true, + }, + { + desc: "response with 0x00 at position 2", + data: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + { + desc: "response with 0x7F at position 2", + data: []byte{0x00, 0x00, 0x7F, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + result := codec.isRequest(test.data) + assert.Equal(t, test.expected, result) + }) + } +} + +// TestDecodeWithInsufficientData tests decode with insufficient header data +func TestDecodeWithInsufficientData(t *testing.T) { + codec := &DubboCodec{} + + // Data shorter than header length + data := []byte{0x01, 0x02} + + decodeResult, length, err := codec.Decode(data) + assert.Nil(t, decodeResult) + assert.Equal(t, 0, length) + assert.NoError(t, err) +} + +// TestDecodeWithEmptyData tests decode with empty data +func TestDecodeWithEmptyData(t *testing.T) { + codec := &DubboCodec{} + + data := []byte{} + + decodeResult, length, err := codec.Decode(data) + assert.Nil(t, decodeResult) + assert.Equal(t, 0, length) + assert.NoError(t, err) +} + +// TestCodecType tests that DubboCodec is properly instantiated +func TestCodecType(t *testing.T) { + codec := &DubboCodec{} + + // Verify codec implements the expected interface by checking it has the methods + assert.NotNil(t, codec.EncodeRequest) + assert.NotNil(t, codec.EncodeResponse) + assert.NotNil(t, codec.Decode) + assert.NotNil(t, codec.encodeHeartbeatRequest) +} + +// TestIsRequestEdgeCases tests isRequest with edge case bit patterns +func TestIsRequestEdgeCases(t *testing.T) { + codec := &DubboCodec{} + + tests := []struct { + desc string + data []byte + expected bool + }{ + { + desc: "bit 0 only (not request bit)", + data: []byte{0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00}, + expected: false, + }, Review Comment: Similar to the previous issue, the description says "bit 6 only (not bit 2)" but 0x02 is 0b00000010, which is bit 1, not bit 6. The description should be updated to accurately reflect which bit is being tested. ```suggestion desc: "bit 1 only (not bit 2)", ``` -- 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]
