Skip to content

feat(go): add comprehensive unit tests for command serialization#2973

Closed
saie-ch wants to merge 5 commits into
apache:masterfrom
saie-ch:go_unit_tests
Closed

feat(go): add comprehensive unit tests for command serialization#2973
saie-ch wants to merge 5 commits into
apache:masterfrom
saie-ch:go_unit_tests

Conversation

@saie-ch
Copy link
Copy Markdown
Contributor

@saie-ch saie-ch commented Mar 18, 2026

Which issue does this PR close?

Closes #2883

Rationale

The Go SDK contains numerous command types that implement the MarshalBinary interface for binary serialization, but many lacked comprehensive unit tests. Without thorough testing, serialization bugs can cause data corruption, runtime panics, and wire protocol incompatibility with the Iggy server and other SDKs.

What changed?

Before: Go SDK had only 6 basic tests for command serialization with no edge case coverage.

After: 88 unit tests (138 including subtests) covering all command types.

Bugs Fixed:

  1. CreatePersonalAccessTokenExpiry field was uint32 serialized with PutUint32 at the wrong offset; changed to uint64 with PutUint64 to
    match the Rust wire format (u64_le)
    2. CreateTopic / UpdateTopicMarshalBinary mutated the receiver by assigning t.ReplicationFactor = new(uint8) when nil; replaced with a local
    variable
    3. UpdateUserMarshalBinary mutated the receiver by assigning u.Username = new(string) when nil; replaced with a local variable and fixed
    buffer size calculation

Test Coverage:

  • Overall package: with 82.2% coverage.
  • All tests pass with the race detector enabled.

What this PR does NOT fix

Local Execution:

All 88 tests pass with race detector enabled and 82.2% statement coverage

AI Usage

Tool: Claude Code (claude-sonnet-4.5)

Scope: Test implementation, bug discovery, and cross-SDK validation

  • Helped design comprehensive test coverage strategy
  • Generated test structures following Go best practices

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.10%. Comparing base (7aa4539) to head (026cff8).

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2973      +/-   ##
============================================
- Coverage     74.16%   74.10%   -0.06%     
  Complexity      943      943              
============================================
  Files          1237     1201      -36     
  Lines        112641   110445    -2196     
  Branches      89201    87518    -1683     
============================================
- Hits          83536    81843    -1693     
+ Misses        26309    25873     -436     
+ Partials       2796     2729      -67     
Components Coverage Δ
Rust Core 75.26% <ø> (-0.04%) ⬇️
Java SDK 58.44% <ø> (ø)
C# SDK 69.43% <ø> (-1.23%) ⬇️
Python SDK 81.43% <ø> (ø)
Node SDK 91.53% <ø> (+0.12%) ⬆️
Go SDK 40.37% <100.00%> (+0.45%) ⬆️
Files with missing lines Coverage Δ
...reign/go/client/tcp/tcp_access_token_management.go 100.00% <100.00%> (ø)
foreign/go/internal/command/access_token.go 100.00% <100.00%> (ø)
foreign/go/internal/command/topic.go 92.10% <100.00%> (+0.21%) ⬆️
foreign/go/internal/command/update_user.go 95.12% <100.00%> (+41.46%) ⬆️
foreign/go/internal/command/user.go 91.30% <100.00%> (ø)

... and 73 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@atharvalade atharvalade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looking good but I found a few things that need to be addressed

Comment thread foreign/go/internal/command/access_token_test.go
Comment thread foreign/go/internal/command/user_test.go
Comment thread foreign/go/internal/command/user_test.go
@atharvalade
Copy link
Copy Markdown
Contributor

atharvalade commented Mar 19, 2026

I found 3 pre-existing issues #2980 #2981 #2982
I will wait for this merge and @saie-ch's comments before starting work on the issues mentioned above.

@hubcio
Copy link
Copy Markdown
Contributor

hubcio commented Mar 19, 2026

@chengxilo could you please also check this?

Comment thread foreign/go/contracts/users.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part should not use byte(0) directly, doesn't align with the behavior in rust sdk.

