You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "atharvalade (via GitHub)" <gi...@apache.org> on 2026/03/26 17:46:53 UTC
[PR] test(go): increase Go SDK Codecov coverage with comprehensive unit tests [iggy]
atharvalade opened a new pull request, #3037:
URL: https://github.com/apache/iggy/pull/3037
## Which issue does this PR close?
Closes #2978
## Rationale
Go SDK Codecov coverage was only 36.37%. This PR adds comprehensive unit tests across all major Go SDK packages and adjusts codecov.yml to exclude non-library code (samples, benchmarks) from the coverage denominator.
## What changed?
Most Go SDK packages (contracts, internal/command, binary_serialization, internal/util) had minimal or no unit tests for their serialization, deserialization, and constructor logic. The coverage profile was also diluted by untestable sample/benchmark code counting against the denominator.
Added 20+ new test files with ~130 test functions covering all command MarshalBinary types, all contract marshal/unmarshal roundtrips, all binary response deserializers, and leader redirection decision paths. Excluded `foreign/go/samples/**` and `foreign/go/benchmarks/**` from Codecov ignore list. Also fixed two buffer allocation bugs in `UpdateUser` and `UpdatePermissions` MarshalBinary discovered by the new tests.
## Local Execution
- Passed
- Pre-commit hooks ran
## AI Usage
1. Opus 4.6
2. Minimal AI used
3. Verified by running full `go test -race` suite locally with coverage profiling, plus gofmt, trailing whitespace/newline, and license header checks
4. Yes, all code can be explained
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive unit tests [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r2997562874
##########
foreign/go/errors/errors_gen_test.go:
##########
Review Comment:
yup. now I have extracted a single package-level `allErrors` slice in `errors_gen_test.go` containing `{name, code, err}` tuples. All three test functions now iterate over this shared slice instead of maintaining their own duplicate lists
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade closed pull request #3037: test(go): increase Go SDK Codecov coverage with comprehensive tests
URL: https://github.com/apache/iggy/pull/3037
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r3002495808
##########
foreign/go/contracts/cluster_node_test.go:
##########
@@ -0,0 +1,105 @@
+// 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 iggcon
+
+import (
+ "strings"
+ "testing"
+)
+
+func TestClusterNode_MarshalUnmarshal_Roundtrip(t *testing.T) {
+ original := ClusterNode{
+ Name: "node-1",
+ IP: "192.168.1.10",
+ Endpoints: NewTransportEndpoints(8090, 8091, 3000, 8080),
+ Role: RoleLeader,
+ Status: Healthy,
+ }
+
+ data, err := original.MarshalBinary()
+ if err != nil {
+ t.Fatalf("unexpected marshal error: %v", err)
+ }
+
+ var decoded ClusterNode
+ if err := decoded.UnmarshalBinary(data); err != nil {
+ t.Fatalf("unexpected unmarshal error: %v", err)
+ }
+
+ if decoded.Name != original.Name {
+ t.Fatalf("Name mismatch: expected %s, got %s", original.Name, decoded.Name)
+ }
+ if decoded.IP != original.IP {
+ t.Fatalf("IP mismatch: expected %s, got %s", original.IP, decoded.IP)
+ }
+ if decoded.Endpoints.Tcp != original.Endpoints.Tcp {
+ t.Fatalf("Tcp mismatch: expected %d, got %d", original.Endpoints.Tcp, decoded.Endpoints.Tcp)
+ }
+ if decoded.Endpoints.Quic != original.Endpoints.Quic {
+ t.Fatalf("Quic mismatch: expected %d, got %d", original.Endpoints.Quic, decoded.Endpoints.Quic)
+ }
+ if decoded.Endpoints.Http != original.Endpoints.Http {
+ t.Fatalf("Http mismatch: expected %d, got %d", original.Endpoints.Http, decoded.Endpoints.Http)
+ }
+ if decoded.Endpoints.WebSocket != original.Endpoints.WebSocket {
+ t.Fatalf("WebSocket mismatch: expected %d, got %d", original.Endpoints.WebSocket, decoded.Endpoints.WebSocket)
+ }
+ if decoded.Role != original.Role {
+ t.Fatalf("Role mismatch: expected %d, got %d", original.Role, decoded.Role)
+ }
+ if decoded.Status != original.Status {
+ t.Fatalf("Status mismatch: expected %d, got %d", original.Status, decoded.Status)
+ }
+}
+
+func TestClusterNode_BufferSize_MatchesMarshaledLength(t *testing.T) {
+ node := ClusterNode{
+ Name: "node-1",
+ IP: "192.168.1.10",
+ Endpoints: NewTransportEndpoints(8090, 8091, 3000, 8080),
+ Role: RoleFollower,
+ Status: Starting,
+ }
+
+ data, err := node.MarshalBinary()
+ if err != nil {
+ t.Fatalf("unexpected marshal error: %v", err)
+ }
+
+ if len(data) != node.BufferSize() {
+ t.Fatalf("expected marshaled length %d to match BufferSize %d", len(data), node.BufferSize())
+ }
+}
+
+func TestClusterNode_String_ContainsNameAndIP(t *testing.T) {
+ node := ClusterNode{
+ Name: "test-node",
+ IP: "10.0.0.1",
+ Endpoints: NewTransportEndpoints(1, 2, 3, 4),
+ Role: RoleLeader,
+ Status: Healthy,
+ }
+
+ s := node.String()
+ if !strings.Contains(s, "test-node") {
+ t.Fatalf("expected string to contain 'test-node', got %s", s)
+ }
+ if !strings.Contains(s, "10.0.0.1") {
+ t.Fatalf("expected string to contain '10.0.0.1', got %s", s)
+ }
+}
Review Comment:
I am not sure if it worth it :(
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r3002412870
##########
foreign/go/client/iggy_client_test.go:
##########
Review Comment:
Do we really need to test these things....
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r3002495808
##########
foreign/go/contracts/cluster_node_test.go:
##########
@@ -0,0 +1,105 @@
+// 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 iggcon
+
+import (
+ "strings"
+ "testing"
+)
+
+func TestClusterNode_MarshalUnmarshal_Roundtrip(t *testing.T) {
+ original := ClusterNode{
+ Name: "node-1",
+ IP: "192.168.1.10",
+ Endpoints: NewTransportEndpoints(8090, 8091, 3000, 8080),
+ Role: RoleLeader,
+ Status: Healthy,
+ }
+
+ data, err := original.MarshalBinary()
+ if err != nil {
+ t.Fatalf("unexpected marshal error: %v", err)
+ }
+
+ var decoded ClusterNode
+ if err := decoded.UnmarshalBinary(data); err != nil {
+ t.Fatalf("unexpected unmarshal error: %v", err)
+ }
+
+ if decoded.Name != original.Name {
+ t.Fatalf("Name mismatch: expected %s, got %s", original.Name, decoded.Name)
+ }
+ if decoded.IP != original.IP {
+ t.Fatalf("IP mismatch: expected %s, got %s", original.IP, decoded.IP)
+ }
+ if decoded.Endpoints.Tcp != original.Endpoints.Tcp {
+ t.Fatalf("Tcp mismatch: expected %d, got %d", original.Endpoints.Tcp, decoded.Endpoints.Tcp)
+ }
+ if decoded.Endpoints.Quic != original.Endpoints.Quic {
+ t.Fatalf("Quic mismatch: expected %d, got %d", original.Endpoints.Quic, decoded.Endpoints.Quic)
+ }
+ if decoded.Endpoints.Http != original.Endpoints.Http {
+ t.Fatalf("Http mismatch: expected %d, got %d", original.Endpoints.Http, decoded.Endpoints.Http)
+ }
+ if decoded.Endpoints.WebSocket != original.Endpoints.WebSocket {
+ t.Fatalf("WebSocket mismatch: expected %d, got %d", original.Endpoints.WebSocket, decoded.Endpoints.WebSocket)
+ }
+ if decoded.Role != original.Role {
+ t.Fatalf("Role mismatch: expected %d, got %d", original.Role, decoded.Role)
+ }
+ if decoded.Status != original.Status {
+ t.Fatalf("Status mismatch: expected %d, got %d", original.Status, decoded.Status)
+ }
+}
+
+func TestClusterNode_BufferSize_MatchesMarshaledLength(t *testing.T) {
+ node := ClusterNode{
+ Name: "node-1",
+ IP: "192.168.1.10",
+ Endpoints: NewTransportEndpoints(8090, 8091, 3000, 8080),
+ Role: RoleFollower,
+ Status: Starting,
+ }
+
+ data, err := node.MarshalBinary()
+ if err != nil {
+ t.Fatalf("unexpected marshal error: %v", err)
+ }
+
+ if len(data) != node.BufferSize() {
+ t.Fatalf("expected marshaled length %d to match BufferSize %d", len(data), node.BufferSize())
+ }
+}
+
+func TestClusterNode_String_ContainsNameAndIP(t *testing.T) {
+ node := ClusterNode{
+ Name: "test-node",
+ IP: "10.0.0.1",
+ Endpoints: NewTransportEndpoints(1, 2, 3, 4),
+ Role: RoleLeader,
+ Status: Healthy,
+ }
+
+ s := node.String()
+ if !strings.Contains(s, "test-node") {
+ t.Fatalf("expected string to contain 'test-node', got %s", s)
+ }
+ if !strings.Contains(s, "10.0.0.1") {
+ t.Fatalf("expected string to contain '10.0.0.1', got %s", s)
+ }
+}
Review Comment:
I’m not sure it’s worth it tbh. I think the implementation of `String()` already makes the exact format clear, and changes to it should naturally be caught during code review.
Also, String() doesn’t seem to be used anywhere right now, and its output is pretty similar to `fmt.Printf("%#v", val)`. We could probably consider removing `String()`s in a separate PR.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r2998611708
##########
foreign/go/errors/errors_gen_test.go:
##########
Review Comment:
I think you can modify the code generator, let it generate a list of error in `errors_gen.go` and let `errors_gen_tests.go` iterate and use it
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive unit tests [iggy]
Posted by "slbotbm (via GitHub)" <gi...@apache.org>.
slbotbm commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r2997307922
##########
foreign/go/binary_serialization/binary_response_deserializer_test.go:
##########
Review Comment:
In this file, `TestDeserializeStream`, `TestDeserializeTopic`, `TestDeserializeConsumerGroup_WithMembers` do not have comments on what exactly is being serialized and then deserialized, but the tests after these have comments. Maybe adding comments would help in understanding better what is being serialized
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "slbotbm (via GitHub)" <gi...@apache.org>.
slbotbm commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r2999342285
##########
foreign/go/binary_serialization/binary_response_deserializer_test.go:
##########
Review Comment:
Please ignore the above comment. Github was somehow showing me old code. LGTM
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive unit tests [iggy]
Posted by "slbotbm (via GitHub)" <gi...@apache.org>.
slbotbm commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r2997382426
##########
foreign/go/errors/errors_gen_test.go:
##########
Review Comment:
The error kinds are getting duplicated here. Is there a better way to do this?
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3037:
URL: https://github.com/apache/iggy/pull/3037#issuecomment-4277235348
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs.
If you need a review, please ensure CI is green and the PR is rebased on the latest master. Don't hesitate to ping the maintainers - either @core on Discord or by mentioning them directly here on the PR.
Thank you for your contribution!
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r3002479343
##########
foreign/go/contracts/cluster_node_test.go:
##########
@@ -0,0 +1,105 @@
+// 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 iggcon
+
+import (
+ "strings"
+ "testing"
+)
+
+func TestClusterNode_MarshalUnmarshal_Roundtrip(t *testing.T) {
+ original := ClusterNode{
+ Name: "node-1",
+ IP: "192.168.1.10",
+ Endpoints: NewTransportEndpoints(8090, 8091, 3000, 8080),
+ Role: RoleLeader,
+ Status: Healthy,
+ }
+
+ data, err := original.MarshalBinary()
+ if err != nil {
+ t.Fatalf("unexpected marshal error: %v", err)
+ }
+
+ var decoded ClusterNode
+ if err := decoded.UnmarshalBinary(data); err != nil {
+ t.Fatalf("unexpected unmarshal error: %v", err)
+ }
+
+ if decoded.Name != original.Name {
+ t.Fatalf("Name mismatch: expected %s, got %s", original.Name, decoded.Name)
+ }
+ if decoded.IP != original.IP {
+ t.Fatalf("IP mismatch: expected %s, got %s", original.IP, decoded.IP)
+ }
+ if decoded.Endpoints.Tcp != original.Endpoints.Tcp {
+ t.Fatalf("Tcp mismatch: expected %d, got %d", original.Endpoints.Tcp, decoded.Endpoints.Tcp)
+ }
+ if decoded.Endpoints.Quic != original.Endpoints.Quic {
+ t.Fatalf("Quic mismatch: expected %d, got %d", original.Endpoints.Quic, decoded.Endpoints.Quic)
+ }
+ if decoded.Endpoints.Http != original.Endpoints.Http {
+ t.Fatalf("Http mismatch: expected %d, got %d", original.Endpoints.Http, decoded.Endpoints.Http)
+ }
+ if decoded.Endpoints.WebSocket != original.Endpoints.WebSocket {
+ t.Fatalf("WebSocket mismatch: expected %d, got %d", original.Endpoints.WebSocket, decoded.Endpoints.WebSocket)
+ }
+ if decoded.Role != original.Role {
+ t.Fatalf("Role mismatch: expected %d, got %d", original.Role, decoded.Role)
+ }
+ if decoded.Status != original.Status {
+ t.Fatalf("Status mismatch: expected %d, got %d", original.Status, decoded.Status)
+ }
+}
Review Comment:
consider use `github.com/google/go-cmp/cmp` or `reflect.DeepEqual`. I personally prever the first approach.
##########
foreign/go/contracts/cluster_node_test.go:
##########
@@ -0,0 +1,105 @@
+// 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 iggcon
+
+import (
+ "strings"
+ "testing"
+)
+
+func TestClusterNode_MarshalUnmarshal_Roundtrip(t *testing.T) {
+ original := ClusterNode{
+ Name: "node-1",
+ IP: "192.168.1.10",
+ Endpoints: NewTransportEndpoints(8090, 8091, 3000, 8080),
+ Role: RoleLeader,
+ Status: Healthy,
+ }
+
+ data, err := original.MarshalBinary()
+ if err != nil {
+ t.Fatalf("unexpected marshal error: %v", err)
+ }
+
+ var decoded ClusterNode
+ if err := decoded.UnmarshalBinary(data); err != nil {
+ t.Fatalf("unexpected unmarshal error: %v", err)
+ }
+
+ if decoded.Name != original.Name {
+ t.Fatalf("Name mismatch: expected %s, got %s", original.Name, decoded.Name)
+ }
+ if decoded.IP != original.IP {
+ t.Fatalf("IP mismatch: expected %s, got %s", original.IP, decoded.IP)
+ }
+ if decoded.Endpoints.Tcp != original.Endpoints.Tcp {
+ t.Fatalf("Tcp mismatch: expected %d, got %d", original.Endpoints.Tcp, decoded.Endpoints.Tcp)
+ }
+ if decoded.Endpoints.Quic != original.Endpoints.Quic {
+ t.Fatalf("Quic mismatch: expected %d, got %d", original.Endpoints.Quic, decoded.Endpoints.Quic)
+ }
+ if decoded.Endpoints.Http != original.Endpoints.Http {
+ t.Fatalf("Http mismatch: expected %d, got %d", original.Endpoints.Http, decoded.Endpoints.Http)
+ }
+ if decoded.Endpoints.WebSocket != original.Endpoints.WebSocket {
+ t.Fatalf("WebSocket mismatch: expected %d, got %d", original.Endpoints.WebSocket, decoded.Endpoints.WebSocket)
+ }
+ if decoded.Role != original.Role {
+ t.Fatalf("Role mismatch: expected %d, got %d", original.Role, decoded.Role)
+ }
+ if decoded.Status != original.Status {
+ t.Fatalf("Status mismatch: expected %d, got %d", original.Status, decoded.Status)
+ }
+}
Review Comment:
consider use `github.com/google/go-cmp/cmp` or `reflect.DeepEqual`. I personally prever the `cmp` approach.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r3002495808
##########
foreign/go/contracts/cluster_node_test.go:
##########
@@ -0,0 +1,105 @@
+// 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 iggcon
+
+import (
+ "strings"
+ "testing"
+)
+
+func TestClusterNode_MarshalUnmarshal_Roundtrip(t *testing.T) {
+ original := ClusterNode{
+ Name: "node-1",
+ IP: "192.168.1.10",
+ Endpoints: NewTransportEndpoints(8090, 8091, 3000, 8080),
+ Role: RoleLeader,
+ Status: Healthy,
+ }
+
+ data, err := original.MarshalBinary()
+ if err != nil {
+ t.Fatalf("unexpected marshal error: %v", err)
+ }
+
+ var decoded ClusterNode
+ if err := decoded.UnmarshalBinary(data); err != nil {
+ t.Fatalf("unexpected unmarshal error: %v", err)
+ }
+
+ if decoded.Name != original.Name {
+ t.Fatalf("Name mismatch: expected %s, got %s", original.Name, decoded.Name)
+ }
+ if decoded.IP != original.IP {
+ t.Fatalf("IP mismatch: expected %s, got %s", original.IP, decoded.IP)
+ }
+ if decoded.Endpoints.Tcp != original.Endpoints.Tcp {
+ t.Fatalf("Tcp mismatch: expected %d, got %d", original.Endpoints.Tcp, decoded.Endpoints.Tcp)
+ }
+ if decoded.Endpoints.Quic != original.Endpoints.Quic {
+ t.Fatalf("Quic mismatch: expected %d, got %d", original.Endpoints.Quic, decoded.Endpoints.Quic)
+ }
+ if decoded.Endpoints.Http != original.Endpoints.Http {
+ t.Fatalf("Http mismatch: expected %d, got %d", original.Endpoints.Http, decoded.Endpoints.Http)
+ }
+ if decoded.Endpoints.WebSocket != original.Endpoints.WebSocket {
+ t.Fatalf("WebSocket mismatch: expected %d, got %d", original.Endpoints.WebSocket, decoded.Endpoints.WebSocket)
+ }
+ if decoded.Role != original.Role {
+ t.Fatalf("Role mismatch: expected %d, got %d", original.Role, decoded.Role)
+ }
+ if decoded.Status != original.Status {
+ t.Fatalf("Status mismatch: expected %d, got %d", original.Status, decoded.Status)
+ }
+}
+
+func TestClusterNode_BufferSize_MatchesMarshaledLength(t *testing.T) {
+ node := ClusterNode{
+ Name: "node-1",
+ IP: "192.168.1.10",
+ Endpoints: NewTransportEndpoints(8090, 8091, 3000, 8080),
+ Role: RoleFollower,
+ Status: Starting,
+ }
+
+ data, err := node.MarshalBinary()
+ if err != nil {
+ t.Fatalf("unexpected marshal error: %v", err)
+ }
+
+ if len(data) != node.BufferSize() {
+ t.Fatalf("expected marshaled length %d to match BufferSize %d", len(data), node.BufferSize())
+ }
+}
+
+func TestClusterNode_String_ContainsNameAndIP(t *testing.T) {
+ node := ClusterNode{
+ Name: "test-node",
+ IP: "10.0.0.1",
+ Endpoints: NewTransportEndpoints(1, 2, 3, 4),
+ Role: RoleLeader,
+ Status: Healthy,
+ }
+
+ s := node.String()
+ if !strings.Contains(s, "test-node") {
+ t.Fatalf("expected string to contain 'test-node', got %s", s)
+ }
+ if !strings.Contains(s, "10.0.0.1") {
+ t.Fatalf("expected string to contain '10.0.0.1', got %s", s)
+ }
+}
Review Comment:
`String()` tests, I am not sure if it worth it. Actually it is method that was never used.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "slbotbm (via GitHub)" <gi...@apache.org>.
slbotbm commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r2999250683
##########
foreign/go/binary_serialization/binary_response_deserializer_test.go:
##########
Review Comment:
Previously, I did not make myself clear on what type comment I was talking about. I apologize for the confusion.
I was thinking comments of the following type:
```
// User: id=42(4) + createdAt=999(8) + status=1(1) + usernameLen=5(1) + "admin"(5) = 19 bytes
// hasPermissions=1(1) + permLen=11(4) + permissions(11) = 16 bytes`
```
Though this is not a blocker to merging
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "slbotbm (via GitHub)" <gi...@apache.org>.
slbotbm commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r2999250683
##########
foreign/go/binary_serialization/binary_response_deserializer_test.go:
##########
Review Comment:
I was thinking comments of the following type:
```
// User: id=42(4) + createdAt=999(8) + status=1(1) + usernameLen=5(1) + "admin"(5) = 19 bytes
// hasPermissions=1(1) + permLen=11(4) + permissions(11) = 16 bytes`
```
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on PR #3037:
URL: https://github.com/apache/iggy/pull/3037#issuecomment-4144229267
@atharvalade I think @saie-ch is already working on the command tests in #2973, so it might be better to drop that part for now to avoid potential conflicts :)
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r3002412870
##########
foreign/go/client/iggy_client_test.go:
##########
Review Comment:
Do we really need to test these things....
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r3002443595
##########
foreign/go/client/iggy_client_test.go:
##########
@@ -0,0 +1,61 @@
+// 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 client
+
+import (
+ "testing"
+ "time"
+
+ "github.com/apache/iggy/foreign/go/client/tcp"
+ iggcon "github.com/apache/iggy/foreign/go/contracts"
+)
+
+func TestGetDefaultOptions(t *testing.T) {
+ opts := GetDefaultOptions()
+ if opts.protocol != iggcon.Tcp {
+ t.Fatalf("expected protocol Tcp, got %v", opts.protocol)
+ }
+ if opts.heartbeatInterval != 5*time.Second {
+ t.Fatalf("expected heartbeat 5s, got %v", opts.heartbeatInterval)
+ }
+ if opts.tcpOptions != nil {
+ t.Fatalf("expected nil tcpOptions")
+ }
+}
+
+func TestWithTcp(t *testing.T) {
+ opts := GetDefaultOptions()
+ opts.protocol = "quic"
+ addrOpt := tcp.WithServerAddress("1.2.3.4:9090")
+ WithTcp(addrOpt)(&opts)
+ if opts.protocol != iggcon.Tcp {
+ t.Fatalf("expected protocol Tcp, got %v", opts.protocol)
+ }
+ if len(opts.tcpOptions) != 1 {
+ t.Fatalf("expected 1 tcp option, got %d", len(opts.tcpOptions))
+ }
+}
Review Comment:
We can use `cli := NewIggyCLient(WithTcp())` and check the cli.protocol is tcp or not
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive unit tests [iggy]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3037:
URL: https://github.com/apache/iggy/pull/3037#issuecomment-4136967174
## [Codecov](https://app.codecov.io/gh/apache/iggy/pull/3037?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `85.71429%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 71.46%. Comparing base ([`f8be024`](https://app.codecov.io/gh/apache/iggy/commit/f8be02437b0bea1d035c5caaa0cc324386a8e8dd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d12c65b`](https://app.codecov.io/gh/apache/iggy/commit/d12c65b35f824b0fc612de5ed626e8d919b24b56?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/iggy/pull/3037?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [foreign/go/internal/command/user.go](https://app.codecov.io/gh/apache/iggy/pull/3037?src=pr&el=tree&filepath=foreign%2Fgo%2Finternal%2Fcommand%2Fuser.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Zm9yZWlnbi9nby9pbnRlcm5hbC9jb21tYW5kL3VzZXIuZ28=) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/3037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3037 +/- ##
============================================
- Coverage 71.82% 71.46% -0.36%
Complexity 930 930
============================================
Files 1117 1113 -4
Lines 93031 92790 -241
Branches 70379 70379
============================================
- Hits 66819 66313 -506
- Misses 23614 23901 +287
+ Partials 2598 2576 -22
```
| [Components](https://app.codecov.io/gh/apache/iggy/pull/3037/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [Rust Core](https://app.codecov.io/gh/apache/iggy/pull/3037/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.44% <ø> (ø)` | |
| [Java SDK](https://app.codecov.io/gh/apache/iggy/pull/3037/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.08% <ø> (ø)` | |
| [C# SDK](https://app.codecov.io/gh/apache/iggy/pull/3037/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.64% <ø> (ø)` | |
| [Python SDK](https://app.codecov.io/gh/apache/iggy/pull/3037/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.43% <ø> (ø)` | |
| [Node SDK](https://app.codecov.io/gh/apache/iggy/pull/3037/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `91.53% <ø> (ø)` | |
| [Go SDK](https://app.codecov.io/gh/apache/iggy/pull/3037/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `29.58% <85.71%> (-9.11%)` | :arrow_down: |
| [Files with missing lines](https://app.codecov.io/gh/apache/iggy/pull/3037?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [foreign/go/internal/command/update\_user.go](https://app.codecov.io/gh/apache/iggy/pull/3037?src=pr&el=tree&filepath=foreign%2Fgo%2Finternal%2Fcommand%2Fupdate_user.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Zm9yZWlnbi9nby9pbnRlcm5hbC9jb21tYW5kL3VwZGF0ZV91c2VyLmdv) | `86.04% <100.00%> (+32.38%)` | :arrow_up: |
| [foreign/go/internal/command/user.go](https://app.codecov.io/gh/apache/iggy/pull/3037?src=pr&el=tree&filepath=foreign%2Fgo%2Finternal%2Fcommand%2Fuser.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Zm9yZWlnbi9nby9pbnRlcm5hbC9jb21tYW5kL3VzZXIuZ28=) | `56.52% <50.00%> (-28.27%)` | :arrow_down: |
... and [44 files with indirect coverage changes](https://app.codecov.io/gh/apache/iggy/pull/3037/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive unit tests [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r2997555093
##########
foreign/go/binary_serialization/binary_response_deserializer_test.go:
##########
Review Comment:
I agree, can you please check now?
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r3002431983
##########
foreign/go/client/iggy_client_test.go:
##########
@@ -0,0 +1,61 @@
+// 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 client
+
+import (
+ "testing"
+ "time"
+
+ "github.com/apache/iggy/foreign/go/client/tcp"
+ iggcon "github.com/apache/iggy/foreign/go/contracts"
+)
+
+func TestGetDefaultOptions(t *testing.T) {
+ opts := GetDefaultOptions()
+ if opts.protocol != iggcon.Tcp {
+ t.Fatalf("expected protocol Tcp, got %v", opts.protocol)
+ }
+ if opts.heartbeatInterval != 5*time.Second {
+ t.Fatalf("expected heartbeat 5s, got %v", opts.heartbeatInterval)
+ }
+ if opts.tcpOptions != nil {
+ t.Fatalf("expected nil tcpOptions")
+ }
+}
Review Comment:
This part is not necessary.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive unit tests [iggy]
Posted by "slbotbm (via GitHub)" <gi...@apache.org>.
slbotbm commented on PR #3037:
URL: https://github.com/apache/iggy/pull/3037#issuecomment-4137884881
Though I do not have experience writing go, I feel that the actual implementation files and the test files are getting mixed up, with only *_test.go indicating whether something is a test file or not. I saw that there is a `tests/` folder in `foreign/go`. Wouldn't shifting these files into `tests/` make more sense?
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3037:
URL: https://github.com/apache/iggy/pull/3037#issuecomment-4230464428
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs.
If you need a review, please ensure CI is green and the PR is rebased on the latest master. Don't hesitate to ping the maintainers - either @core on Discord or by mentioning them directly here on the PR.
Thank you for your contribution!
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive unit tests [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on PR #3037:
URL: https://github.com/apache/iggy/pull/3037#issuecomment-4138063532
> Though I do not have experience writing go, I feel that the actual implementation files and the test files are getting mixed up, with only *_test.go indicating whether something is a test file or not. I saw that there is a `tests/` folder in `foreign/go`. Wouldn't shifting these files into `tests/` make more sense?
thanks for the review, that's a great thought, but in Go the standard convention is to co-locate `*_test.go` files alongside the package they test... this is how the Go `stdlib`, Kubernetes, and Docker all do it, and it enables white-box testing of unexported internals. The existing `foreign/go/tests/` directory specifically holds integration/e2e tests that need a running server, so the separation is already intentional. @slbotbm
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3037:
URL: https://github.com/apache/iggy/pull/3037#issuecomment-4185845232
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs.
If you need a review, please ensure CI is green and the PR is rebased on the latest master. Don't hesitate to ping the maintainers - either @core on Discord or by mentioning them directly here on the PR.
Thank you for your contribution!
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r3002479343
##########
foreign/go/contracts/cluster_node_test.go:
##########
@@ -0,0 +1,105 @@
+// 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 iggcon
+
+import (
+ "strings"
+ "testing"
+)
+
+func TestClusterNode_MarshalUnmarshal_Roundtrip(t *testing.T) {
+ original := ClusterNode{
+ Name: "node-1",
+ IP: "192.168.1.10",
+ Endpoints: NewTransportEndpoints(8090, 8091, 3000, 8080),
+ Role: RoleLeader,
+ Status: Healthy,
+ }
+
+ data, err := original.MarshalBinary()
+ if err != nil {
+ t.Fatalf("unexpected marshal error: %v", err)
+ }
+
+ var decoded ClusterNode
+ if err := decoded.UnmarshalBinary(data); err != nil {
+ t.Fatalf("unexpected unmarshal error: %v", err)
+ }
+
+ if decoded.Name != original.Name {
+ t.Fatalf("Name mismatch: expected %s, got %s", original.Name, decoded.Name)
+ }
+ if decoded.IP != original.IP {
+ t.Fatalf("IP mismatch: expected %s, got %s", original.IP, decoded.IP)
+ }
+ if decoded.Endpoints.Tcp != original.Endpoints.Tcp {
+ t.Fatalf("Tcp mismatch: expected %d, got %d", original.Endpoints.Tcp, decoded.Endpoints.Tcp)
+ }
+ if decoded.Endpoints.Quic != original.Endpoints.Quic {
+ t.Fatalf("Quic mismatch: expected %d, got %d", original.Endpoints.Quic, decoded.Endpoints.Quic)
+ }
+ if decoded.Endpoints.Http != original.Endpoints.Http {
+ t.Fatalf("Http mismatch: expected %d, got %d", original.Endpoints.Http, decoded.Endpoints.Http)
+ }
+ if decoded.Endpoints.WebSocket != original.Endpoints.WebSocket {
+ t.Fatalf("WebSocket mismatch: expected %d, got %d", original.Endpoints.WebSocket, decoded.Endpoints.WebSocket)
+ }
+ if decoded.Role != original.Role {
+ t.Fatalf("Role mismatch: expected %d, got %d", original.Role, decoded.Role)
+ }
+ if decoded.Status != original.Status {
+ t.Fatalf("Status mismatch: expected %d, got %d", original.Status, decoded.Status)
+ }
+}
Review Comment:
consider use `github.com/google/go-cmp/cmp` or `reflect.DeepEqual`. I personally prever the `cmp` package.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3037:
URL: https://github.com/apache/iggy/pull/3037#discussion_r3002495808
##########
foreign/go/contracts/cluster_node_test.go:
##########
@@ -0,0 +1,105 @@
+// 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 iggcon
+
+import (
+ "strings"
+ "testing"
+)
+
+func TestClusterNode_MarshalUnmarshal_Roundtrip(t *testing.T) {
+ original := ClusterNode{
+ Name: "node-1",
+ IP: "192.168.1.10",
+ Endpoints: NewTransportEndpoints(8090, 8091, 3000, 8080),
+ Role: RoleLeader,
+ Status: Healthy,
+ }
+
+ data, err := original.MarshalBinary()
+ if err != nil {
+ t.Fatalf("unexpected marshal error: %v", err)
+ }
+
+ var decoded ClusterNode
+ if err := decoded.UnmarshalBinary(data); err != nil {
+ t.Fatalf("unexpected unmarshal error: %v", err)
+ }
+
+ if decoded.Name != original.Name {
+ t.Fatalf("Name mismatch: expected %s, got %s", original.Name, decoded.Name)
+ }
+ if decoded.IP != original.IP {
+ t.Fatalf("IP mismatch: expected %s, got %s", original.IP, decoded.IP)
+ }
+ if decoded.Endpoints.Tcp != original.Endpoints.Tcp {
+ t.Fatalf("Tcp mismatch: expected %d, got %d", original.Endpoints.Tcp, decoded.Endpoints.Tcp)
+ }
+ if decoded.Endpoints.Quic != original.Endpoints.Quic {
+ t.Fatalf("Quic mismatch: expected %d, got %d", original.Endpoints.Quic, decoded.Endpoints.Quic)
+ }
+ if decoded.Endpoints.Http != original.Endpoints.Http {
+ t.Fatalf("Http mismatch: expected %d, got %d", original.Endpoints.Http, decoded.Endpoints.Http)
+ }
+ if decoded.Endpoints.WebSocket != original.Endpoints.WebSocket {
+ t.Fatalf("WebSocket mismatch: expected %d, got %d", original.Endpoints.WebSocket, decoded.Endpoints.WebSocket)
+ }
+ if decoded.Role != original.Role {
+ t.Fatalf("Role mismatch: expected %d, got %d", original.Role, decoded.Role)
+ }
+ if decoded.Status != original.Status {
+ t.Fatalf("Status mismatch: expected %d, got %d", original.Status, decoded.Status)
+ }
+}
+
+func TestClusterNode_BufferSize_MatchesMarshaledLength(t *testing.T) {
+ node := ClusterNode{
+ Name: "node-1",
+ IP: "192.168.1.10",
+ Endpoints: NewTransportEndpoints(8090, 8091, 3000, 8080),
+ Role: RoleFollower,
+ Status: Starting,
+ }
+
+ data, err := node.MarshalBinary()
+ if err != nil {
+ t.Fatalf("unexpected marshal error: %v", err)
+ }
+
+ if len(data) != node.BufferSize() {
+ t.Fatalf("expected marshaled length %d to match BufferSize %d", len(data), node.BufferSize())
+ }
+}
+
+func TestClusterNode_String_ContainsNameAndIP(t *testing.T) {
+ node := ClusterNode{
+ Name: "test-node",
+ IP: "10.0.0.1",
+ Endpoints: NewTransportEndpoints(1, 2, 3, 4),
+ Role: RoleLeader,
+ Status: Healthy,
+ }
+
+ s := node.String()
+ if !strings.Contains(s, "test-node") {
+ t.Fatalf("expected string to contain 'test-node', got %s", s)
+ }
+ if !strings.Contains(s, "10.0.0.1") {
+ t.Fatalf("expected string to contain '10.0.0.1', got %s", s)
+ }
+}
Review Comment:
I’m not sure it’s worth it tbh. I think the implementation of `String()` already makes the exact format clear, and changes to it should naturally be caught during code review.
Also, String() doesn’t seem to be used anywhere right now, and its output is pretty similar to `fmt.Printf("%#v", val)`. We could probably consider removing `String()`s in a separate PR.
I checked the git blame and the person who write this `String()` method is me LOL. I guess it's because a previous refactor or something.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(go): increase Go SDK Codecov coverage with comprehensive tests [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3037:
URL: https://github.com/apache/iggy/pull/3037#issuecomment-4331539841
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs.
If you need a review, please ensure CI is green and the PR is rebased on the latest master. Don't hesitate to ping the maintainers - either @core on Discord or by mentioning them directly here on the PR.
Thank you for your contribution!
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org