if current_stream < streams_count {
current_stream += 1;
bytes.put_u8(1);
} else {
bytes.put_u8(0);
}
}

@saie-ch would you mind to solve this in another PR first (to Implement unit test for permission and fix the problem in permission)?

Comment thread foreign/go/contracts/users.go
Comment on lines 39 to 43
@@ -43,14 +43,14 @@ func (u *UpdateUser) MarshalBinary() ([]byte, error) {
username := *u.Username
Copy link
Copy Markdown
Contributor

@chengxilo chengxilo Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the u.Username is actually modified by the method, I don't think it's a good approach. Since you already modifed this method, would you mind to modified this too?

@chengxilo
Copy link
Copy Markdown
Contributor

regarding #2973 (comment), I think it would be great to write tests to check whether they are modified after call MarshalBinary, for each command.

@saie-ch
Copy link
Copy Markdown
Contributor Author

saie-ch commented Mar 20, 2026

Thanks @atharvalade and @chengxilo, I will implement the necessary changes.

@hubcio
Copy link
Copy Markdown
Contributor

hubcio commented Mar 26, 2026

@saie-ch @chengxilo what's the status of this PR? is it ready to be merged? should #3015 be merged first?

@saie-ch
Copy link
Copy Markdown
Contributor Author

saie-ch commented Mar 26, 2026

@saie-ch @chengxilo what's the status of this PR? is it ready to be merged? should #3015 be merged first?

Yes please @hubcio, let's wait for that to be merged first. I just pushed new changes.

@chengxilo
Copy link
Copy Markdown
Contributor

chengxilo commented Mar 26, 2026

This PR may still need some further review and improvement. We should merge #3015 first tho.

@hubcio
Copy link
Copy Markdown
Contributor

hubcio commented Mar 30, 2026

#3015 is merged.

@hubcio
Copy link
Copy Markdown
Contributor

hubcio commented Mar 30, 2026

@saie-ch @atharvalade what is the difference between this PR and #3037 ?

@chengxilo
Copy link
Copy Markdown
Contributor

@saie-ch @atharvalade what is the difference between this PR and #3037 ?

This one is created a few days earlier and only covers the test for commands.🤕

@github-actions
Copy link
Copy Markdown

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!

@github-actions github-actions Bot added S-stale Inactive issue or pull request and removed S-stale Inactive issue or pull request labels Apr 12, 2026
@github-actions
Copy link
Copy Markdown

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!

@github-actions github-actions Bot added S-stale Inactive issue or pull request and removed S-stale Inactive issue or pull request labels Apr 25, 2026
@atharvalade
Copy link
Copy Markdown
Contributor

@saie-ch do you plan to implement anything else now that #3015 is merged? Once I get a green light from you, I can start review again and plan to merge this too after reviews. BTW I closed #3037 because of multiple conflicts and outdated code.

@saie-ch
Copy link
Copy Markdown
Contributor Author

saie-ch commented Apr 29, 2026

@saie-ch do you plan to implement anything else now that #3015 is merged? Once I get a green light from you, I can start review again and plan to merge this too after reviews. BTW I closed #3037 because of multiple conflicts and outdated code.

Yes, please review. I believe, i've implemented the necessary changes.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

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!

@github-actions github-actions Bot added S-stale Inactive issue or pull request and removed S-stale Inactive issue or pull request labels May 8, 2026
@hubcio
Copy link
Copy Markdown
Contributor

hubcio commented May 14, 2026

/ready

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label May 14, 2026
@hubcio
Copy link
Copy Markdown
Contributor

hubcio commented May 14, 2026

@saie-ch I noticed that comments from this PR are not fixed (output from claude code):

  #: 2958694591
  Reviewer: atharvalade
  Location: access_token.go:36
  Ask: Expiry field → uint64, use PutUint64 (current PutUint32 writes expiry into high 32 bits → server reads expiry << 32)
  Status: ❌ NOT fixed — file never touched. Line 24 still uint32, line 36 still PutUint32
  ────────────────────────────────────────
  #: 2958698450
  Reviewer: atharvalade
  Location: user.go:77 (CreateUser)
  Ask: nil-permissions branch must also write permissions_len:u32_le=0
  Status: ❌ NOT fixed — else branch still writes only 1 flag byte; capacity has no +4 for nil case
  ────────────────────────────────────────
  #: 2958701940
  Reviewer: atharvalade
  Location: user.go UpdatePermissions
  Ask: base length = len(userIdBytes)+1+4; else branch must write 4 zero bytes
  Status: ❌ NOT fixed — line 119 still len(userIdBytes)+1, else branch line 142 still single 0 byte. Code untouched
  ────────────────────────────────────────
  #: 2962372305
  Reviewer: chengxilo
  Location: update_user.go:43
  Ask: stop mutating receiver u.Username
  Status: ❌ NOT fixed — lines 39-43 still assign u.Username = new(string) then deref

can you fix them?

/author

@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels May 14, 2026
@saie-ch
Copy link
Copy Markdown
Contributor Author

saie-ch commented May 14, 2026

@saie-ch I noticed that comments from this PR are not fixed (output from claude code):

  #: 2958694591
  Reviewer: atharvalade
  Location: access_token.go:36
  Ask: Expiry field → uint64, use PutUint64 (current PutUint32 writes expiry into high 32 bits → server reads expiry << 32)
  Status: ❌ NOT fixed — file never touched. Line 24 still uint32, line 36 still PutUint32
  ────────────────────────────────────────
  #: 2958698450
  Reviewer: atharvalade
  Location: user.go:77 (CreateUser)
  Ask: nil-permissions branch must also write permissions_len:u32_le=0
  Status: ❌ NOT fixed — else branch still writes only 1 flag byte; capacity has no +4 for nil case
  ────────────────────────────────────────
  #: 2958701940
  Reviewer: atharvalade
  Location: user.go UpdatePermissions
  Ask: base length = len(userIdBytes)+1+4; else branch must write 4 zero bytes
  Status: ❌ NOT fixed — line 119 still len(userIdBytes)+1, else branch line 142 still single 0 byte. Code untouched
  ────────────────────────────────────────
  #: 2962372305
  Reviewer: chengxilo
  Location: update_user.go:43
  Ask: stop mutating receiver u.Username
  Status: ❌ NOT fixed — lines 39-43 still assign u.Username = new(string) then deref

can you fix them?

/author

thanks @hubcio , will try to fix them.

@saie-ch
Copy link
Copy Markdown
Contributor Author

saie-ch commented May 17, 2026

@hubcio Fixed #1 (Expiry uint32→uint64) and #4 (stop mutating receiver). Also fixed the same mutation pattern in CreateTopic and UpdateTopic, and added non-mutation tests per @chengxilo's ask.

For #2 and #3 — the reviewer's suggestion to write permissions_len:u32_le=0 in the nil case is incorrect per the Rust reference. Both create_user.rs:63-64 and update_permissions.rs:53-54 only write put_u8(0) when permissions are None — no permissions_len field. The Rust test at create_user.rs:201 confirms: &[1,b'u', 1, b'p', 0, 0]. The real buffer bugs were already fixed by PR #3097. The Go code matches the Rust wire format as-is.

@hubcio
Copy link
Copy Markdown
Contributor

hubcio commented May 18, 2026

you can use /ready at the beginning of the line to change label.

/ready

@saie-ch i will check this later in upcoming days.

@github-actions github-actions Bot added S-waiting-on-review PR is waiting on a reviewer and removed S-waiting-on-author PR is waiting on author response labels May 18, 2026
Copy link
Copy Markdown
Contributor

@hubcio hubcio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few items that don't map to changed lines in this diff:

  • foreign/go/contracts/users.go Permissions.MarshalBinary iterates p.Streams and stream.Topics (go maps) without sorting keys, so the produced wire bytes are non-deterministic across runs. two calls on the same Permissions value can produce different byte sequences. this file is not touched in this pr, but the new tests do not catch it either (since the structures used have either nil or single-entry maps). worth either fixing here (sort keys before iterating, e.g. slices.Sorted(maps.Keys(p.Streams))) or filing a follow-up.

  • pr description says "Permissions.MarshalBinary() - Fixed first permission byte corruption when streams are nil" but users.go is unchanged in this pr. that fix landed earlier in #3015 (169a09bcd). consider rewording to "verified by new tests" or dropping the claim.

  • pr description says "CreateUser - Fixed off-by-one buffer allocation (extra trailing byte)" but old capacity = 4 + N + M and new 1 + N + 1 + M + 1 + 1 = 4 + N + M are arithmetically identical. the change is a readability refactor, not a bugfix.

bytes[0] = byte(len(c.Name))
copy(bytes[1:], c.Name)
binary.LittleEndian.PutUint32(bytes[len(bytes)-4:], c.Expiry)
binary.LittleEndian.PutUint64(bytes[1+len(c.Name):], c.Expiry)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy(bytes[1:], c.Name) writes into a dest of size N+8 but only copies N bytes. works, but reads as if it could overflow. copy(bytes[1:1+len(c.Name)], c.Name) is clearer and lines up with the explicit bytes[1+len(c.Name):] index on the next line.

Comment on lines 44 to 46
if len(username) != 0 {
length += 2 + len(username)
length += 1 + len(username)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

len(username) == 0 collapses Username: stringPtr("") to the same wire bytes as Username: nil (has-username=0). that means a caller cannot distinguish "clear the username" from "leave it untouched" - the server sees the latter in both cases. TestSerialize_UpdateUser_EmptyUsername locks this behavior in. is the collapse intentional? if yes, leave a comment explaining it; if no, gate on the pointer (u.Username != nil) instead of the string length.

Comment on lines +30 to +60
func buildExpectedLoginUser(username, password string) []byte {
versionBytes := []byte(iggcon.Version)
contextBytes := []byte("")

totalLength := 1 + len(username) + 1 + len(password) +
4 + len(versionBytes) + 4 + len(contextBytes)

buf := make([]byte, totalLength)
pos := 0

buf[pos] = byte(len(username))
pos++
copy(buf[pos:], username)
pos += len(username)

buf[pos] = byte(len(password))
pos++
copy(buf[pos:], password)
pos += len(password)

binary.LittleEndian.PutUint32(buf[pos:], uint32(len(versionBytes)))
pos += 4
copy(buf[pos:], versionBytes)
pos += len(versionBytes)

binary.LittleEndian.PutUint32(buf[pos:], uint32(len(contextBytes)))
pos += 4
copy(buf[pos:], contextBytes)

return buf
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buildExpectedLoginUser rebuilds the expected bytes the same way the production code does (1-byte len prefix, iggcon.Version, same offsets). if prod is wrong, expected is wrong in lockstep and the assertion passes silently. consider either (a) hardcoding golden bytes for one fixed case with a known/substituted version, or (b) asserting each field individually against literal byte values instead of a mirror helper. the version case alone (TestSerialize_LoginUser_ContainsVersion) reads version from the same offset it was written at, so a wrong offset on both sides would still pass.

Comment on lines +43 to +48
perms := &iggcon.Permissions{
Global: iggcon.GlobalPermissions{
ManageServers: true,
ReadServers: true,
},
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snap here (and &u / &s / &rf / perms in the cases below) shares pointers with cmd. reflect.DeepEqual on the dereferenced struct values walks through pointers and compares the same backing memory, so a MarshalBinary that mutated *Permissions content (e.g. sorted Streams in place) or wrote through *Username would not be caught. the test only catches pointer reassignment - which is what the old t.ReplicationFactor = new(uint8) bug did, so it works for the fixes in this pr, but the name promises more than it delivers. deep-copy the receiver into snapshot (e.g. via a small helper that returns a value with freshly-allocated *string/*uint8/*Permissions) to actually verify no-mutation.

Copy link
Copy Markdown
Contributor

@slbotbm slbotbm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look alright to me, though there is no validation for the input in most of the implementations that these tests test, and thus there are no negative tests. That means these will silently. This can be a follow-up PR.

Also, I noticed that there were tests testing serialization of empty strings. Aren't those supposed to be errors? Adding them shows that they are expected behaviour, not errors.

Comment on lines +151 to +153
cmd := CreatePersonalAccessToken{
Name: "long_token",
Expiry: 18446744073709551615, // Max uint64 value
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a magic number. The following can be used instead:

import "math"

x := uint64(math.MaxUint64)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like internal/command/access_token.go does not have error paths for the commands, always returning bytes, nil or []bytes, nil. Thus cases like Name > 255 chars is not handled by the implementation and would not raise an error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation could be a follow to this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this file as access_token.go. Validation for things like Name > 255 chars is not happening.

While CreateConsumerGroup, GetConsumerGroups, etc. return an error, there is no meaningful local validation / error paths.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previously stated, no negative tests asserting MarshalBinary() errors.

Comment on lines +33 to +35
func TestSerialize_StoreConsumerOffsetRequest_WithPartition(t *testing.T) {
consumerId, _ := iggcon.NewIdentifier(uint32(42))
streamId, _ := iggcon.NewIdentifier("stream1")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test with max uint32 can also be added for StoreConsumerOffsetRequest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same pattern as the other files here - no negative/error-case tests here as well, and no validation in session.go

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as others - no negative/error-case tests and no validation in system.go.

Comment on lines +132 to +133
ClientID: 4294967295, // Max uint32 value
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this - no negative tests.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same - no negative tests.

@slbotbm
Copy link
Copy Markdown
Contributor

slbotbm commented May 19, 2026

/author

@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels May 19, 2026
@saie-ch
Copy link
Copy Markdown
Contributor Author

saie-ch commented May 24, 2026

The changes look alright to me, though there is no validation for the input in most of the implementations that these tests test, and thus there are no negative tests. That means these will silently. This can be a follow-up PR.

Also, I noticed that there were tests testing serialization of empty strings. Aren't those supposed to be errors? Adding them shows that they are expected behaviour, not errors.

The empty string tests reflect the current serialization behavior. Should we remove them until validation is added in the follow-up PR, as you said ?

@slbotbm
Copy link
Copy Markdown
Contributor

slbotbm commented May 25, 2026

The changes look alright to me, though there is no validation for the input in most of the implementations that these tests test, and thus there are no negative tests. That means these will silently. This can be a follow-up PR.
Also, I noticed that there were tests testing serialization of empty strings. Aren't those supposed to be errors? Adding them shows that they are expected behaviour, not errors.

The empty string tests reflect the current serialization behavior. Should we remove them until validation is added in the follow-up PR, as you said ?

maybe @chengxilo would be a better person to ask

@ryankert01
Copy link
Copy Markdown
Member

I think we can change the title of this pr because it also somehow changes some code and fix bug?

@chengxilo
Copy link
Copy Markdown
Contributor

chengxilo commented May 26, 2026

The changes look alright to me, though there is no validation for the input in most of the implementations that these tests test, and thus there are no negative tests. That means these will silently. This can be a follow-up PR.
Also, I noticed that there were tests testing serialization of empty strings. Aren't those supposed to be errors? Adding them shows that they are expected behaviour, not errors.

The empty string tests reflect the current serialization behavior. Should we remove them until validation is added in the follow-up PR, as you said ?

maybe @chengxilo would be a better person to ask

If they are errors, I think it's better to just remove them. The tests should enforce correct behavior rather than just reflecting the current implementation. (I don't think I am a "better person" tho)

@hubcio
Copy link
Copy Markdown
Contributor

hubcio commented May 26, 2026

Closing this due to lack of value added, no activity from author and code being too hard to review.

@hubcio hubcio closed this May 26, 2026
@github-actions github-actions Bot removed the S-waiting-on-author PR is waiting on author response label May 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Go SDK: Add unit tests for command serialization/deserialization

6 participants